CVE-2022-24765 and core.sharedRepository (was: What's cooking in git.git (Apr 2022, #03; Tue, 12))

On Tue, Apr 12 2022, Philippe Blain wrote:

[A change of $subject seems in order]

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...

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@evledraar.gmail.com/