Hi Junio,
On Sat, 18 May 2024, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
