Subject: Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Junio C Hamano <[email-masked]>
Date: Fri, 28 Jun 2024 09:48:54 -0700

Phillip Wood <[email-masked]> writes:

>>     We may want to discuss who protects from whom with the
>>     safe.directory mechanism and git-daemon-export-ok mechanism.  The
>>     former is "the daemon trusts that repositories won't harm the
>>     daemon user", while the latter is "the repository owner is OK for
>>     it to be published".
>
> Yes that would be helpful

OK, let's see if somebody volunteers for documentation updates in
this area.

> I think doing this would be more helpful than updating the
> documentation to recommend adding "safe.directory=.". If we do this we
> would also want to convert "//" -> "/" in the config keys as we've
> been forcing users to add paths like "/srv/git//my-repo" if the
> --base-path argument to git-daemon ended with a "/"

OK, so the idea is to normalize both safe.directory and data->path
(which might come from either worktree or gitdir) and then look for
a match.  path needs to be normalized because it can say '.' and
'/srv/git//my-repo', and values of safe.directory need to be
normalized because the users may have written '.' --- oops, relative
to what directory do we normalize safe.directory values?  That would
not work.  Let me retry.

 - Compare entries of safe.directory with data->path literally
   without normalization, as the user may have written in the
   configuration "safe.directory=.", expecting that data->path to be
   '.' (the git-daemon use case).

 - Normalize entries of safe.directory and data->path and then
   compare them, turning path="." (the git-daemon use case) into
   "/srv/git/my-repo" and a safe.directory entry "/srv/git//my-repo"
   user wrote into "/srv/git/my-repo", so that they match.  

Or we could treat "." on safe.directory as a synonym for "*"
(i.e. "anything goes"), and compare all other cases only after
normalization (which would save the cost of "literal" comparison for
safe.directory entries that are not ".")?

I may have missed some corner cases, but either of these would
probably work.

>>   * For "http-backend" invocations, we should think about potential
>>     additions that would help users, similar to what I listed above
>>     for "git daemon".
>
> That sounds sensible.

OK.

>> Having said all that, I do not think I mind GIT_SAFE_DIRECTORIES
>> that is a ":" separated list of paths that is honored just like the
>> multi-valued configuration variable safe.directory.  Once an
>> attacker can influence your environment variables, it already is
>> game over, so trusting it does not make the attack surface any
>> worse.
>
> Indeed in that case the attacker can influence the path that we read
> the protected config from by setting $HOME (and do far worse by
> setting $PATH)
> ...
> Yes having to set all the GIT_CONFIG_* variables can be rather confusing

OK.  

So an independent effort may be to introduce the said environment
variable, and have it split at ':' and feed into the same machinery
used to check paths against safe.directory configuration.  It may
need a minor refactoring to lift the current comparison logic that
assumes it is _only_ driven by the git_config() callback, and we
would probably want to define how these two sources of whitelist
entries interact (who overrides what, is "an empty element clears
all the previous entries" still true, etc.).

So, I think we have three actionable items here.

Thanks.