On Sat, May 18, 2024 at 10:06:36PM +0200, Johannes Schindelin wrote: > > > One disadvantage of config-based hooks is that it is quite hard to > > > verify the provenance of the settings: Was it the user who added it, > > > was it a program the user called, or was it exploiting a vulnerability > > > whereby the config was written inadvertently? > > > > But isn't that true of the safe.hook.sha256 value, too? > > No, because `safe.hook.sha256` (like `safe.directory` and > `safe.bareRepository`) is only respected in "protected" configs, i.e. > system-wide, user-wide and command-line config. Any such settings in the > repository-local config are ignored. Ah, true. I think the issue still holds for all of the other config that runs arbitrary code, though, doesn't it? > Rolling back the defense-in-depth would be a mistake: We do see (seemingly > on a yearly cadence) reports of vulnerabilities in Git that often raise to > critical severity by exploiting the hooks feature (typically in > conjunction with submodules). There is no reason to believe that this > steady trickle will stop any time soon. The defense-in-depth we introduced > would stop at least that escalation path that turns those vulnerabilities > into critical attack vectors putting users at risk. Most of the vulnerabilities that I recall could just as easily write over .git/config. But I didn't catalog them. Do you have specific ones in mind? > Even worse: If we removed these protections without any replacement, now > we basically told hackers where to look for nice, exploitable bugs, > publicly. I don't find this line of reasoning all that compelling. The existing vulnerabilities that led you to the defense-in-depth protection already pointed them in the right direction. So I'm not convinced that temporarily moving back to the v2.45.0 state is all that dangerous. If it were a known vulnerability, yes, I'd worry. For defense-in-depth, less so. > For what it's worth, I was originally also in favor of the pretty surgical > addition of the hard-coded hashes specifically to unbreak Git LFS-enabled > clones. You must have seen my proposal that I sent to the Git security > mailing list. > > However, brian suggested that Git LFS may not be the only 3rd-party > application that is affected by the clone protections. I have my doubts > that other applications use a similar route, it strikes me as quite hacky > to install a hook while running a `smudge` filter, yet I do admit that > there is a possibility. Which is why we introduced the `safe.hooks.sha256` > settings. > > This strikes a good balance between unbreaking Git LFS and still > benefitting from the defense-in-depth that helps fend off future critical > vulnerabilities. > > If we did not have such a balanced way to address the Git LFS breakage, I > would totally agree with you that we would need to consider rolling back > the defense-in-depth. Happily, we don't have to. My main complaint is that it introduces a confusing and complicated requirement that LFS (and maybe others) have to think about in perpetuity. And we may end up with a better solution. We got bit by pushing out the v2.45.1 change without a lot of end-user testing. Now it seems like v2.45.2 is being rushed out to fix it. It would hopefully see _more_ testing, as it's being done in the open. But it sounds like we're throwing away our usual release-engineering practices (where the usual practice for a regression is "revert it, it can happen in the next cycle") in favor of a security fix. Again, for a vulnerability fix, that makes sense. But for layered defense, I find it less compelling. Anyway, I have said my piece and I don't think I have much to add. So either you agree or not, and if this is the direction the project wants to go, I won't object further. > P.S.: For what it's worth, the pattern we see in Git LFS is relatively > hard to replicate. `git clone` does not offer any easy and convenient way > to install hooks during the operation other than via templates (which, > unlike Git LFS-enabled clones, is _not_ broken in v2.45.1). Of course, > users could start a clone and then manually copy a `post-checkout` hook > into `.git/hooks/` _while the clone is still running_. I kind of doubt > that that's common practice we need to support, though. I think we've run into similar issues with remote helpers, which run arbitrary code and could install hooks. The recent one I'm thinking of is: https://lore.kernel.org/git/20240503020432.2fxwuhjsvumy7i7z@xxxxxxxxxxxx/ though that wasn't a hook problem, but rather leaving the repo in an uninitialized state for longer. -Peff
