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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 18, 2024, 19:37 UTC
Message-ID: <6ad4cc61-f434-3ea4-8f54-728e6ad6b045@gmx.de>
In-Reply-To: <xmqqmsonne6m.fsf@gitster.g>

Hi Junio,

On Sat, 18 May 2024, Junio C Hamano wrote:

Show 37 quoted lines

> Jeff King <peff@peff.net> writes:
>
> > 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
>
> I am not sure why any of the above is needed.
>
> Hmph.
>
> I was somehow (because that is how "git config --help" explains
> "safe.hook.*") led to believe that this "safety" was only about "git
> clone would prefer not to run ANY hook before it finishes operation
> and gives back the control to the end user, but historically it ran
> any enabled hooks in the resulting repository that was freshly
> created by it---so let's at least make sure the contents of the
> hooks are known-to-be-good ones when 'git clone' runs the hooks".
> Most importantly, once "git clone" gives control back to the end
> user and the end user had a chance to inspect the resulting
> repository, the files in $GIT_DIR/hooks can be updated and the hooks
> will run without incurring any cost of checking.
>
> Isn't that what happens?
>
> Looking at the control flow, hook.c:find_hook() is the one that
> calls the function is_hook_safe_during_clone() to reject "unsafe"
> ones (and allow the white-listed ones), but I do not know offhand
> how the code limits the rejection only during clone.  So perhaps
> this set of patches need further work to restrict the checks only to
> "while we are cloning" case?

The logic in `find_hook()` reads like this:

        if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
            !is_hook_safe_during_clone(name, path.buf, sha256))
                die(_("active `%s` hook found during `git clone`:\n\t%s\n"
                      "For security reasons, this is disallowed by default.\n"
                      "If this is intentional and the hook is safe to run, "
                      "please run the following command and try again:\n\n"
                      "  git config --global --add safe.hook.sha256 %s"),
                    name, path.buf, sha256);

The `!git_hooks_path` accounts for the fact that users can choose to set the `core.hooksPath` in their global configs, in which case `git clone` _should_ expect hooks to be present that do not originate from Git's templates.

The `GIT_CLONE_PROTECTION_ACTIVE` check is the one that limits the rejection to only happen during a clone: This environment variable is set in `git clone` (carefully passing 0 as `overwrite` parameter to `setenv()` to allow users to override this protection).

The reason why it has to be done via an environment variable is that `git clone` can spawn many processes that all need to respect this protection, most notably when a recursive clone calls `git submodule--helper update`.

Ciao, Johannes