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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jeff,

On Sat, 18 May 2024, Jeff King wrote:

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

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