diff mbox series

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

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

Commit Message

Johannes Schindelin May 18, 2024, 10:32 a.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

As defense-in-depth measures, v2.39.4 and friends leading up to v2.45.1
introduced code that detects when hooks have been installed during a
`git clone`, which is indicative of a common attack vector with critical
severity that allows Remote Code Execution.

There are legitimate use cases for such behavior, though, for example
when those hooks stem from Git's own templates, which system
administrators are at liberty to modify to enforce, say, commit message
conventions. The git clone protections specifically add exceptions to
allow for that.

Another legitimate use case that has been identified too late to be
handled in these security bug-fix versions is Git LFS: It behaves
somewhat similar to common attack vectors by writing a few hooks while
running the `smudge` filter during a regular clone, which means that Git
has no chance to know that the hooks are benign and e.g. the
`post-checkout` hook can be safely executed as part of the clone
operation.

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/safe.txt |  6 ++++
 hook.c                        | 66 ++++++++++++++++++++++++++++++++---
 t/t1800-hook.sh               | 15 ++++++++
 3 files changed, 82 insertions(+), 5 deletions(-)

Comments

Jeff King May 18, 2024, 6:14 p.m. UTC | #1
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
Junio C Hamano May 18, 2024, 6:54 p.m. UTC | #2
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?
Johannes Schindelin May 18, 2024, 7:32 p.m. UTC | #3
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
Jeff King May 18, 2024, 7:35 p.m. UTC | #4
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
Johannes Schindelin May 18, 2024, 7:37 p.m. UTC | #5
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
Jeff King May 18, 2024, 7:47 p.m. UTC | #6
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
Johannes Schindelin May 18, 2024, 8:06 p.m. UTC | #7
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.
Jeff King May 18, 2024, 9:12 p.m. UTC | #8
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
Junio C Hamano May 19, 2024, 1:15 a.m. UTC | #9
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.
Johannes Schindelin May 20, 2024, 4:05 p.m. UTC | #10
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.
Junio C Hamano May 20, 2024, 6:18 p.m. UTC | #11
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.
Johannes Schindelin May 20, 2024, 7:38 p.m. UTC | #12
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.
>
Junio C Hamano May 20, 2024, 8:07 p.m. UTC | #13
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.
Johannes Schindelin May 20, 2024, 9:03 p.m. UTC | #14
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 mbox series

Patch

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