Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: "brian m. carlson" <>
To: Johannes Schindelin <>
Cc:, Junio C Hamano <>
Subject: Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
Date: Tue, 14 May 2024 19:41:35 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

[-- Attachment #1: Type: text/plain, Size: 5710 bytes --]

On 2024-05-14 at 19:07:28, Johannes Schindelin wrote:
> brian,
> On Tue, 14 May 2024, brian m. carlson wrote:
> > This causes most CI systems to be broken in such a case, as well as a
> > confusing message for the user.
> When using `actions/checkout` in GitHub workflows, nothing is broken
> because `actions/checkout` uses a fetch + checkout (to allow for things
> like sparse checkout), which obviously lacks the clone protections because
> it is not a clone.

I'm not saying all CI systems do this, but we probably have broken quite
a few.

> > It's not really possible to avoid the need to install the hooks at this
> > location because the post-checkout hook must be ready during the
> > checkout that's part of the clone in order to properly adjust
> > permissions on files.  Thus, we'll need to revert the changes to
> > restrict hooks while cloning, which this series does.
> Dropping protections is in general a bad idea. While previously, hackers
> wishing to exploit weaknesses in Git might have been unaware of the
> particular attack vector we want to prevent with these defense-in-depth
> measurements, we now must assume that they are fully aware. Reverting
> those protections can be seen as a very public invitation to search for
> ways to exploit the now re-introduced avenues to craft Remote Code
> Execution attacks.

If these protections hadn't broken things, I'd agree that we should keep
them.  However, they have broken things and they've introduced a
serious regression breaking a major project, and we should revert them.

I'm not opposed to us exploring other alternatives for defense in depth
measures on the list.  I definitely think such work is valuable and
important, but I want to make sure the changes we make don't regress
important functionality.

> I have pointed out several times that there are alternatives while
> discussing this under embargo, even sent them to the git-security list
> before the embargo was lifted, and have not received any reply. One
> proposal was to introduce a way to cross-check the SHA-256 of hooks that
> _were_ written during a clone operation against a list of known-good ones.
> Another alternative was to special-case Git LFS by matching the hooks'
> contents against a regular expression that matches Git LFS' current
> hooks'.

I have replied to those on the security list and to the general idea.  I
don't think we should special-case Git LFS here.  That's antithetical to
the long-standing ethos of the project.  While Git LFS is _one_ known
project to have been broken, there may be others, or people may have
forks of Git LFS under other names, and we want that tooling to be able
to take advantage of all of the same features.

I'm also opposed to any kind of pinning of the Git LFS hooks to Git in
general, whatever the approach is.  Git LFS runs against multiple
versions of Git in our CI, and if we change the hooks in a way that Git
hasn't pinned, either via SHA-256 hash or regex, then Git LFS (and its
CI) is broken until Git gets an update.  We've already had to deal with
small incompatible changes that have broken Git LFS, and I don't think
coupling the projects in this way is a good idea.

Finally, it's very easy to work around the protections by merely placing
a malicious binary called "git-lfs" earlier in the PATH, so we're not
necessarily getting a substantial amount of improved security by
requiring that binary.

> Both alternatives demonstrate that we are far from _needing_ to revert the
> changes that were designed to prevent future vulnerabilities from
> immediately becoming critical Remote Code Executions. It might be an
> easier way to address the Git LFS breakage, but "easy" does not equal
> "right".
> I did not yet get around to sending these patches to the Git mailing list
> solely because I am still busy with a lot of follow-up work of the
> embargoed release. It was an unwelcome surprise to see this here patch
> series in my inbox and still no reply to the patches I had sent to the
> git-security list for comments.

As you may know, I don't read the git or git-security lists except on
my personal computer using my personal email, and I have been at work
most of the day putting out fires related to the Git LFS breakage above.
Thus, I haven't seen them until just recently, when I did try to get a
reply out.

> I am still busy wrapping up follow-up work and won't be able to
> participate in this here mail thread meaningfully for the next hours. I do
> want to invite you to think about alternative ways to address the Git LFS
> issues, alternatives that do not re-open weaknesses we had hoped to
> address for good.

I don't want to put undue pressure on you.  Please feel free to respond
or not at your leisure.

> I do want to extend the invitation to work with me on that, for example by
> reviewing those patches I sent to the git-security mailing list (or even
> to send them to the Git mailing list for public review on my behalf, that
> would be helpful).

I think the substance of my response was the same one that I gave above.

With all due respect, I don't think your patches are the right way to
address the problem, and I fear that by bringing them to the list, I'd
be giving the list the misleading impression that I endorsed them with
my sign-off.  While I respect you as a colleague and a contributor, even
if we may disagree on this or other issues, I don't agree with the
approach in the patches, and I don't think it would be a good idea to
apply my sign-off in sending them.
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

  reply	other threads:[~2024-05-14 19:41 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-14 18:16 [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS brian m. carlson
2024-05-14 18:16 ` [PATCH 1/2] Revert "clone: prevent hooks from running during a clone" brian m. carlson
2024-05-14 18:16 ` [PATCH 2/2] Revert "core.hooksPath: add some protection while cloning" brian m. carlson
2024-05-14 19:07 ` [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS Johannes Schindelin
2024-05-14 19:41   ` brian m. carlson [this message]
2024-05-22  9:49     ` Joey Hess
2024-05-27 19:35       ` Johannes Schindelin
2024-05-28  2:13         ` Joey Hess
     [not found]           ` <>
2024-05-28 23:46             ` Junio C Hamano
2024-05-29  8:54           ` Jeff King
2024-05-29 12:17             ` Johannes Schindelin
2024-05-29 16:17               ` Junio C Hamano
2024-05-30  8:17               ` Jeff King
2024-05-24 17:37     ` Joey Hess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).