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
