Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 28, 2024, 16:48 UTC
Message-ID: <xmqq7ce96mix.fsf@gitster.g>
In-Reply-To: <27cadffb-ca3f-487d-86b7-3508c45c446d@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 7 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 12 quoted lines

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