Re: What's cooking in git.git (Oct 2022, #06; Wed, 19)

From: Glen Choo <chooglen@google.com>
Date: Oct 21, 2022, 19:45 UTC
Message-ID: <kl6lczalx8ox.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <Y1ImS1Muvk1MAQeC@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 47 quoted lines

> On Thu, Oct 20, 2022 at 09:16:38PM -0400, Michael McClimon wrote:
>
>> > * mm/git-pm-try-catch-syntax-fix (2022-10-17) 1 commit
>> >  - Git.pm: add semicolon after catch statement
>> > 
>> >  Fix a longstanding syntax error in Git.pm error codepath.
>> > 
>> >  Will merge to 'next'??
>> >  source: <20221016212236.12453-2-michael@mcclimon.org>
>> 
>> I am not totally sure what these question marks mean, but I'm happy for
>> this to go to next, for what it's worth. (There was an outstanding
>> question about the general behavior of Git.pm in bare unsafe
>> repositories, but I certainly am not planning to solve it in this
>> series.) Thanks!
>
> Me too. I had hoped for a test, but because of those outstanding
> questions about the behavior, I think we are better off skipping it for
> now, and fixing what is obviously a bug.
>
> There is one devil's advocate thing that is nagging at me, though.
> Because this whole safe-directory thing is security-relevant, could we
> make things worse by "fixing" it partially? That is, consider
> this code:
>
>   git init --bare foo.git
>   sudo chown -R nobody foo.git
>   cd foo.git
>   perl -MGit -e 'Git->repository'
>
> Post 2.35.4, etc, is it a security vulnerability for that final step to
> open the repository? If so, then right now we are "not vulnerable" in
> the sense that an unrelated bug causes Git.pm to bail in the final step.
> And fixing that will mean we become vulnerable.
>
> To be clear, having an unrelated bug is not a good way to prevent
> security vulnerabilities. But it is the status quo. If the fixed version
> (after your patch to add the semicolon) is vulnerable, but the released
> one in the hands of users is not, then fixing it does make things worse
> in the short term (even though the obviously correct thing in the long
> run is to fix that bug and also correct the vulnerability the right
> way).
>
> I wasn't around when the directory-ownership stuff was discussed or
> worked on. But it might be nice for people who worked on it or thought a
> lot about it to give an opinion on whether the state after your patch is
> in fact vulnerable to a security flaw.

Off the top of my mind, the safe.* protections mitigate:

1) safe.directory: Attacker creates a difficult-to-spot repo high in the
   directory structure in a shared machine, and the victim navigates
   into an innocuous-looking directory that happens to be a subdirectory
   of that repo.
2) safe.directory: Attacker creates a repo somewhere on a shared machine
   and tricks the victim into navigating into that repo.
3) safe.bareRepository: Attacker creates a repo with a bare repo in its
   working tree and tricks the victim into cloning the repo and
   navigating into the bare repo.

In each of these cases, having the user specify GIT_DIR to mean "yes this directory is really the one I wanted" is a reasonable tradeoff IMO. Some ways of setting GIT_DIR might be ok, e.g. GIT_DIR=. everywhere is pretty flawed for 2 and 3, but it does mitigate against 1 (which is arguably the scariest).

However, if GIT_DIR is set by searching for the repo (e.g. by "git rev-parse --git-dir"), this just disables every safe.* protection. IIRC, you mentioned that this is exactly what Git.pm does, so as it is, yes this does leave us in a vulnerable state.

If we are using `git rev-parse --git-dir`, maybe a reasonable solution would be to teach `rev-parse` to respect safe.* protections? e.g. `git rev-parse --git-dir --safe` could error out just like `git` in an unsafe repository.

[1] https://lore.kernel.org/git/Y02SHlW1rNQdfCHI@coredump.intra.peff.net

>
> -Peff