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

though that wasn't a hook problem, but rather leaving the repo in an
uninitialized state for longer.


[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