Re: [PATCH 00/12] Fix various overly aggressive protections in 2.45.1 and friends

From: Joey Hess <id@joeyh.name>
Date: May 23, 2024, 16:31 UTC
Message-ID: <Zk9vafYPijqyWpXv@kitenet.net>
In-Reply-To: <xmqqv835xekc.fsf@gitster.g>

Junio C Hamano wrote:

Show 7 quoted lines

>  - The extra check seems to have meant to target the symbolic links
>    that point at objects, refs, config, and anything _we_ care
>    about, as opposed to random garbage (from _our_ point of view)
>    files third-parties throw into .git/ directory.  Would it have
>    made a better trade-off if we tried to make the check more
>    precise, only complaining about the things we care about (in
>    other words, what _we_ use)

I wondered about that possibility too. But it's not at all clear to me how a symlink to .git/objects/foo risks any more security problem to git than one to .git/annex/whatever, or indeed to /home/linus/.bashrc.

Git clearly has to get the security right of handling working tree files that are symlinks.

The security hole that triggered this defense in depth, CVE-2024-32021, involved an attacker with write access to .git/objects/ making a symlink in there while another repo was cloning it. So it involved symlinks inside a remote .git/objects/, which is very different than symlinks into .git/objects/.

While it's understandable that dealing with such a symlink related security hole may make one want to throw out the baby with the bathwater, this fsck check is more like you've kept the bathwater and only thrown out the baby. ;-)

>  - In any case, if it is merely warnings, not errors, these checks
>    can be configured out.  Wouldn't that be an escape-hatch enough?

The issue with that is, as we've experienced with Gitlab, git hosts that choose to set receive.fsckObjects will prevent pushes of git-annex repositories, and there's probably no way for a user to configure it out. So every major git host that does it has to be approached to configure it out, and some fraction probably won't. Which will be a major impact and ongoing concern for git-annex users[1], all for something that certainly adds no security to a bare repository on such a host.

> I am not sure which one is more practical between ripping
> everything out and demoting these new fsck error types with
> FSCK_WARN to FSCK_IGNORE.

It could indeed be beneficial to have some kind of symlink check that is at FSCK_IGNORE by default. If someone is receiving a repository from an untrusted source, and doesn't want to deal with the security risks of symlinks in the working tree, they could configure it to be an error. Such a symlink check would probably need to catch more symlinks than only the ones into .git/ though. Having this available to git users seems like it could prevent a much larger class of security holes.

As for the symlink length check, I do think it makes sense for fsck to notice symlinks that are too long to make sense for any OS and so picking some appropriate value, rather than the local PATH_MAX, could keep that one.

-- 
see shy jo

[1] I'm particularly concerned about the class of large institutional
    users who are managing more data with git-annex than the total size of
    all of Github[2]. They have a good reason to be risk averse,
    and it could be a major disruption to cross-organizational workflows
    and need updates to DOIs etc for them to switch hosting providers.
[2] https://hachyderm.io/@joeyh/112486445240754919