| Message ID | b841db8392ebd924d1893829a7e5e22240f1e9cf.1716028366.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various fixes for v2.45.1 and friends | expand |
On Sat, May 18, 2024 at 10:32:43AM +0000, Johannes Schindelin via GitGitGadget wrote: > To help Git LFS, and other tools behaving similarly (if there are any), > let's add a new, multi-valued `safe.hook.sha256` config setting. Like > the already-existing `safe.*` settings, it is ignored in > repository-local configs, and it is interpreted as a list of SHA-256 > checksums of hooks' contents that are safe to execute during a clone > operation. Future Git LFS versions will need to write those entries at > the same time they install the `smudge`/`clean` filters. This scheme seems more complicated for the user than the sometimes discussed ability to specify hook paths via config (not core.hooksPath, which covers _all_ hooks, but one which allows a per-hook path). 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 Whereas if the config can just point at the hook, then there is only one step: add the config for the hook (presumably pointing to a system version that would have been copied into $GIT_DIR/hooks previously). Likewise for updates of the hooks, where the sha256 scheme requires computing and adding a new hash. But when the config just points to the path, there is no additional step for updating. In either scheme, programs like git-lfs would have to adjust to the new world view. The main advantage of the sha256 scheme, it seems to me, is that the baked-in sha256 values let existing versions of git-lfs work. But we could also support that internally, without exposing safe.hook.sha256 to the world (and thus creating an ecosystem where we have to support it forever). Implied here is that I also think config-based hooks have a lot of _other_ advantages, and so would be worth pursuing anyway, and this extra safety would come along for free. I won't enumerate those advantages here, but we that can be a separate discussion if need be. And of course that feature doesn't yet exist, and is a much larger one. But besides un-breaking current LFS, I'm not sure that we need to rush out a more generic version of the feature. -Peff
Jeff King <peff@peff.net> 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?
Hi Jeff, On Sat, 18 May 2024, Jeff King wrote: > On Sat, May 18, 2024 at 10:32:43AM +0000, Johannes Schindelin via GitGitGadget wrote: > > > To help Git LFS, and other tools behaving similarly (if there are any), > > let's add a new, multi-valued `safe.hook.sha256` config setting. Like > > the already-existing `safe.*` settings, it is ignored in > > repository-local configs, and it is interpreted as a list of SHA-256 > > checksums of hooks' contents that are safe to execute during a clone > > operation. Future Git LFS versions will need to write those entries at > > the same time they install the `smudge`/`clean` filters. > > This scheme seems more complicated for the user than the sometimes > discussed ability to specify hook paths via config (not core.hooksPath, > which covers _all_ hooks, but one which allows a per-hook path). Right, it is more complicated. But then, we are talking about `git clone` protections, as Junio points out, i.e. preventing hooks from running that the user did not install. Git LFS' `post-checkout` hook is an example: The user never explicitly installed this hook, and it was not there before the checkout phase of the clone started, yet it is there after it finished. That's the same pattern as many attack vectors used that we saw in the path for critical CVEs. > 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 Well, there is tooling support: With the proposed patches (patch 5, to be precise), Git will complain about hooks that are installed _during_ a clone, and then provide the following advice: If this is intentional and the hook is safe to run, please run the following command and try again: git config --global --add safe.hook.sha256 <hash> While this won't help with the just-completed clone operation, it assists preventing the same issue in future clones. > Whereas if the config can just point at the hook, then there is only one > step: add the config for the hook (presumably pointing to a system > version that would have been copied into $GIT_DIR/hooks previously). > > Likewise for updates of the hooks, where the sha256 scheme requires > computing and adding a new hash. But when the config just points to the > path, there is no additional step for updating. > > In either scheme, programs like git-lfs would have to adjust to the new > world view. The main advantage of the sha256 scheme, it seems to me, is > that the baked-in sha256 values let existing versions of git-lfs work. > But we could also support that internally, without exposing > safe.hook.sha256 to the world (and thus creating an ecosystem where we > have to support it forever). > > Implied here is that I also think config-based hooks have a lot of > _other_ advantages, and so would be worth pursuing anyway, and this > extra safety would come along for free. I won't enumerate those > advantages here, but we that can be a separate discussion if need be. 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? > And of course that feature doesn't yet exist, and is a much larger one. > But besides un-breaking current LFS, I'm not sure that we need to rush > out a more generic version of the feature. Exactly. We need to unbreak Git LFS-enabled clones and release v2.45.2 before I even have the head space to think more about config-based hooks. Ciao, Johannes
On Sat, May 18, 2024 at 11:54:41AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> 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
Hi Junio, On Sat, 18 May 2024, Junio C Hamano wrote: > Jeff King <peff@peff.net> 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
On Sat, May 18, 2024 at 09:32:07PM +0200, Johannes Schindelin wrote: > > - compute the sha256 of the hook (for which we provide no tooling > > support, though hopefully it is obvious how to use other tools) > [...] > > Well, there is tooling support: With the proposed patches (patch 5, to be > precise), Git will complain about hooks that are installed _during_ a > clone, and then provide the following advice: > > If this is intentional and the hook is safe to run, > please run the following command and try again: > > git config --global --add safe.hook.sha256 <hash> > > While this won't help with the just-completed clone operation, it assists > preventing the same issue in future clones. What I meant by tooling support was: how do you find out the sha256 hash of the hook you're wanting to bless? I'd imagine you'd reach for the stand-alone "sha256" tool. But there is no Git tool to compute the hash (you can't use any of the usual tools like "hash-object" because it is not a pure hash of the content). Should we provide one? Or at least tell the user which third-party command is likely to be used? > > Implied here is that I also think config-based hooks have a lot of > > _other_ advantages, and so would be worth pursuing anyway, and this > > extra safety would come along for free. I won't enumerate those > > advantages here, but we that can be a separate discussion if need be. > > 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? If I can attack .git/config, then I can set it to match the attack hook (not to mention the zillion other config options which execute arbitrary code). If we really want to harden .git against attacks which can overwrite files in it, then I think the long-term path may be something like: - add support for specifying hooks via config. Leave .git/hooks for compatibility. - introduce a config option to disable .git/hooks support. Respect it only outside of .git/config. Default to false to start for backwards compatibility. Eventually flip it to true by default. And then perhaps something similar for in-repo config (add an option to disable in-repo config except for repos marked as safe). > > And of course that feature doesn't yet exist, and is a much larger one. > > But besides un-breaking current LFS, I'm not sure that we need to rush > > out a more generic version of the feature. > > Exactly. We need to unbreak Git LFS-enabled clones and release v2.45.2 > before I even have the head space to think more about config-based hooks. To be clear, I'm not proposing doing nothing. I'm proposing un-breaking LFS either by rolling back the defense-in-depth or adding hard-coded hashes, neither of which introduces a user-visible feature that must be supported. And then proceed with new features in the regular cycle. The hard-coded hashes are obviously a ticking time bomb until lfs updates again (and they don't help any as-yet-unknown program which does the same thing). So I'd suggest just rolling back the feature entirely in the meantime. -Peff
Hi Jeff, On Sat, 18 May 2024, Jeff King wrote: > On Sat, May 18, 2024 at 09:32:07PM +0200, Johannes Schindelin wrote: > > > > Implied here is that I also think config-based hooks have a lot of > > > _other_ advantages, and so would be worth pursuing anyway, and this > > > extra safety would come along for free. I won't enumerate those > > > advantages here, but we that can be a separate discussion if need > > > be. > > > > 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. > If I can attack .git/config, then I can set it to match the attack hook > (not to mention the zillion other config options which execute arbitrary > code). > > If we really want to harden .git against attacks which can overwrite > files in it, then I think the long-term path may be something like: > > - add support for specifying hooks via config. Leave .git/hooks for > compatibility. > > - introduce a config option to disable .git/hooks support. Respect it > only outside of .git/config. Default to false to start for backwards > compatibility. Eventually flip it to true by default. > > And then perhaps something similar for in-repo config (add an option to > disable in-repo config except for repos marked as safe). > > > > And of course that feature doesn't yet exist, and is a much larger one. > > > But besides un-breaking current LFS, I'm not sure that we need to rush > > > out a more generic version of the feature. > > > > Exactly. We need to unbreak Git LFS-enabled clones and release v2.45.2 > > before I even have the head space to think more about config-based hooks. > > To be clear, I'm not proposing doing nothing. I'm proposing un-breaking > LFS either by rolling back the defense-in-depth or adding hard-coded > hashes, neither of which introduces a user-visible feature that must be > supported. And then proceed with new features in the regular cycle. > > The hard-coded hashes are obviously a ticking time bomb until lfs > updates again (and they don't help any as-yet-unknown program which does > the same thing). So I'd suggest just rolling back the feature entirely > in the meantime. 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. Even worse: If we removed these protections without any replacement, now we basically told hackers where to look for nice, exploitable bugs, publicly. 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. Ciao, Johannes 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.
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@glandium.org/ though that wasn't a hook problem, but rather leaving the repo in an uninitialized state for longer. -Peff
Jeff King <peff@peff.net> writes: > 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. I find it a lot less compelling, too. It unfortunately involves about the same amount of conflict management to do the (partial) revert for all these maintenance tracks as it would then later take a "fix in the next cycle" for all these tracks, which made me feel somewhat hesitant. But considering that we are not talking about lifting vulnerability fix, it may make sense to do the (partial) revert all the way down to 2.39 track but do the "fix in the next cycle" only for 2.45 and later (or even in 2.46 only, without even aiming to touch 2.45 track). Thanks for a dose of sanity.
Hi, On Sat, 18 May 2024, Junio C Hamano wrote: > But considering that we are not talking about lifting vulnerability fix, > it may make sense to do the (partial) revert all the way down to 2.39 > track but do the "fix in the next cycle" only for 2.45 and later (or > even in 2.46 only, without even aiming to touch 2.45 track). To ensure that I don't misunderstand you: You are talking about reverting 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28), right? It is _technically_ true that this is not a vulnerability fix. But only _technically_. Practically, it is preventing vulnerabilities from reaching the critical level. Let's take the most recently-addressed critical vulnerability, CVE-2024-32002. I carefully crafted above-mentioned commit such that it would have prevent the Remote Code Execution attack vector noted in that CVE. To put this into perspective: If this protection had been put in place before v2.39.4, the CVSS score of CVS-2024-32002 would not have been 9.1 (Critical), but instead 2.2 (Low). In other words, even if a vulnerability was found, Git's users would have been safer with this defense-in-depth in place. The same `post-checkout` hook attack vector likewise raised the severity of CVE-2021-21300, CVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349. It also raised the severity of the vulnerability fixed in v1.8.5.6 (for which we did not obtain a CVE). Based on past experience, we must expect the semi-steady trickle of Git vulnerabilities to continue, and having this defense-in-depth in place will invaluable in helping to keep the severity of those future vulnerabilities low. Therefore reverting that commit would put Git's users at risk. In combination with the fact that we have a path forward via the patches that are under discussion in this here mail thread, a path forward that avoids that risk and incurs an acceptable cost [*1*], the plan to revert 8db1e8743c0 instead should be questioned. Ciao, Johannes P.S.: A concern was raised about `safe.hook.sha256` not having tooling to generate those SHA-256 checksums, and putting a burden on 3rd-party tool developers. However, tooling to generate SHA-256 checksums of files is ubiquitous, there is `sha256sum` and `openssl dgst -sha256`, just to name two tools that are widely available. And it's not as if _Git users_ would have to generate those checksums. It would be a one-time cost for the developers whose tools install those hooks during a clone (Git offers no option to install hooks during cloning other than templates, which is not broken in v2.39.4, ..., v2.45.1). Which brings me to that mysterious mention of tools other than Git LFS being potentially affected. It is quite dubitable that tools other than Git LFS use this method of changing the Git repository configuration in such a major way as to install hooks _while running a `smudge` filter_. It must be put into doubt, too, that this method of abusing the `smudge` filter is the best design to address Git LFS' needs. A serious downside, for example, is that this Git LFS strategy is in conflict with user-provided `post-checkout` hooks that are copied via templates. A better design that comes immediately to mind would be to add a new, generic batched-smudge filter that Git LFS could use, and that would be configured in the system or user-wide config just like Git LFS' `smudge`/`clean` filters, without the need to play hacky games during a clone operation that are very similar to malicious attacks' strategies to abuse Git's hook feature to allow Remote Code Execution.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > To put this into perspective: If this protection had been put in place > before v2.39.4, the CVSS score of CVS-2024-32002 would not have been > 9.1 (Critical), but instead 2.2 (Low). But we wouldn't have a working git-lfs then, so that comparison is not quite fair. As brian already said, you can reduce the score by making Git do nothing, which is _also_ an absurd position to take "security" (in air quotes) over everything else like usability and functionality. And this time, the layered security went a bit too aggressive. Also as Peff said and I agreed to, we are not talking about refusing to do anything on top. It was just that the "never run any approved hook during clone" turned out to be not-quite-fully thought out and it should be reworked in the open, and reverting that wholesale would hopefully give us a cleaner ground to design it. The end-result of such a reworking in the open may turn out to be the same (or similar) "register the blob object name of the contents to appear in approved hook scripts", or it may look completely different. But the road to get there, and the state of the system while we get there, would be different. I would probably see if I can take brian's revert directly; if it applies to the oldest maint-2.39 track, it would be the ideal, but we'd still need to prepare a similar 7-track cascade like we did for the js/fix-clone-w-hooks-2.XX topics. If it is only for the master, it needs to be munged to apply to maint-2.39 first. Thanks.
Hi Junio, On Mon, 20 May 2024, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > To put this into perspective: If this protection had been put in place > > before v2.39.4, the CVSS score of CVS-2024-32002 would not have been > > 9.1 (Critical), but instead 2.2 (Low). > > But we wouldn't have a working git-lfs then, so that comparison is > not quite fair. I obviously did not mean to break Git LFS. And obviously I did not mean to insist on the current clone protection if we could not have fixed Git LFS. But we have patches in hand for that. > As brian already said, you can reduce the score by making Git do > nothing, which is _also_ an absurd position to take "security" (in air > quotes) over everything else like usability and functionality. And this > time, the layered security went a bit too aggressive. Right. And I never said that we should do something as absurd, so I fail to see your point. > Also as Peff said and I agreed to, we are not talking about refusing > to do anything on top. It was just that the "never run any approved > hook during clone" turned out to be not-quite-fully thought out and > it should be reworked in the open, and reverting that wholesale > would hopefully give us a cleaner ground to design it. > > The end-result of such a reworking in the open may turn out to be > the same (or similar) "register the blob object name of the contents > to appear in approved hook scripts", or it may look completely > different. But the road to get there, and the state of the system > while we get there, would be different. I see there is no convincing you that the difference to our regular "revert-then-redesign-in-the-open" process is that we're talking about something security-relevant here, and that the revert should hence not be done lightly. I've made my position clear, so have you. Since you have the last say, it's what you say that goes. Let me quickly iterate on this here patch series (as well as the `tentative/maint-*` branches) so that we can accelerate toward a fixed version again; Git LFS has been broken for long enough, I'd think. Ciao, Johannes > I would probably see if I can take brian's revert directly; if it > applies to the oldest maint-2.39 track, it would be the ideal, but > we'd still need to prepare a similar 7-track cascade like we did for > the js/fix-clone-w-hooks-2.XX topics. If it is only for the master, > it needs to be munged to apply to maint-2.39 first. > > Thanks. >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> As brian already said, you can reduce the score by making Git do >> nothing, which is _also_ an absurd position to take "security" (in air >> quotes) over everything else like usability and functionality. And this >> time, the layered security went a bit too aggressive. > > Right. And I never said that we should do something as absurd, so I fail > to see your point. It went a bit too aggressive, closer to an absurd version of Git that does nothing, for users of git-lfs and the hooksdir config. Luckily these two were reported/found soon enough but we do not know what other fallouts remain. > Let me quickly iterate on this here patch series (as well as the > `tentative/maint-*` branches) so that we can accelerate toward a fixed > version again; Git LFS has been broken for long enough, I'd think. It would be nice to go back to the pre-2.39.4 state so that we can redo it from the clean slate soon. Thanks.
Hi, On Mon, 20 May 2024, Johannes Schindelin wrote: > Let me quickly iterate on this here patch series (as well as the > `tentative/maint-*` branches) so that we can accelerate toward a fixed > version again v3 is on the list. The `tentative/maint-*` branches have been pushed to https://github.com/dscho/git, with these commit OIDs: b9a96c4e5dc4e04258214ab772972a0e1eefd3c5 refs/heads/tentative/maint-2.39 4bf5d57da62f91db9b74d490d5dae69e65cbdc73 refs/heads/tentative/maint-2.40 5215e4e36879d1ee0ad5da7790f4598c3314ed45 refs/heads/tentative/maint-2.41 33efa2ad1a6c14fc5d8bc5cdf38ba13b25926b42 refs/heads/tentative/maint-2.42 0aeca2f80b17fcfdd9186c585ce84004ed43f46a refs/heads/tentative/maint-2.43 9953011fcdd895fd3ff4e2f2e5ff266eaf8b0b49 refs/heads/tentative/maint-2.44 aeddcb02756259e4b221f37a60e4ee1ece3889f1 refs/heads/tentative/maint-2.45 Ciao, Johannes
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index bde7f31459b..69ee845be89 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -59,3 +59,9 @@ which id the original user has. If that is not what you would prefer and want git to only trust repositories that are owned by root instead, then you can remove the `SUDO_UID` variable from root's environment before invoking git. + +safe.hook.sha256:: + The value is the SHA-256 of hooks that are considered to be safe + to run during a clone operation. ++ +Multiple values can be added via `git config --global --add`. diff --git a/hook.c b/hook.c index fc974cee1d8..a2479738451 100644 --- a/hook.c +++ b/hook.c @@ -2,6 +2,7 @@ #include "hook.h" #include "run-command.h" #include "config.h" +#include "strmap.h" static int identical_to_template_hook(const char *name, const char *path) { @@ -29,11 +30,65 @@ static int identical_to_template_hook(const char *name, const char *path) return ret; } +static struct strset safe_hook_sha256s = STRSET_INIT; +static int safe_hook_sha256s_initialized; + +static int get_sha256_of_file_contents(const char *path, char *sha256) +{ + struct strbuf sb = STRBUF_INIT; + int fd; + ssize_t res; + + git_hash_ctx ctx; + const struct git_hash_algo *algo = &hash_algos[GIT_HASH_SHA256]; + unsigned char hash[GIT_MAX_RAWSZ]; + + if ((fd = open(path, O_RDONLY)) < 0) + return -1; + res = strbuf_read(&sb, fd, 400); + close(fd); + if (res < 0) + return -1; + + algo->init_fn(&ctx); + algo->update_fn(&ctx, sb.buf, sb.len); + strbuf_release(&sb); + algo->final_fn(hash, &ctx); + + hash_to_hex_algop_r(sha256, hash, algo); + + return 0; +} + +static int safe_hook_cb(const char *key, const char *value, void *d) +{ + struct strset *set = d; + + if (value && !strcmp(key, "safe.hook.sha256")) + strset_add(set, value); + + return 0; +} + +static int is_hook_safe_during_clone(const char *name, const char *path, char *sha256) +{ + if (get_sha256_of_file_contents(path, sha256) < 0) + return 0; + + if (!safe_hook_sha256s_initialized) { + safe_hook_sha256s_initialized = 1; + git_protected_config(safe_hook_cb, &safe_hook_sha256s); + } + + return strset_contains(&safe_hook_sha256s, sha256); +} + const char *find_hook(const char *name) { static struct strbuf path = STRBUF_INIT; int found_hook; + char sha256[GIT_SHA256_HEXSZ + 1] = { '\0' }; strbuf_reset(&path); strbuf_git_path(&path, "hooks/%s", name); @@ -65,13 +120,14 @@ const char *find_hook(const char *name) return NULL; } if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) && - !identical_to_template_hook(name, path.buf)) + !identical_to_template_hook(name, path.buf) && + !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 should actually " - "be run, please\nrun the command again with " - "`GIT_CLONE_PROTECTION_ACTIVE=false`"), - name, path.buf); + "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); return path.buf; } diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 2ef3579fa7c..0f74c9154d0 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,4 +177,19 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' +test_expect_success '`safe.hook.sha256` and clone protections' ' + git init safe-hook && + write_script safe-hook/.git/hooks/pre-push <<-\EOF && + echo "called hook" >safe-hook.log + EOF + + test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ + git -C safe-hook hook run pre-push 2>err && + cmd="$(grep "git config --global --add safe.hook.sha256 [0-9a-f]" err)" && + eval "$cmd" && + GIT_CLONE_PROTECTION_ACTIVE=true \ + git -C safe-hook hook run pre-push && + test "called hook" = "$(cat safe-hook/safe-hook.log)" +' + test_done