Hi Junio
On 28/06/2024 17:48, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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
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.
