| Message ID | 20240521195659.870714-1-gitster@pobox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix various overly aggressive protections in 2.45.1 and friends | expand |
Hi Junio, On Tue, 21 May 2024, Junio C Hamano wrote: > I'll figure out a way to convey conflict resolutions as this topic > gets merged up to newer maintenance tracks on the list so that > people can assist with ensuring correctness of the result by > reviewing, and follow up. ("git show --remerge-diff" might turn out > to be such a way, but I do not know yet). I pushed 12/12 to https://github.com/dscho/git's `various-fixes-for-v2.45.1-and-friends` branch, and updated the `tentative/maint-*` branches accordingly: + b9a96c4e5dc...c6da96aa5f0 maint-2.39 -> tentative/maint-2.39 (forced update) + 4bf5d57da62...fff57b200d1 maint-2.40 -> tentative/maint-2.40 (forced update) + 5215e4e3687...616450032a0 maint-2.41 -> tentative/maint-2.41 (forced update) + 33efa2ad1a6...b1ea89bc2d6 maint-2.42 -> tentative/maint-2.42 (forced update) + 0aeca2f80b1...093c42a6c6b maint-2.43 -> tentative/maint-2.43 (forced update) + 9953011fcdd...3c7a7b923b3 maint-2.44 -> tentative/maint-2.44 (forced update) Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Tue, 21 May 2024, Junio C Hamano wrote: > >> I'll figure out a way to convey conflict resolutions as this topic >> gets merged up to newer maintenance tracks on the list so that >> people can assist with ensuring correctness of the result by >> reviewing, and follow up. ("git show --remerge-diff" might turn out >> to be such a way, but I do not know yet). > > I pushed 12/12 to https://github.com/dscho/git's > `various-fixes-for-v2.45.1-and-friends` branch, and updated the > `tentative/maint-*` branches accordingly: Thanks, but I suspect it is not productive use of your time to merge these up until we know we are happy with what we are merging up. Even though I did the 12/12 that reverts the "iffy ownership check during repository discovery", it is far from clear without discussion if that is a reasonable thing to do or use of safe.directory by narrow non-target audience (like k.org that may use gitolite and/or bare git for hosting) that lets nobody access trusted user repositories. Every time we find new issues or different solutions, somebody has to merge it up, eventually to maint-2.45, but I am afraid it may be a bit too early to commit. > + b9a96c4e5dc...c6da96aa5f0 maint-2.39 -> tentative/maint-2.39 (forced update) > + 4bf5d57da62...fff57b200d1 maint-2.40 -> tentative/maint-2.40 (forced update) > + 5215e4e3687...616450032a0 maint-2.41 -> tentative/maint-2.41 (forced update) > + 33efa2ad1a6...b1ea89bc2d6 maint-2.42 -> tentative/maint-2.42 (forced update) > + 0aeca2f80b1...093c42a6c6b maint-2.43 -> tentative/maint-2.43 (forced update) > + 9953011fcdd...3c7a7b923b3 maint-2.44 -> tentative/maint-2.44 (forced update) > > Ciao, > Johannes
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> On Tue, 21 May 2024, Junio C Hamano wrote: >> >>> I'll figure out a way to convey conflict resolutions as this topic >>> gets merged up to newer maintenance tracks on the list so that >>> people can assist with ensuring correctness of the result by >>> reviewing, and follow up. ("git show --remerge-diff" might turn out >>> to be such a way, but I do not know yet). >> >> I pushed 12/12 to https://github.com/dscho/git's >> `various-fixes-for-v2.45.1-and-friends` branch, and updated the >> `tentative/maint-*` branches accordingly: > > Thanks, but I suspect it is not productive use of your time to merge > these up until we know we are happy with what we are merging up. > > Even though I did the 12/12 that reverts the "iffy ownership check > during repository discovery", it is far from clear without > discussion if that is a reasonable thing to do or use of > safe.directory by narrow non-target audience (like k.org that may > use gitolite and/or bare git for hosting) that lets nobody access > trusted user repositories. Every time we find new issues or > different solutions, somebody has to merge it up, eventually to > maint-2.45, but I am afraid it may be a bit too early to commit. Another thing is that, now this is not an embargoed set of secret releases, I'd want to have them go through 'next' down to 'master' in the usual way, with the usual "develop in the open" fashion, before we convince ourselves that the whole cascade is ready. We may find necessary updates while these fixes are in 'next' due to $WORK folks feeding 'next' based updates to $CORP users and helping us find issues, in which case we would need to add more patches on top of the topic based on maint-2.39 (and merge it up all the way). After that is all done, we would finally become ready to write release notes for the tracks, which will be merged up the same way as the "oops, we found we need another patch while the series was in 'next'" case. Preparing tentative/maint-* set of branches your way, with release notes and GIT-VERSION-GEN updates together ready to be tagged, would not help prepare something that I can feed other developers in 'seen' and then 'next'. Thanks.
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. > Let's quickly get them in working order back first, with the vision that > we will then rebuild layered defence more carefully in the open on > top as necessary. Exellent plan.
Joey Hess <id@joeyh.name> 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@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/
Junio C Hamano wrote: > - 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.
Junio C Hamano <gitster@pobox.com> writes: > Joey Hess <id@joeyh.name> writes: > >> 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: > ... > 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. So at least for now, I've queued a full revert of the change in question in the "revert over-eager layering-on-top changes" pile, but as we haven't really seen anybody give good input to help me decide what to do with this step, the pile is still kept outside of the 'next' branch. At least it is visible on 'seen' as of tonight. Thanks.
Hi Joey, On Thu, 23 May 2024, Joey Hess wrote: > Junio C Hamano wrote: > > - 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. It risks more security problems because `.git/objects/??/*` is not re-hashed when it is being used by Git. That's a very easy way to slip in unwanted file contents. And there is a good reason _not_ to write stuff inside the `.git/` directory unless you happen to be, well, Git itself: Git makes no guarantees whatsoever that you can write into that directory whatever you want. A future Git version might even write a file `.git/annex`, breaking `git-annex`' assumptions, and that'd be totally within the guarantees Git makes. > 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/. No, the vulnerability that triggered this defense-in-depth was not CVE-2024-32021, but instead CVE-2024-32002, a critical security issue. Removing the defense-in-depth makes it more likely for otherwise relatively harmless bugs à la "oh whoops, we wrote something we shouldn't have" to escalate to full, critical Remote Code Execution vulnerabilities. Ciao, Johannes
Johannes Schindelin wrote: > And there is a good reason _not_ to write stuff inside the `.git/` > directory unless you happen to be, well, Git itself: Git makes no > guarantees whatsoever that you can write into that directory whatever you > want. A future Git version might even write a file `.git/annex`, breaking > `git-annex`' assumptions, and that'd be totally within the guarantees Git > makes. Well git-annex is hardly the only program to decide to carve out part of .git/ for its own use. For example, git-lfs uses .git/lfs/ rather similarly. Anyway, I hope I can ask nicely and not have tne git developers choose to use .git/annex/ for something. Since it would cause a large amount of pain to a large number of users, who would all have to rebase histories of (often massive) git repos to update symlinks pointing there. > No, the vulnerability that triggered this defense-in-depth was not > CVE-2024-32021, but instead CVE-2024-32002, a critical security issue. Ahh, thanks, I understand the concerns a little bit better now.
Hi Johannes On 27/05/2024 20:51, Johannes Schindelin wrote: > Hi Joey, > > On Thu, 23 May 2024, Joey Hess wrote: > >> Junio C Hamano wrote: >>> - 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. > > It risks more security problems because `.git/objects/??/*` is not > re-hashed when it is being used by Git. That's a very easy way to slip in > unwanted file contents. What checks do we have in place to prevent git checking out blobs and gitlinks to paths under .git/? I'd have thought we should be applying the same restrictions to the target of symbolic links as we do to those. > And there is a good reason _not_ to write stuff inside the `.git/` > directory unless you happen to be, well, Git itself: Git makes no > guarantees whatsoever that you can write into that directory whatever you > want. A future Git version might even write a file `.git/annex`, breaking > `git-annex`' assumptions, and that'd be totally within the guarantees Git > makes. This seems a bit harsh - many tools store their state under .git/ and I think it makes sense for them to do so as it avoids creating untracked files in the working copy. I would hope that we'd be considerate of widely used tools such as 'git annex' when adding new paths under .git/ Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > What checks do we have in place to prevent git checking out blobs and > gitlinks to paths under .git/? I'd have thought we should be applying > the same restrictions to the target of symbolic links as we do to > those. We do not even allow ".git" slip into the index (most likely from a malicious tree object), so a direct "checkout" is not much of an issue. Of course you can introduce bugs to that regular mechanism in the future but that is not the target for 2.45.1's check we are going to revert. I think what Dscho worries about in his message is that we might by mistake write via a symbolic link in the working tree. If our procedure to update a checked out blob in the working tree were open/truncate/write/close an existing file, a checkout that switches from a version with a symbolic link at path F to a version with a regular file at path F may end up overwriting the target of F. I think the idea was (Dscho can correct me if I am misleading the log messge of a33fea08 (fsck: warn about symlink pointing inside a gitdir, 2024-04-10)) that such a bug from overwriting a file in our repository if we did not allow a symbolic link F to point into our repository.
Phillip Wood <phillip.wood123@gmail.com> writes: >> And there is a good reason _not_ to write stuff inside the `.git/` >> directory unless you happen to be, well, Git itself: Git makes no >> guarantees whatsoever that you can write into that directory whatever you >> want. A future Git version might even write a file `.git/annex`, breaking >> `git-annex`' assumptions, and that'd be totally within the guarantees Git >> makes. > > This seems a bit harsh - many tools store their state under .git/ and > I think it makes sense for them to do so as it avoids creating > untracked files in the working copy. I would hope that we'd be > considerate of widely used tools such as 'git annex' when adding new > paths under .git/ Yes, a .git/annex file _can_ happen, but between civilized developer groups, such a thing would not happen without a good reason. If we have no good reason (apparently you and I did not think of any) to create such a file, "it can happen" is a poor straw-man, as we would be aiming to work well together. Yes, when we have a symbolic link as a tracked content, updating the target of the link when we need to update it is simply a bug, and it does not matter if it points at a file inside our own repository, or a file inside a different and unrelated repository that is owned by the same user, or a file in the user's home directory. Our own repository is not all that special from that perspective, and a change to penalize symbolic links that point into our repository specifically probably did make a bad choice. Thanks.
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. Let's quickly get them in working order back first, with the vision that we will then rebuild layered defence more carefully in the open on top as necessary. What we have here are the first "revert" part. These patches are designed to apply to 2.39.4; the series may have to grow as we discover more things to revert, but for now here are the patches to - revert the over-eager "refusal to work" went into 2.39.4 - adjust 2.39.4 codebase to cleanly build and test (at CI and locally) by backported fixes It would have been better if we did not have to have the latter class, but such is life. I'll figure out a way to convey conflict resolutions as this topic gets merged up to newer maintenance tracks on the list so that people can assist with ensuring correctness of the result by reviewing, and follow up. ("git show --remerge-diff" might turn out to be such a way, but I do not know yet). Jeff King (5): send-email: drop FakeTerm hack send-email: avoid creating more than one Term::ReadLine object ci: drop mention of BREW_INSTALL_PACKAGES variable ci: avoid bare "gcc" for osx-gcc job ci: stop installing "gcc-13" for osx-gcc Johannes Schindelin (6): hook: plug a new memory leak init: use the correct path of the templates directory again Revert "core.hooksPath: add some protection while cloning" tests: verify that `clone -c core.hooksPath=/dev/null` works again clone: drop the protections where hooks aren't run Revert "Add a helper function to compare file contents" Junio C Hamano (1): Revert "fetch/clone: detect dubious ownership of local repositories" .github/workflows/main.yml | 3 +- Makefile | 2 +- builtin/clone.c | 12 +------- cache.h | 14 --------- ci/install-dependencies.sh | 2 -- config.c | 13 +------- copy.c | 58 ----------------------------------- git-send-email.perl | 32 +++++++------------ hook.c | 32 ------------------- path.c | 2 -- t/helper/test-path-utils.c | 10 ------ t/t0060-path-utils.sh | 41 ------------------------- t/t0411-clone-from-partial.sh | 6 ++-- t/t1350-config-hooks-path.sh | 7 +++++ t/t1800-hook.sh | 15 --------- t/t5601-clone.sh | 51 ------------------------------ t/t9001-send-email.sh | 5 +-- 17 files changed, 28 insertions(+), 277 deletions(-)