Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: May 9, 2022, 21:39 UTC
Message-ID: <20220509213915.GA2043@szeder.dev>
In-Reply-To: <xmqqee1il09v.fsf@gitster.g>

On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> > $PATH does), that would have been absolutely necessary to document
> > how it works, but GIT_CONFIG_* is merely an implementation detail of
> > how "git -c var=val" works and I am not sure if it is even a good
> > idea to hardcode how they happen to work like these tests.  The only
> > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> > used internally by the implementation and they should not muck with
> > it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> > index 6d764fe0cc..ae0e2e3bdb 100644
> > --- a/Documentation/config/safe.txt
> > +++ b/Documentation/config/safe.txt
> > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
> >  `safe.directory` entry with an empty value.
> >  +
> >  This config setting is only respected when specified in a system or global
> > -config, not when it is specified in a repository config or via the command
> > -line option `-c safe.directory=<path>`.
> > +config, not when it is specified in a repository config, via the command
> > +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.

Well, the proposed phrasing covers all environment variables that affect the configuration.

It doesn't feel right to explicitly list all those variables, including GIT_CONFIG_PARAMETERS, because that vairable is such an implementation detail that it isn't even mentioned elsewhere in the documentation at all. OTOH, listing only some of the ignored variables but omitting others doesn't feel quite right either, hence the general "in environment variables".

> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.

Is it safe to have such an environment variable?

So, it's quite clear why 'safe.directory' is ignored in the repository config: it can be under control of someone else, and thus a malicious repositoy could trivially safe-list itself. However, it's unclear (to me) why 'git -c safe.directory=...' is ignored: 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) only states that it's ignored, but doesn't justify why. Now, let's suppose that there was a compelling reason to do so, e.g. in some way that I don't readily see it can be spoofed and thus could be abused by an attacker to safe-list a malicious repository. If that were indeed the case, then couldn't 'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused just as easily?

Or to approach it from the opposite direction: if such a GIT_SAFE_DIRECTORIES environment variable is safe, then why should we ignore 'safe.directory' in the command line, or in the environment for that matter?