Re: [PATCH v2 5/8] hook(clone protections): add escape hatch

From: Jeff King <peff@peff.net>
Date: May 18, 2024, 21:12 UTC
Message-ID: <20240518211224.GA1574761@coredump.intra.peff.net>
In-Reply-To: <86d57213-e3b2-c985-6d69-71568c66fc9c@gmx.de>

On Sat, May 18, 2024 at 10:06:36PM +0200, Johannes Schindelin wrote:

Show 11 quoted lines

> > > One disadvantage of config-based hooks is that it is quite hard to
> > > verify the provenance of the settings: Was it the user who added it,
> > > was it a program the user called, or was it exploiting a vulnerability
> > > whereby the config was written inadvertently?
> >
> > But isn't that true of the safe.hook.sha256 value, too?
> 
> No, because `safe.hook.sha256` (like `safe.directory` and
> `safe.bareRepository`) is only respected in "protected" configs, i.e.
> system-wide, user-wide and command-line config. Any such settings in the
> repository-local config are ignored.

Ah, true. I think the issue still holds for all of the other config that runs arbitrary code, though, doesn't it?

Show 7 quoted lines

> Rolling back the defense-in-depth would be a mistake: We do see (seemingly
> on a yearly cadence) reports of vulnerabilities in Git that often raise to
> critical severity by exploiting the hooks feature (typically in
> conjunction with submodules). There is no reason to believe that this
> steady trickle will stop any time soon. The defense-in-depth we introduced
> would stop at least that escalation path that turns those vulnerabilities
> into critical attack vectors putting users at risk.

Most of the vulnerabilities that I recall could just as easily write over .git/config. But I didn't catalog them. Do you have specific ones in mind?

> Even worse: If we removed these protections without any replacement, now
> we basically told hackers where to look for nice, exploitable bugs,
> publicly.

I don't find this line of reasoning all that compelling. The existing vulnerabilities that led you to the defense-in-depth protection already pointed them in the right direction.

So I'm not convinced that temporarily moving back to the v2.45.0 state is all that dangerous. If it were a known vulnerability, yes, I'd worry. For defense-in-depth, less so.

Show 19 quoted lines

> For what it's worth, I was originally also in favor of the pretty surgical
> addition of the hard-coded hashes specifically to unbreak Git LFS-enabled
> clones. You must have seen my proposal that I sent to the Git security
> mailing list.
> 
> However, brian suggested that Git LFS may not be the only 3rd-party
> application that is affected by the clone protections. I have my doubts
> that other applications use a similar route, it strikes me as quite hacky
> to install a hook while running a `smudge` filter, yet I do admit that
> there is a possibility. Which is why we introduced the `safe.hooks.sha256`
> settings.
> 
> This strikes a good balance between unbreaking Git LFS and still
> benefitting from the defense-in-depth that helps fend off future critical
> vulnerabilities.
> 
> If we did not have such a balanced way to address the Git LFS breakage, I
> would totally agree with you that we would need to consider rolling back
> the defense-in-depth. Happily, we don't have to.

My main complaint is that it introduces a confusing and complicated requirement that LFS (and maybe others) have to think about in perpetuity. And we may end up with a better solution. We got bit by pushing out the v2.45.1 change without a lot of end-user testing. Now it seems like v2.45.2 is being rushed out to fix it. It would hopefully see _more_ testing, as it's being done in the open.

But it sounds like we're throwing away our usual release-engineering practices (where the usual practice for a regression is "revert it, it can happen in the next cycle") in favor of a security fix. Again, for a vulnerability fix, that makes sense. But for layered defense, I find it less compelling.

Anyway, I have said my piece and I don't think I have much to add. So either you agree or not, and if this is the direction the project wants to go, I won't object further.

Show 7 quoted lines

> P.S.: For what it's worth, the pattern we see in Git LFS is relatively
> hard to replicate. `git clone` does not offer any easy and convenient way
> to install hooks during the operation other than via templates (which,
> unlike Git LFS-enabled clones, is _not_ broken in v2.45.1). Of course,
> users could start a clone and then manually copy a `post-checkout` hook
> into `.git/hooks/` _while the clone is still running_. I kind of doubt
> that that's common practice we need to support, though.

I think we've run into similar issues with remote helpers, which run arbitrary code and could install hooks. The recent one I'm thinking of is:

  https://lore.kernel.org/git/20240503020432.2fxwuhjsvumy7i7z@glandium.org/

though that wasn't a hook problem, but rather leaving the repo in an uninitialized state for longer.

-Peff