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

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

 



On Sat, May 18, 2024 at 11:54:41AM -0700, 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?

Yes, sorry if I was unclear. This is _only_ about the hooks-during-clone
safety. So my "a user must do this" is really "a user who wants a hook
to be installed during a clone must do this". And plausibly speaking,
that is mostly going to be script/program writers like git-lfs.

So the extra complexity is limited to those cases.

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

I think the git_env_bool() call to check GIT_CLONE_PROTECTION_ACTIVE is
what kicks in here. During non-clone calls, that will use the default of
0.

-Peff




[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