Joey Hess <id@xxxxxxxxxx> writes:
> 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:
> 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@xxxxxx>
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@xxxxxxxxxxx/
