Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS
On 2024-05-14 at 19:07:28, Johannes Schindelin wrote:
Show 10 quoted lines
> 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.
Show 13 quoted lines
> > 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.
Show 8 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 5 quoted lines
> 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
brian m. carlson
