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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 18, 2024, 20:06 UTC
Message-ID: <86d57213-e3b2-c985-6d69-71568c66fc9c@gmx.de>
In-Reply-To: <20240518194724.GB1573807@coredump.intra.peff.net>

Hi Jeff,

On Sat, 18 May 2024, Jeff King wrote:

Show 14 quoted lines

> On Sat, May 18, 2024 at 09:32:07PM +0200, Johannes Schindelin wrote:
>
> > > Implied here is that I also think config-based hooks have a lot of
> > > _other_ advantages, and so would be worth pursuing anyway, and this
> > > extra safety would come along for free. I won't enumerate those
> > > advantages here, but we that can be a separate discussion if need
> > > be.
> >
> > 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.

Show 33 quoted lines

> If I can attack .git/config, then I can set it to match the attack hook
> (not to mention the zillion other config options which execute arbitrary
> code).
>
> If we really want to harden .git against attacks which can overwrite
> files in it, then I think the long-term path may be something like:
>
>   - add support for specifying hooks via config. Leave .git/hooks for
>     compatibility.
>
>   - introduce a config option to disable .git/hooks support. Respect it
>     only outside of .git/config. Default to false to start for backwards
>     compatibility. Eventually flip it to true by default.
>
> And then perhaps something similar for in-repo config (add an option to
> disable in-repo config except for repos marked as safe).
>
> > > And of course that feature doesn't yet exist, and is a much larger one.
> > > But besides un-breaking current LFS, I'm not sure that we need to rush
> > > out a more generic version of the feature.
> >
> > Exactly. We need to unbreak Git LFS-enabled clones and release v2.45.2
> > before I even have the head space to think more about config-based hooks.
>
> To be clear, I'm not proposing doing nothing. I'm proposing un-breaking
> LFS either by rolling back the defense-in-depth or adding hard-coded
> hashes, neither of which introduces a user-visible feature that must be
> supported. And then proceed with new features in the regular cycle.
>
> The hard-coded hashes are obviously a ticking time bomb until lfs
> updates again (and they don't help any as-yet-unknown program which does
> the same thing). So I'd suggest just rolling back the feature entirely
> in the meantime.

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.

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

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.

Ciao, Johannes

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.