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

From: Jeff King <peff@peff.net>
Date: May 18, 2024, 19:47 UTC
Message-ID: <20240518194724.GB1573807@coredump.intra.peff.net>
In-Reply-To: <c201bbe3-b404-feed-fcef-8333f72068dc@gmx.de>

On Sat, May 18, 2024 at 09:32:07PM +0200, Johannes Schindelin wrote:

Show 15 quoted lines

> >   - compute the sha256 of the hook (for which we provide no tooling
> >     support, though hopefully it is obvious how to use other tools)
> [...]
> 
> Well, there is tooling support: With the proposed patches (patch 5, to be
> precise), Git will complain about hooks that are installed _during_ a
> clone, and then provide the following advice:
> 
> 	If this is intentional and the hook is safe to run,
> 	please run the following command and try again:
> 
> 	  git config --global --add safe.hook.sha256 <hash>
> 
> While this won't help with the just-completed clone operation, it assists
> preventing the same issue in future clones.

What I meant by tooling support was: how do you find out the sha256 hash of the hook you're wanting to bless?

I'd imagine you'd reach for the stand-alone "sha256" tool. But there is no Git tool to compute the hash (you can't use any of the usual tools like "hash-object" because it is not a pure hash of the content). Should we provide one? Or at least tell the user which third-party command is likely to be used?

Show 9 quoted lines

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

Show 6 quoted lines

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

-Peff