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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2024, 05:49 UTC
Message-ID: <xmqqv835xekc.fsf@gitster.g>
In-Reply-To: <Zk3ChIHr5amGh8Mt@kitenet.net>

Joey Hess <id@joeyh.name> writes:

Show 15 quoted lines

> Junio C Hamano wrote:
>> As people have seen, the latest "security fix" release turned out to
>> be a mixed bag of good vulnerability fixes with a bit over-eager
>> "layered defence" that broke real uses cases like git-lfs.
>
> "fsck: warn about symlink pointing inside a gitdir"
> (a33fea0886cfa016d313d2bd66bdd08615bffbc9) also broke pushing git-annex
> repositories to eg Gitlab and has several other problems including dodgy
> PATH_MAX checks that cause new OS interoperability problems. (I posted
> details to an earlier thread but have now found this current one, oops.)
>
> Please also revert it, or at least the portions for 
> and symlinkPointsToGitDir and symlinkTargetLength. The
> checks for symlinkTargetBlob and symlinkTargetMissing seem worth
> keeping.

Looking at the change in question, in a33fea08 (fsck: warn about symlink pointing inside a gitdir, 2024-04-10), you said:

Show 16 quoted lines

> fsck: warn about symlink pointing inside a gitdir
> 
> In the wake of fixing a vulnerability where `git clone` mistakenly
> followed a symbolic link that it had just written while checking out
> files, writing into a gitdir, let's add some defense-in-depth by
> teaching `git fsck` to report symbolic links stored in its trees that
> point inside `.git/`.
> 
> Even though the Git project never made any promises about the exact
> shape of the `.git/` directory's contents, there are likely repositories
> out there containing symbolic links that point inside the gitdir. For
> that reason, let's only report these as warnings, not as errors.
> Security-conscious users are encouraged to configure
> `fsck.symlinkPointsToGitDir = error`.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

I can make a few observations (in addition to what Joey already observed in the code around PATH_MAX, etc. [*]):

 - Yes, if git-annex wants to keep its private data in a private
   directory .git/annex/objects it carved out for itself and want to
   point at them from the working tree with symbolic links, the
   extra check would trigger as fsck would see the symbolic link
   contents pointing into the .git/ directory, so certainly they
   would be affected.

 - 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), or will it become too brittle
   because what we care about will change over time?

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

So, I am ambivalent. I have prepared a revert queued on maint-2.39 and cascaded it up to the maintenance track, which I tentatively queued on top of 'seen', to see how much damage such a reversion involves (and it did not look too bad), but

 (1) I am not sure if this is such a huge deal that requires a
     revert;

 (2) 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.  If the structure of the code to
     perform these checks is more or less good and the actual check
     the code implements is bad, the latter might give us a better
     foundation to build on than rebuilding everything from scratch.
     On the other hand, if we are redoing things in the open, it may
     be better to forget about the code in 2.45.1/2.39.4 and to build
     from scratch for those reviewers and developers who will offer
     help.

 (3) As I look at the change by a33fea08 again, it actually adds a
     few new fsck error types with FSCK_ERROR.  Perhaps that is a
     good enough reason to do a straight revert for now?

Thanks. It is past my bedtime so I won't be pushing out the 'seen' with the revert I prepared as a practice, at least tonight.

[Reference]

 * https://lore.kernel.org/git/Zk2_mJpE7tJgqxSp@kitenet.net/