| Message ID | 20240514181641.150112-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Revert defense-in-depth patches breaking Git LFS | expand |
brian, On Tue, 14 May 2024, brian m. carlson wrote: > The recent defense-in-depth patches to restrict hooks while cloning > broke Git LFS because it installs necessary hooks when it is invoked by > Git's smudge filter. This means that currently, anyone with Git LFS > installed who attempts to clone a repository with at least one LFS file > will see a message like the following (fictitious example): > > ---- > $ git clone https://github.com/octocat/xyzzy.git > Cloning into 'pull-bug'... > remote: Enumerating objects: 1275, done. > remote: Counting objects: 100% (343/343), done. > remote: Compressing objects: 100% (136/136), done. > remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932 > Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done. > Resolving deltas: 100% (226/226), done. > Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done. > fatal: active `post-checkout` hook found during `git clone`: > /home/octocat/xyzzy/.git/hooks/post-checkout > For security reasons, this is disallowed by default. > If this is intentional and the hook should actually be run, please > run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false` > warning: Clone succeeded, but checkout failed. > You can inspect what was checked out with 'git status' > and retry with 'git restore --source=HEAD :/' > ---- > > 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. > 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. 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'. 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. 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 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). Ciao, Johannes
On 2024-05-14 at 19:07:28, Johannes Schindelin wrote: > 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. > > 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. > 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. > 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. > 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 wrote: > 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. More than one major project; they also broke git-annex in the case where a git-annex repository, which contains symlinks into .git/annex/objects/, is pushed to a bare repository with receive.fsckObjects set. (Gitlab is currently affected[1].) BTW, do I understand correctly that the defence in depth patch set was developed under embargo and has never been publically reviewed? Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed that its PATH_MAX check is also dodgy due to that having values ranging from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git repositories containing legitimate, working symlinks can now fail to be pushed depending on what OS happens to host a reciving bare repository. + if (is_ntfs_dotgit(p)) This means that symlinks to eg "git~1" are also warned about, which seems strange behavior on eg Linux. + backslash = memchr(p, '\\', slash - p); This and other backslash handling code for some reason is also run on linux, so a symlink to eg "ummmm\\git~1" is also warned about. + if (!buf || size > PATH_MAX) { I suspect, but have not confirmed, that this is allows a symlink target 1 byte longer than the OS supports, because PATH_MAX includes a trailing NUL. All in all, this seems to need more review and a more careful consideration of breakage now that the security holes are not under embargo.
brian m. carlson wrote: > > 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. I was surprised today to find that git-annex also triggers the hook problem. In particular, a git clone that uses git-remote-annex can cause several hooks to get created. I think the hook check is already scheduled for reversion, but in case not, here's another data point against hard-coding known-good hooks as a solution.
Hi Joey, On Wed, 22 May 2024, Joey Hess wrote: > brian m. carlson wrote: > > 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. > > More than one major project; they also broke git-annex in the case where > a git-annex repository, which contains symlinks into > .git/annex/objects/, is pushed to a bare repository with > receive.fsckObjects set. (Gitlab is currently affected[1].) This added fsck functionality was specifically marked as `WARN` instead of `ERROR`, though. So it should not have failed. > BTW, do I understand correctly that the defence in depth patch set was > developed under embargo and has never been publically reviewed? > > Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed > that its PATH_MAX check is also dodgy due to that having values ranging > from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git > repositories containing legitimate, working symlinks can now fail to be > pushed depending on what OS happens to host a reciving bare repository. Likewise, this fsck functionality was specifically marked as `WARN` instead of `ERROR`, to prevent exactly the issue you are seeing. Are you saying that Gitlab is upgrading fsck warnings to errors? If so, I fear we need to ask Gitlab to stop doing that. > + if (is_ntfs_dotgit(p)) > > This means that symlinks to eg "git~1" are also warned about, > which seems strange behavior on eg Linux. Only until you realize that there are many cross-platform projects, and that Windows Subsystem for Linux is a thing. > + backslash = memchr(p, '\\', slash - p); > > This and other backslash handling code for some reason is also run on > linux, so a symlink to eg "ummmm\\git~1" is also warned about. Right. As far as I can tell, there are very few Linux-only projects left, so this is in line with many (most?) projects being cross-platform. > + if (!buf || size > PATH_MAX) { > > I suspect, but have not confirmed, that this is allows a symlink > target 1 byte longer than the OS supports, because PATH_MAX includes > a trailing NUL. True. That condition is basically imitating the `size > ATTR_MAX_FILE_SIZE` one a couple of lines earlier, but it should be `>=` here because `PATH_MAX` is supposed to accommodate the trailing NUL. Ciao, Johannes
Johannes Schindelin wrote: > > More than one major project; they also broke git-annex in the case where > > a git-annex repository, which contains symlinks into > > .git/annex/objects/, is pushed to a bare repository with > > receive.fsckObjects set. (Gitlab is currently affected[1].) > > This added fsck functionality was specifically marked as `WARN` instead of > `ERROR`, though. So it should not have failed. A git push into a bare repository with receive.fsckobjects = true fails: joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck receive.fsckobjects=true joey@darkstar:~/tmp/bench/bar.git>cd .. joey@darkstar:~/tmp/bench>cd foo joey@darkstar:~/tmp/bench/foo>git push ../bar.git master Enumerating objects: 4, done. Counting objects: 100% (4/4), done. Delta compression using up to 12 threads Compressing objects: 100% (2/2), done. Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done. Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir remote: fatal: fsck error in pack objects error: remote unpack failed: unpack-objects abnormal exit To ../bar.git ! [remote rejected] master -> master (unpacker error) error: failed to push some refs to '../bar.git' So I guess that the WARN doesn't work like you expected it to in this case of receive.fsckobjects checking. > > This means that symlinks to eg "git~1" are also warned about, > > which seems strange behavior on eg Linux. > > Only until you realize that there are many cross-platform projects, and > that Windows Subsystem for Linux is a thing. I realize that of course, but I also reserve the right to make git repos that contain files named eg "CON" if I want to. Git should not demand filename interoperability with arbitrary OSes. > > + backslash = memchr(p, '\\', slash - p); > > > > This and other backslash handling code for some reason is also run on > > linux, so a symlink to eg "ummmm\\git~1" is also warned about. > > Right. As far as I can tell, there are very few Linux-only projects left, > so this is in line with many (most?) projects being cross-platform. We may have very different lived experiences then.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2024-05-28 at 02:13:55, Joey Hess wrote: >> Johannes Schindelin wrote: >> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done. >> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) >> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir >> remote: fatal: fsck error in pack objects >> error: remote unpack failed: unpack-objects abnormal exit >> To ../bar.git >> ! [remote rejected] master -> master (unpacker error) >> error: failed to push some refs to '../bar.git' >> >> So I guess that the WARN doesn't work like you expected it to in this case of >> receive.fsckobjects checking. > > Then my guess is that this will affect most forges. FWIW, it is detected as "error" according to the above. In any case, a33fea08 (fsck: warn about symlink pointing inside a gitdir, 2024-04-10) adds two ERRORs, in addition to two WARNs: + FUNC(SYMLINK_TARGET_MISSING, ERROR) \ + FUNC(SYMLINK_TARGET_BLOB, ERROR) \ + FUNC(SYMLINK_TARGET_LENGTH, WARN) \ + FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \ so "they are only warnings and won't break" is not quite what I see in the change, but what is causing the above error does look like the one that is marked as WARN in the patch. Thanks.
On Mon, May 27, 2024 at 10:13:55PM -0400, Joey Hess wrote: > Johannes Schindelin wrote: > > > More than one major project; they also broke git-annex in the case where > > > a git-annex repository, which contains symlinks into > > > .git/annex/objects/, is pushed to a bare repository with > > > receive.fsckObjects set. (Gitlab is currently affected[1].) > > > > This added fsck functionality was specifically marked as `WARN` instead of > > `ERROR`, though. So it should not have failed. > > A git push into a bare repository with receive.fsckobjects = true fails: > > joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck > receive.fsckobjects=true > joey@darkstar:~/tmp/bench/bar.git>cd .. > joey@darkstar:~/tmp/bench>cd foo > joey@darkstar:~/tmp/bench/foo>git push ../bar.git master > Enumerating objects: 4, done. > Counting objects: 100% (4/4), done. > Delta compression using up to 12 threads > Compressing objects: 100% (2/2), done. > Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done. > Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) > remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir > remote: fatal: fsck error in pack objects > error: remote unpack failed: unpack-objects abnormal exit > To ../bar.git > ! [remote rejected] master -> master (unpacker error) > error: failed to push some refs to '../bar.git' > > So I guess that the WARN doesn't work like you expected it to in this case of > receive.fsckobjects checking. This is a long-standing weirdness with the fsck severities. The fsck_options struct used for fetches/pushes has the "strict" flag set, which upgrades warnings to errors. But if you manually configure a severity to "warn", then we respect that. For example, try: git init git commit --allow-empty -m 'message with NUL' commit=$(git cat-file commit HEAD | perl -pe 's/NUL/\0/' | git hash-object -w --stdin -t commit --literally) git update-ref HEAD $commit which is defined as WARN in fsck.h. And hence: $ git fsck; echo $? warning in commit 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body Checking object directories: 100% (256/256), done. 0 But that's upgraded to ERROR for transfers: $ git init --bare dst.git $ git -C dst.git config transfer.fsckObjects true $ git push dst.git ... remote: error: object e6db180f21250e03b633a3684f593ceb7b9cd844: nulInCommit: NUL byte in the commit object body remote: fatal: fsck error in packed object error: remote unpack failed: unpack-objects abnormal exit To dst.git ! [remote rejected] main -> main (unpacker error) error: failed to push some refs to 'dst.git' Unless we override it: $ git -C dst.git config receive.fsck.nulInCommit warn $ git push dst.git remote: warning: object 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body To dst.git * [new branch] main -> main But of course most sites just use the defaults, so all warnings are effectively errors. I think it's been this way at least since c99ba492f1 (fsck: introduce identifiers for fsck messages, 2015-06-22). We've discussed it once or twice on the list. It mostly seemed like a cosmetic issue to me, but in this case it looks like it caused functional confusion. I don't think just turning off the "strict" flag is a good idea, though. The current severities are all over the place. A missing space in an ident line is an error, but a tree with a ".git" directory is just a warning! So I think we'd first want to straighten out the severities, and then think about letting warnings bypass transfer fscks. Though it's not clear to me what hosters would want; pushing to a public site is a great time to let people know their objects are broken _before_ everyone else sees them, even if it's "just" a warning. But when you do have old history with broken objects, the control and incentives are in the wrong place; every person who wants to interact with the repo has to loosen their fsck config. So it's not clear to me how aggressive transfer-level fsck-ing should be. In the meantime, we also have an "INFO" severity which gets reported but not upgraded via strict. It sounds like that's what was intended here. It should be available in all backport versions if we want it; it was introduced in f27d05b170 (fsck: allow upgrading fsck warnings to errors, 2015-06-22). -Peff
Hi Jeff, On Wed, 29 May 2024, Jeff King wrote: > [...] But of course most sites just use the defaults, so all warnings > are effectively errors. I wish that had been pointed out on the git-security mailing list when I offered this patch up for review. > In the meantime, we also have an "INFO" severity which gets reported but > not upgraded via strict. It sounds like that's what was intended here. Precisely. So this is what the fix-up patch would look like to make the code match my intention: -- snipsnap -- Subject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE The idea of the symlink check to prevent overly-long symlink targets and targets inside the `.git/` directory was to _warn_, but not to prevent any operation. However, that's not how Git works, I was confused by the label `WARN`. What we need instead is the `IGNORE` label, which still warns (confusingly so ;-)), but does not prevent any operations from continuing. Adjust t1450 accordingly, documenting that `git fsck` unfortunately no longer warns about these issues by default. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Documentation/fsck-msgids.txt | 4 ++-- fsck.h | 4 ++-- t/t1450-fsck.sh | 13 ++++++++++++- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt index b06ec385aff..f5016ecda6a 100644 --- a/Documentation/fsck-msgids.txt +++ b/Documentation/fsck-msgids.txt @@ -158,13 +158,13 @@ (WARN) Tree contains entries pointing to a null sha1. `symlinkPointsToGitDir`:: - (WARN) Symbolic link points inside a gitdir. + (INFO) Symbolic link points inside a gitdir. `symlinkTargetBlob`:: (ERROR) A non-blob found instead of a symbolic link's target. `symlinkTargetLength`:: - (WARN) Symbolic link target longer than maximum path length. + (INFO) Symbolic link target longer than maximum path length. `symlinkTargetMissing`:: (ERROR) Unable to read symbolic link target's blob. diff --git a/fsck.h b/fsck.h index 130fa8d8f91..d41ec98064b 100644 --- a/fsck.h +++ b/fsck.h @@ -74,8 +74,6 @@ enum fsck_msg_type { FUNC(NULL_SHA1, WARN) \ FUNC(ZERO_PADDED_FILEMODE, WARN) \ FUNC(NUL_IN_COMMIT, WARN) \ - FUNC(SYMLINK_TARGET_LENGTH, WARN) \ - FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \ /* infos (reported as warnings, but ignored by default) */ \ FUNC(BAD_FILEMODE, INFO) \ FUNC(GITMODULES_PARSE, INFO) \ @@ -84,6 +82,8 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(SYMLINK_TARGET_LENGTH, INFO) \ + FUNC(SYMLINK_POINTS_TO_GIT_DIR, INFO) \ /* ignored (elevated when requested) */ \ FUNC(EXTRA_HEADER_ENTRY, IGNORE) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 5669872bc80..8339e60efb2 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -1032,7 +1032,18 @@ test_expect_success 'fsck warning on symlink target with excessive length' ' warning in blob $symlink_target: symlinkTargetLength: symlink target too long EOF git fsck --no-dangling >actual 2>&1 && - test_cmp expected actual + test_cmp expected actual && + + test_when_finished "git tag -d symlink-target-length" && + git tag symlink-target-length $tree && + test_when_finished "rm -rf throwaway.git" && + git init --bare throwaway.git && + git --git-dir=throwaway.git config receive.fsckObjects true && + git --git-dir=throwaway.git config receive.fsck.symlinkTargetLength error && + test_must_fail git push throwaway.git symlink-target-length && + git --git-dir=throwaway.git config --unset receive.fsck.symlinkTargetLength && + git push throwaway.git symlink-target-length 2>err && + grep "warning.*symlinkTargetLength" err ' test_expect_success 'fsck warning on symlink target pointing inside git dir' '
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Wed, 29 May 2024, Jeff King wrote: > >> [...] But of course most sites just use the defaults, so all warnings >> are effectively errors. > > I wish that had been pointed out on the git-security mailing list when I > offered this patch up for review. I sympathize with the sentiment, but there are things that becomes much clearer once you know what to look for by getting specific complaints, and I am sure that you would have come to "ah, there is this strict thing in addition to the msg_type" yourself, without anybody pointing it out to you, once you looked, if we had Joey's report while working on the patch. I would have noticed it with a breakage example back when the patch was first floated on the security list, but of course I didn't, because the patch was only on the security list without wider testers. The take home lesson from this episode should not be "people should speak up more in the security list". It instead is "let's try to limit the work under embargo to absolute minimum, and work in the open for anything on top". "We saw an issue that we followed a symlink when we shouldn't, which we are going to fix here, but it became high severity because of where that symlink pointed at" may be a valid sentiment to have, but we should stop at "fixing" it under embargo, and addressing the "but ... because" issue on top is better done in the open. Even if we propose "let's not allow symlink at all---that way even if we wrote through symlinks by mistake, we won't damage anything", there will be more people to correct us when we worked in the open. In any case, let's clean up the mess we created in 2.45.1 and friends quickly to prepare a solid foundation to allow us do additional work on top. The reverts are in 'next' and I plan to merge it down to 'master', which hopefully allows us to do the follow up releases soonish.
On Wed, May 29, 2024 at 02:17:41PM +0200, Johannes Schindelin wrote: > On Wed, 29 May 2024, Jeff King wrote: > > > [...] But of course most sites just use the defaults, so all warnings > > are effectively errors. > > I wish that had been pointed out on the git-security mailing list when I > offered this patch up for review. Me too. But I agree with everything Junio already responded here. > So this is what the fix-up patch would look like to make the code match my > intention: > > -- snipsnap -- > Subject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE I think you mean s/IGNORE/INFO/ here and elsewhere in the commit message? The actual code change looks correct. -Peff
The recent defense-in-depth patches to restrict hooks while cloning broke Git LFS because it installs necessary hooks when it is invoked by Git's smudge filter. This means that currently, anyone with Git LFS installed who attempts to clone a repository with at least one LFS file will see a message like the following (fictitious example): ---- $ git clone https://github.com/octocat/xyzzy.git Cloning into 'pull-bug'... remote: Enumerating objects: 1275, done. remote: Counting objects: 100% (343/343), done. remote: Compressing objects: 100% (136/136), done. remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932 Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done. Resolving deltas: 100% (226/226), done. Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done. fatal: active `post-checkout` hook found during `git clone`: /home/octocat/xyzzy/.git/hooks/post-checkout For security reasons, this is disallowed by default. If this is intentional and the hook should actually be run, please run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false` warning: Clone succeeded, but checkout failed. You can inspect what was checked out with 'git status' and retry with 'git restore --source=HEAD :/' ---- This causes most CI systems to be broken in such a case, as well as a confusing message for the user. 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. brian m. carlson (2): Revert "clone: prevent hooks from running during a clone" Revert "core.hooksPath: add some protection while cloning" builtin/clone.c | 5 ----- config.c | 13 +----------- hook.c | 32 ------------------------------ t/t1800-hook.sh | 15 -------------- t/t5601-clone.sh | 51 ------------------------------------------------ 5 files changed, 1 insertion(+), 115 deletions(-)