Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2024, 17:32 UTC
Message-ID: <xmqqtth9xbk7.fsf@gitster.g>
In-Reply-To: <6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com>

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

Show 10 quoted lines

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

IOW, we do not bend over backwards to try to be backward compatible on this point? I can go with that, especially because it smells like (I haven't thought deeply about it yet, though) that approach can simplify the checks.

Show 27 quoted lines

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

Hmph. Are runtime-detected (not from $GIT_DIR environment and friends) gitdir and/or worktree paths always relative? If we let getcwd() involved in the process of turning them absolute, these paths may already have symbolic links "expanded", so we have no choice other than expanding symbolic links before using paths in safe.directory to compare with them.

For example, the paths from safe.directory come from the end-user, and may say things like "/home/phillip/repos/*", because phillip and everybody else on the system think /home/$USER should be where their home directories ought to be, but that was based on the niceness of sysadmins to arrange "/home/phillip" to be a symbolic link to "/home1/phillip" because the "/home" partition has already run out of space to add more users. When gitdir and/or worktree paths are computed using getcwd(), they would be paths somewhere under the "/home1/phillip/repos/" directory. Letting real_pathdup() to deal with the symbolic links may be the only usable way in such a set-up available for us.

But we will of course make tons more synonymous paths by using real_pathdup() to normalize both the safe.directory entries and the gitdir and/or worktree paths---are there security downsides in doing so?