Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 1, 2024, 15:24 UTC
Message-ID: <6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com>
In-Reply-To: <xmqq7ce96mix.fsf@gitster.g>

Hi Junio

On 28/06/2024 17:48, Junio C Hamano wrote:

Show 20 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> 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).

I'm not sure this is a good idea because it is not clear which directory the user wanted to mark as safe when they added a relative directory to safe.directory. In the case of git-daemon one needs both the absolute path to the repository and "." to be present in safe.directory so we can ignore "." and match the absolute path.

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

We have several of normalization functions available:

  - normalize_path_copy() does a textual normalization which cleans up
    "//", "/./" and "/../".

  - absolute_pathdup() which prepends the current directory to relative
    paths attempting to use $PWD for the current directory where possible
    but does not expand symbolic links and does not clean up the path
    passed to it.

  - real_pathdup() which expands symbolic links

One way forward would be to clean up the entries in safe.directory with normalize_path_copy() and compare them to the result of normalizing $git_dir with absolute_pathdup() followed by normalize_path_copy(). That will ensure that we're always comparing the safe.directory entries against an absolute path and both sides of the comparison are textually normalized. I'm not sure whether we'd be better to use absolute_pathdup() or real_pathdup() or if we'd be safer comparing the output of both against safe.directory if they give different results. If this sounds reasonable I'll try and put a patch together later this week.

> 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 ".")?

Having more than one way to spell "*" sounds confusing. Assuming "." has only been added as a workaround for the current limitations in our safe.directory comparisons I think we'd be better to ignore it.

Best Wishes

Phillip

Show 40 quoted lines

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