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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux