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 10:32:43AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > To help Git LFS, and other tools behaving similarly (if there are any),
> > let's add a new, multi-valued `safe.hook.sha256` config setting. Like
> > the already-existing `safe.*` settings, it is ignored in
> > repository-local configs, and it is interpreted as a list of SHA-256
> > checksums of hooks' contents that are safe to execute during a clone
> > operation. Future Git LFS versions will need to write those entries at
> > the same time they install the `smudge`/`clean` filters.
>
> This scheme seems more complicated for the user than the sometimes
> discussed ability to specify hook paths via config (not core.hooksPath,
> which covers _all_ hooks, but one which allows a per-hook path).

Right, it is more complicated.

But then, we are talking about `git clone` protections, as Junio points
out, i.e. preventing hooks from running that the user did not install.

Git LFS' `post-checkout` hook is an example: The user never explicitly
installed this hook, and it was not there before the checkout phase of the
clone started, yet it is there after it finished. That's the same pattern
as many attack vectors used that we saw in the path for critical CVEs.

> In either case, we're considering config to be a trusted source of
> truth, so I think the security properties are the same. But for the
> system here, a user updating a hook needs to do multiple steps:
>
>   - compute the sha256 of the hook (for which we provide no tooling
>     support, though hopefully it is obvious how to use other tools)
>
>   - add the config for the sha256
>
>   - install the new hook into $GIT_DIR/hooks

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.

> Whereas if the config can just point at the hook, then there is only one
> step: add the config for the hook (presumably pointing to a system
> version that would have been copied into $GIT_DIR/hooks previously).
>
> Likewise for updates of the hooks, where the sha256 scheme requires
> computing and adding a new hash. But when the config just points to the
> path, there is no additional step for updating.
>
> In either scheme, programs like git-lfs would have to adjust to the new
> world view. The main advantage of the sha256 scheme, it seems to me, is
> that the baked-in sha256 values let existing versions of git-lfs work.
> But we could also support that internally, without exposing
> safe.hook.sha256 to the world (and thus creating an ecosystem where we
> have to support it forever).
>
> 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?

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

Ciao,
Johannes