On Tue, Apr 12 2022, Philippe Blain wrote:
[A change of $subject seems in order]
> Le 2022-04-12 à 13:04, Junio C Hamano a écrit :
>>
>>
>> Security releases for the 2.30-2.35 maintenance tracks have been
>> tagged to address CVE-2022-24765, which allows a user to trick other
>> users into running a command of their choice easily on multi-user
>> machines with a shared "mob" directory. The fix has been also
>> merged to Git 2.36-rc2 and to all integration branches.
>>
>
> This is quite a big behaviour change for some environments [1], so I would think maybe it
> deserves to be fully spelled out in the release notes for 2.36.0,
> instead of just referring readers to the release notes for the maintenance
> release, where they can read a full description only in the release notes
> for 2.30.3 ?
Yes, I think it deserves to be noted very prominently, and also that we
had some mechanism for publishing relevant git-security@ discussions
(possibly with some parts redacted) after the issues become public.
Non knowing if others involved are OK with being quoted I'll just say
that this issue was discussed at some length on the list, in particular
that it'll severely hinder some core.sharedRepository workflows.
Quoting (part of) my own reply from one of those exchanges (this is in
reply to Johannes Schindelin):
But I don't understand why we need to immediately die() when we detect
this situation in setup.c.
Why don't we just detect it, then set a:
naughty_fsmonitor = "/scratch/.git"
And then later:
diff --git a/config.c b/config.c
index 383b1a4885b..c9ac12e47b0 100644
--- a/config.c
+++ b/config.c
@@ -2630,6 +2630,9 @@ int git_config_get_fsmonitor(void)
{
if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
+ else if (naughty_fsmonitor)
+ die(_("zOMG we got a core.fsmonitor setting from a possibly bad path '%s' ..."),
+ git_config_get_fsmonitor);
if (core_fsmonitor && !*core_fsmonitor)
core_fsmonitor = NULL;
Where the "..." would be some version of your advice in 2/2 about
safe.directory, which would also cause "naughty_fsmonitor" to remain
NULL in this case.
Doesn't that give us all of the relevant security mitigation without
potentially breaking users in the wild who are relying on git to work in
the core.sharedRepository case?
To Edward's upthread "I will wager a handsome sum[...]" comment: An
ex-employer has exactly that setup, with AFAIK on the orders of thousand
of users (a shared directory deployment system across a fleet of
"staging" servers).
I'd really like us to avoid unduly disrupting those kinds of setups if
we can help it.
In this case doing so seems like a rather easy addition: Let's ignore
and/or die on the combination of this sort of "unsafe" directory and
core.fsmonitor in particular. No?
That patch doesn't apply anymore (recent fsmonitor changes), but I still
think something like that would be a nice thing to have, e.g. being able
to configure in /etc/gitconfig that "this system is OK with not needing
safe.directory whitelisting, except maybe for core.fsmonitor".
Hopefully Johannes will chime in, but I think it's fair to say that
(this is just a summarizing from memory, I've surely missed some bits):
* Yes, for the *known* issues we could go for a much more narrow
solution (something like the above).
* There are other bits of config that also point to executable things,
e.g. core.editor, aliases etc, but nothing has been found yet that
provides the "at a distance" effect that the core.fsmonitor vector
does.
I.e. a user is unlikely to go to /tmp/some-crap/here and run "git
commit", but they (or their shell prompt) might run "git status", and
if you have a /tmp/.git ...
* Third-party software is also a wildcard here, i.e. we can know that
for git itself we have such-and-such interaction with core.editor or
whatever, but is the same true of the plethora of third-party git
integrations?
* Johannes et al were concerned that with the cat being out of the bag
(as it were) other similar issues would be poked at/discovered.
* Therefore a more thorough initial solution was preferred.
And maybe?:
* Now that this is out, the people involved would be OK with discussing
something more surgical, in particular to accommodate the
core.sharedRepository case (after the current rc phase, preferably).
In any case, the core.sharedRepository case isn't personally my itch to
scratch anymore, so I'm not going to pursue this, but perhaps someone
else is interested...
> [1] the commit message for the change mentions "shared directories",
> but in some environments, it is quite common for each user to have
> read access to other uers's home directories. I'm mostly thinking about
> high performance computing clusters, which is the kind of systems I have
> experience with. This makes it really easy for local
> "git experts" to 'cd' into a colleague's repo and help them when they
> are facing a Git problem. The fact that it won't be possible to do that
> without manually invoking 'git config --add safe.directory $PWD' beforehand
> is a little sad... What were the arguments for specifically disabling
> 'git -c safe.directory' for this fix ?
I wasn't aware/hadn't noticed that aspect of it, but I don't think
that's "by design" or whatever, but just an "and by the way..." in
8959555cee7 (setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02).
I.e. for no particularly good reason other than historical codebase
growth we'll parse the command-line in git.c after we run the setup.c
bits, which I believe is the reason this isn't supported on the
command-line.
The same is true for the trace2.* config, which likewise is for no
particular reason other than nobody felt like refactoring that tricky
core bit of code to make it work.
I.e. if you go back and read the discussions when the trace2.* config
was added it was essentially a "yeah, -c would be nice, but this is good
enough", and not "we'd like to forbid -c by design".
I hope all of that helps.
P.s.: For anyone wanting to hoist the "-c" handling earlier this is
probably a good start:
https://lore.kernel.org/git/220128.8635l7d7y6.gmgdl@xxxxxxxxxxxxxxxxxxx/
