{"thread":{"id":"59544","subject":"[PATCH] clone: error specifically with --local and symlinked objects","startedAt":"2023-04-04T23:48:48Z","lastAt":"2023-04-11T18:38:34Z","messageCount":14,"participants":["Glen Choo via GitGitGadget","Junio C Hamano","Glen Choo","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474813","messageId":"pull.1488.git.git.1680652122547.gitgitgadget@gmail.com","threadId":"59544","inReplyTo":null,"subject":"[PATCH] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-04T23:48:42Z","receivedAt":"2023-04-04T23:48:48Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n6f054f9fb3 (builtin/clone.c: disallow --local clones with\nsymlinks, 2022-07-28) gives a good error message when \"git clone\n--local\" fails when the repo to clone has symlinks in\n\"$GIT_DIR/objects\". In bffc762f87 (dir-iterator: prevent top-level\nsymlinks without FOLLOW_SYMLINKS, 2023-01-24), we later extended this\nrestriction to the case where \"$GIT_DIR/objects\" is itself a symlink,\nbut we didn't update the error message then - bffc762f87's tests show\nthat we print a generic \"failed to start iterator over\" message.\n\nThis is exacerbated by the fact that Documentation/git-clone.txt\nmentions neither restriction, so users are left wondering if this is\nintentional behavior or not.\n\nFix this by adding a check to builtin/clone.c: when doing a local clone,\nperform an extra check to see if \"$GIT_DIR/objects\" is a symlink, and if\nso, assume that that was the reason for the failure and report the\nrelevant information. Ideally, dir_iterator_begin() would tell us that\nthe real failure reason is the presence of the symlink, but (as far as I\ncan tell) there isn't an appropriate errno value for that.\n\nAlso, update Documentation/git-clone.txt to reflect that this\nrestriction exists.\n\nSigned-off-by: Glen Choo \n---\n clone: error specifically with --local and symlinked objects\n \n We noticed this because repo [1] creates Git repos where\n \"$GIT_DIR/objects\" is a symlink, and users have gotten confused as to\n whether this was intended behavior or not.\n \n I'm no good with lstat() and errno, so if there's a better to do this,\n I'd appreciate the input :)\n \n [1] https://gerrit.googlesource.com/git-repo\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1488%2Fchooglen%2Fpush-nymnqqnttzxz-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1488/chooglen/push-nymnqqnttzxz-v1\nPull-Request: https://github.com/git/git/pull/1488\n\n Documentation/git-clone.txt | 5 +++++\n builtin/clone.c | 7 ++++++-\n t/t5604-clone-reference.sh | 2 +-\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex d6434d262d6..c37c4a37f74 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -58,6 +58,11 @@ never use the local optimizations). Specifying `--no-local` will\n override the default when `/path/to/repo` is given, using the regular\n Git transport instead.\n +\n+If the repository's `$GIT_DIR/objects` has symbolic links or is a\n+symbolic link, the clone will fail. This is a security measure to\n+prevent the unintentional copying of files by dereferencing the symbolic\n+links.\n++\n *NOTE*: this operation can race with concurrent modification to the\n source repository, similar to running `cp -r src dst` while modifying\n `src`.\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 462c286274c..74ec5b8b02a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -327,8 +327,13 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \n \titer = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);\n \n-\tif (!iter)\n+\tif (!iter) {\n+\t\tstruct stat st;\n+\t\tif (lstat(src->buf, &st) >= 0 && S_ISLNK(st.st_mode))\n+\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n+\t\t\t src->buf);\n \t\tdie_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n+\t}\n \n \tstrbuf_addch(src, '/');\n \tsrc_len = src->len;\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 83e3c97861d..9845fc04d59 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -358,7 +358,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '\n \ttest_must_fail git clone --local malicious clone 2>err &&\n \n \ttest_path_is_missing clone &&\n-\tgrep \"failed to start iterator over\" err\n+\tgrep \"is a symlink, refusing to clone with --local\" err\n '\n \n test_done\n\nbase-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e\n-- \ngitgitgadget\n"},{"id":"474814","messageId":"xmqq7curxk22.fsf@gitster.g","threadId":"59544","inReplyTo":"pull.1488.git.git.1680652122547.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T00:13:41Z","receivedAt":"2023-04-05T00:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> \titer = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);\n> \n> -\tif (!iter)\n> +\tif (!iter) {\n> +\t\tstruct stat st;\n> +\t\tif (lstat(src->buf, &st) >= 0 && S_ISLNK(st.st_mode))\n> +\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n> +\t\t\t src->buf);\n\nIf you want to do lstat(2) yourself, the canonical way to check its\nsuccess is to see the returned value is 0, not \"not negative\", but\nlet's first see how dir_iterator_begin() can fail. I suspect it may\nnot necessary to do another lstat(2). The function returns NULL:\n\n * if lstat() fails; errno is left from the failed lstat() in this case.\n\n * if lstat() succeeds, but the path is *not* a directory; errno is\n explicitly set to ENOTDIR. Unfortunately, if lstat(2) failed\n with ENOTDIR (e.g. dir_iterator_begin() gets called with a path\n whose leading component is not a directory), the caller will also\n see ENOTDIR, but the distinction may not matter in practice. I\n haven't thought things through.\n\nAssuming that the distinction does not matter, then,\n\n\tif (!iter) {\n\t\tif (errno == ENOTDIR)\n\t\t\tdie(_(\"'%s' is not a directory, refusing to clone with --local\"),\n\t\t\t src->buf);\n\t\telse\n\t\t\tdie_errno(_(\"failed to stat '%s'\"), src->buf);\n\t}\n\nmay be sufficient. But because this is an error codepath, it is not\nworth optimizing what happens there, and an extra lstat(2) is not\ntoo bad, if the code gains extra clarity.\n"},{"id":"474860","messageId":"kl6ly1n65l7t.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59544","inReplyTo":"xmqq7curxk22.fsf@gitster.g","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-05T16:48:22Z","receivedAt":"2023-04-05T16:48:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> If you want to do lstat(2) yourself, the canonical way to check its\n> success is to see the returned value is 0, not \"not negative\", but\n> let's first see how dir_iterator_begin() can fail.\n\nAh, thanks.\n\n> Unfortunately, if lstat(2) failed\n> with ENOTDIR (e.g. dir_iterator_begin() gets called with a path\n> whose leading component is not a directory), the caller will also\n> see ENOTDIR, but the distinction may not matter in practice. I\n> haven't thought things through.\n>\n> ...\n>\n> \tif (!iter) {\n> \t\tif (errno == ENOTDIR)\n> \t\t\tdie(_(\"'%s' is not a directory, refusing to clone with --local\"),\n> \t\t\t src->buf);\n> \t\telse\n> \t\t\tdie_errno(_(\"failed to stat '%s'\"), src->buf);\n> \t}\n>\n> may be sufficient. But because this is an error codepath, it is not\n> worth optimizing what happens there, and an extra lstat(2) is not\n> too bad, if the code gains extra clarity.\n\nYeah, the considerations here make sense to me.\n\nSince this is an error code path, I think the extra lstat() is probably\nworth it since it lets us be really specific about the error. Maybe:\n\n\tif (!iter) {\n\t\tstruct stat st;\n\n if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n\t\t\t src->buf);\n\n die_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n\t}\n\nDoing the extra lstat() only makes sense if we saw ENOTDIR orignally\nanyway.\n\nAlternatively, since we do care about the distinction between ENOTDIR\nfrom lstat vs ENOTDIR because dir_iterator_begin() saw a symlink, maybe\nit's better to refactor dir_iterator_begin() so that we stop\npiggybacking on \"errno = ENOTDIR\" in these two cases. I didn't want to\ndo this because I wasn't sure who might be relying on this behavior, but\nthis check was pretty recently introduced anyway (bffc762f87\n(dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS,\n2023-01-24), so maybe nobody needs it.\n"},{"id":"474877","messageId":"xmqq4jpuw2yj.fsf@gitster.g","threadId":"59544","inReplyTo":"kl6ly1n65l7t.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T19:20:36Z","receivedAt":"2023-04-05T19:20:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Since this is an error code path, I think the extra lstat() is probably\n> worth it since it lets us be really specific about the error.\n\nOK. Sounds good. Thanks.\n\n\n"},{"id":"474954","messageId":"ZC85ODogr79t0ZXZ@nand.local","threadId":"59544","inReplyTo":"xmqq7curxk22.fsf@gitster.g","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-06T21:27:20Z","receivedAt":"2023-04-06T21:27:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 04, 2023 at 05:13:41PM -0700, Junio C Hamano wrote:\n> * if lstat() succeeds, but the path is *not* a directory; errno is\n> explicitly set to ENOTDIR. Unfortunately, if lstat(2) failed\n> with ENOTDIR (e.g. dir_iterator_begin() gets called with a path\n> whose leading component is not a directory), the caller will also\n> see ENOTDIR, but the distinction may not matter in practice. I\n> haven't thought things through.\n\nYeah. The real tragedy here is that there's no way to signal that a file\n*is* a symbolic link and represent that as an error code in errno.\n\nSo I think the best that we can do here is to continue to report\nENOTDIR, and have the caller figure out what to do with it.\n\nThanks,\nTaylor\n"},{"id":"474956","messageId":"ZC87IcLcBnxBRCdr@nand.local","threadId":"59544","inReplyTo":"kl6ly1n65l7t.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-06T21:35:29Z","receivedAt":"2023-04-06T21:35:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 05, 2023 at 09:48:22AM -0700, Glen Choo wrote:\n> Since this is an error code path, I think the extra lstat() is probably\n> worth it since it lets us be really specific about the error. Maybe:\n\nFWIW, I don't totally think it's necessary for us to report back to the\nuser that they're trying to clone a repository with `--local` whose\n`$GIT_DIR/objects` either is or contains a symbolic link. Especially so\nsince you're already updating the documentation here.\n\nBut I think it's a compelling argument in the name of improving the user\nexperience, so I think that this is a reasonable direction. And I agree,\nthat while the extra lstat() is unfortunate, I don't think there is a\nconvenient way to do it otherwise.\n\nWe *could* teach the dir-iterator API to return a specialized error code either\nthrough a pointer, like:\n\n struct dir_iterator *dir_iterator_begin(const char *path,\n unsigned int flags, int *error)\n\nand set error to something like -DIR_ITERATOR_IS_SYMLINK when error is\nnon-NULL.\n\nOr we could do something like this:\n\n int dir_iterator_begin(struct dir_iterator **it,\n const char *path, unsigned int flags)\n\nand have the `dir_iterator_begin()` function return its specialized\nerror and initialize the dir iterator through a pointer. Between these\ntwo, I prefer the latter, but I think it's up to individual taste.\n\n> \tif (!iter) {\n> \t\tstruct stat st;\n>\n> if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n\nCouple of nit-picks here. Instead of writing \"lstat(src->buf, &st ==\n0)\", we should write \"!lstat(src->buf, &st)\" to match\nDocumentation/CodingGuidelines.\n\nBut note that that call to lstat() will clobber your errno value, so\nyou'd want to save it off beforehand. If you end up going this route,\nI'd probably do something like:\n\n if (!iter) {\n int saved_errno = errno;\n if (errno == ENOTDIR) {\n struct stat st;\n\n if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n die(_(\"'%s' is a symlink, refusing to clone with `--local`\"),\n src->buf);\n }\n errno = saved_errno;\n\n die_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n }\n\nThanks,\nTaylor\n"},{"id":"474958","messageId":"kl6lmt3k3ccc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59544","inReplyTo":"ZC87IcLcBnxBRCdr@nand.local","subject":"Re: [PATCH] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-06T21:55:15Z","receivedAt":"2023-04-06T21:56:01Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for the feedback!\n\n(and phew, I was a few minutes away from submitting v2 :P)\n\nTaylor Blau writes:\n\n> We *could* teach the dir-iterator API to return a specialized error code either\n> through a pointer, like:\n>\n> struct dir_iterator *dir_iterator_begin(const char *path,\n> unsigned int flags, int *error)\n>\n> and set error to something like -DIR_ITERATOR_IS_SYMLINK when error is\n> non-NULL.\n>\n> Or we could do something like this:\n>\n> int dir_iterator_begin(struct dir_iterator **it,\n> const char *path, unsigned int flags)\n>\n> and have the `dir_iterator_begin()` function return its specialized\n> error and initialize the dir iterator through a pointer. Between these\n> two, I prefer the latter, but I think it's up to individual taste.\n\nYeah, I've thought about doing the latter. That's also what I'd prefer.\nSince you've brought it up, I'll give that a try.\n\n>> \tif (!iter) {\n>> \t\tstruct stat st;\n>>\n>> if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n>\n> Couple of nit-picks here. Instead of writing \"lstat(src->buf, &st ==\n> 0)\", we should write \"!lstat(src->buf, &st)\" to match\n> Documentation/CodingGuidelines.\n\nAh, thanks.\n\n> But note that that call to lstat() will clobber your errno value, so\n> you'd want to save it off beforehand. If you end up going this route,\n> I'd probably do something like:\n>\n> if (!iter) {\n> int saved_errno = errno;\n> if (errno == ENOTDIR) {\n> struct stat st;\n>\n> if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n> die(_(\"'%s' is a symlink, refusing to clone with `--local`\"),\n> src->buf);\n> }\n> errno = saved_errno;\n>\n> die_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n> }\n\nAh thanks, yes. I think it isn't different in practice, since we're\nlstat()-ing the same path, but we should always use the \"real\" errno to\nbe safe.\n\nThis is another good reason to return an error code instead of\noverloading errno.\n"},{"id":"475085","messageId":"pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com","threadId":"59544","inReplyTo":"pull.1488.git.git.1680652122547.gitgitgadget@gmail.com","subject":"[PATCH v2] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-10T22:18:50Z","receivedAt":"2023-04-10T22:18:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n6f054f9fb3 (builtin/clone.c: disallow --local clones with\nsymlinks, 2022-07-28) gives a good error message when \"git clone\n--local\" fails when the repo to clone has symlinks in\n\"$GIT_DIR/objects\". In bffc762f87 (dir-iterator: prevent top-level\nsymlinks without FOLLOW_SYMLINKS, 2023-01-24), we later extended this\nrestriction to the case where \"$GIT_DIR/objects\" is itself a symlink,\nbut we didn't update the error message then - bffc762f87's tests show\nthat we print a generic \"failed to start iterator over\" message.\n\nThis is exacerbated by the fact that Documentation/git-clone.txt\nmentions neither restriction, so users are left wondering if this is\nintentional behavior or not.\n\nFix this by adding a check to builtin/clone.c: when doing a local clone,\nperform an extra check to see if \"$GIT_DIR/objects\" is a symlink, and if\nso, assume that that was the reason for the failure and report the\nrelevant information. Ideally, dir_iterator_begin() would tell us that\nthe real failure reason is the presence of the symlink, but (as far as I\ncan tell) there isn't an appropriate errno value for that.\n\nAlso, update Documentation/git-clone.txt to reflect that this\nrestriction exists.\n\nSigned-off-by: Glen Choo \n---\n clone: error specifically with --local and symlinked objects\n \n Thanks for the close review on v1, folks.\n \n I tried teaching dir_iterator_begin() to return an exit code to indicate\n that we found a symlink, but it ended up looking quite ugly when I\n started to consider plain files as well as generic lstat failures. I\n think the extra lstat() is fine as a one-off, but if we need another\n instance of this, we'd be better off doing the refactor.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1488%2Fchooglen%2Fpush-nymnqqnttzxz-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1488/chooglen/push-nymnqqnttzxz-v2\nPull-Request: https://github.com/git/git/pull/1488\n\nRange-diff vs v1:\n\n 1: 5c2d5eb10cd ! 1: 574cea062cf clone: error specifically with --local and symlinked objects\n @@ builtin/clone.c: static void copy_or_link_directory(struct strbuf *src, struct s\n \n -\tif (!iter)\n +\tif (!iter) {\n -+\t\tstruct stat st;\n -+\t\tif (lstat(src->buf, &st) >= 0 && S_ISLNK(st.st_mode))\n -+\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n -+\t\t\t src->buf);\n ++\t\tif (errno == ENOTDIR) {\n ++\t\t\tint saved_errno = errno;\n ++\t\t\tstruct stat st;\n ++\t\t\tif (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n ++\t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n ++\t\t\t\t src->buf);\n ++\t\t\terrno = saved_errno;\n ++\t\t}\n \t\tdie_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n +\t}\n \n\n\n Documentation/git-clone.txt | 5 +++++\n builtin/clone.c | 11 ++++++++++-\n t/t5604-clone-reference.sh | 2 +-\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex d6434d262d6..c37c4a37f74 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -58,6 +58,11 @@ never use the local optimizations). Specifying `--no-local` will\n override the default when `/path/to/repo` is given, using the regular\n Git transport instead.\n +\n+If the repository's `$GIT_DIR/objects` has symbolic links or is a\n+symbolic link, the clone will fail. This is a security measure to\n+prevent the unintentional copying of files by dereferencing the symbolic\n+links.\n++\n *NOTE*: this operation can race with concurrent modification to the\n source repository, similar to running `cp -r src dst` while modifying\n `src`.\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 462c286274c..46f6f689c85 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -327,8 +327,17 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \n \titer = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);\n \n-\tif (!iter)\n+\tif (!iter) {\n+\t\tif (errno == ENOTDIR) {\n+\t\t\tint saved_errno = errno;\n+\t\t\tstruct stat st;\n+\t\t\tif (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n+\t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n+\t\t\t\t src->buf);\n+\t\t\terrno = saved_errno;\n+\t\t}\n \t\tdie_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n+\t}\n \n \tstrbuf_addch(src, '/');\n \tsrc_len = src->len;\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 83e3c97861d..9845fc04d59 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -358,7 +358,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '\n \ttest_must_fail git clone --local malicious clone 2>err &&\n \n \ttest_path_is_missing clone &&\n-\tgrep \"failed to start iterator over\" err\n+\tgrep \"is a symlink, refusing to clone with --local\" err\n '\n \n test_done\n\nbase-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e\n-- \ngitgitgadget\n"},{"id":"475086","messageId":"xmqqttxncr0k.fsf@gitster.g","threadId":"59544","inReplyTo":"pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone: error specifically with --local and symlinked objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-10T22:27:07Z","receivedAt":"2023-04-10T22:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> ++\t\tif (errno == ENOTDIR) {\n> ++\t\t\tint saved_errno = errno;\n> ++\t\t\tstruct stat st;\n> ++\t\t\tif (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n> ++\t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n> ++\t\t\t\t src->buf);\n> ++\t\t\terrno = saved_errno;\n> ++\t\t}\n> \t\tdie_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n\nI doubt you need saved_errno in the code immediately after this\npatch gets applied, as what you are saving is guaranteed to be\nENOTDIR so you can just restore it before you fall through out of\nthe block.\n\nIt however would not hurt, though. Especially if the condition that\nguarding this block is likely to change in the future, we can view\nthe apparent waste as paying insurance premium to protect from\nfuture breakages.\n\nWill queue. Thanks.\n\n"},{"id":"475098","messageId":"ZDSYyDjJ4ks4ECNv@nand.local","threadId":"59544","inReplyTo":"pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone: error specifically with --local and symlinked objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T23:16:24Z","receivedAt":"2023-04-10T23:16:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 10, 2023 at 10:18:50PM +0000, Glen Choo via GitGitGadget wrote:\n> I tried teaching dir_iterator_begin() to return an exit code to indicate\n> that we found a symlink, but it ended up looking quite ugly when I\n> started to consider plain files as well as generic lstat failures. I\n> think the extra lstat() is fine as a one-off, but if we need another\n> instance of this, we'd be better off doing the refactor.\n\n;-). Heh, I was wondering what happened since your [1] made it sound\nlike you would investigate making dir_iterator_begin() return an error\ncode.\n\n[1]: https://lore.kernel.org/git/kl6lmt3k3ccc.fsf@chooglen-macbookpro.roam.corp.google.com/\n\n> Documentation/git-clone.txt | 5 +++++\n> builtin/clone.c | 11 ++++++++++-\n> t/t5604-clone-reference.sh | 2 +-\n> 3 files changed, 16 insertions(+), 2 deletions(-)\n\nBut if it turned out ugly, I don't mind; this version looks great to me.\n\nThanks,\nTaylor\n"},{"id":"475107","messageId":"ZDS+zJAilFqxMSqa@nand.local","threadId":"59544","inReplyTo":"pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone: error specifically with --local and symlinked objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T01:58:36Z","receivedAt":"2023-04-11T01:58:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 10, 2023 at 10:18:50PM +0000, Glen Choo via GitGitGadget wrote:\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 462c286274c..46f6f689c85 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -327,8 +327,17 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n>\n> \titer = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);\n>\n> -\tif (!iter)\n> +\tif (!iter) {\n> +\t\tif (errno == ENOTDIR) {\n> +\t\t\tint saved_errno = errno;\n> +\t\t\tstruct stat st;\n> +\t\t\tif (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n\nI missed it on my first read, but you may want to consider \"!lstat(...)\"\ninstead of \"lstat(...) == 0\". Probably not worth a reroll, though.\n\nThanks,\nTaylor\n"},{"id":"475157","messageId":"pull.1488.v3.git.git.1681232918484.gitgitgadget@gmail.com","threadId":"59544","inReplyTo":"pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com","subject":"[PATCH v3] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-11T17:08:38Z","receivedAt":"2023-04-11T17:08:44Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n6f054f9fb3 (builtin/clone.c: disallow --local clones with\nsymlinks, 2022-07-28) gives a good error message when \"git clone\n--local\" fails when the repo to clone has symlinks in\n\"$GIT_DIR/objects\". In bffc762f87 (dir-iterator: prevent top-level\nsymlinks without FOLLOW_SYMLINKS, 2023-01-24), we later extended this\nrestriction to the case where \"$GIT_DIR/objects\" is itself a symlink,\nbut we didn't update the error message then - bffc762f87's tests show\nthat we print a generic \"failed to start iterator over\" message.\n\nThis is exacerbated by the fact that Documentation/git-clone.txt\nmentions neither restriction, so users are left wondering if this is\nintentional behavior or not.\n\nFix this by adding a check to builtin/clone.c: when doing a local clone,\nperform an extra check to see if \"$GIT_DIR/objects\" is a symlink, and if\nso, assume that that was the reason for the failure and report the\nrelevant information. Ideally, dir_iterator_begin() would tell us that\nthe real failure reason is the presence of the symlink, but (as far as I\ncan tell) there isn't an appropriate errno value for that.\n\nAlso, update Documentation/git-clone.txt to reflect that this\nrestriction exists.\n\nSigned-off-by: Glen Choo \n---\n clone: error specifically with --local and symlinked objects\n \n A quick reroll to fix the style issue Taylor spotted in v2 (good\n catch!). I didn't spot this patch in 'next' or 'seen', so hopefully I'm\n still in time :)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1488%2Fchooglen%2Fpush-nymnqqnttzxz-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1488/chooglen/push-nymnqqnttzxz-v3\nPull-Request: https://github.com/git/git/pull/1488\n\nRange-diff vs v2:\n\n 1: 574cea062cf ! 1: 4c5fc42594b clone: error specifically with --local and symlinked objects\n @@ builtin/clone.c: static void copy_or_link_directory(struct strbuf *src, struct s\n +\t\tif (errno == ENOTDIR) {\n +\t\t\tint saved_errno = errno;\n +\t\t\tstruct stat st;\n -+\t\t\tif (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))\n ++\t\t\tif (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n +\t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n +\t\t\t\t src->buf);\n +\t\t\terrno = saved_errno;\n\n\n Documentation/git-clone.txt | 5 +++++\n builtin/clone.c | 11 ++++++++++-\n t/t5604-clone-reference.sh | 2 +-\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex d6434d262d6..c37c4a37f74 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -58,6 +58,11 @@ never use the local optimizations). Specifying `--no-local` will\n override the default when `/path/to/repo` is given, using the regular\n Git transport instead.\n +\n+If the repository's `$GIT_DIR/objects` has symbolic links or is a\n+symbolic link, the clone will fail. This is a security measure to\n+prevent the unintentional copying of files by dereferencing the symbolic\n+links.\n++\n *NOTE*: this operation can race with concurrent modification to the\n source repository, similar to running `cp -r src dst` while modifying\n `src`.\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 462c286274c..dd2b4e130e6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -327,8 +327,17 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \n \titer = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);\n \n-\tif (!iter)\n+\tif (!iter) {\n+\t\tif (errno == ENOTDIR) {\n+\t\t\tint saved_errno = errno;\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n+\t\t\t\t src->buf);\n+\t\t\terrno = saved_errno;\n+\t\t}\n \t\tdie_errno(_(\"failed to start iterator over '%s'\"), src->buf);\n+\t}\n \n \tstrbuf_addch(src, '/');\n \tsrc_len = src->len;\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex 83e3c97861d..9845fc04d59 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -358,7 +358,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '\n \ttest_must_fail git clone --local malicious clone 2>err &&\n \n \ttest_path_is_missing clone &&\n-\tgrep \"failed to start iterator over\" err\n+\tgrep \"is a symlink, refusing to clone with --local\" err\n '\n \n test_done\n\nbase-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e\n-- \ngitgitgadget\n"},{"id":"475159","messageId":"xmqqmt3e9wab.fsf@gitster.g","threadId":"59544","inReplyTo":"pull.1488.v3.git.git.1681232918484.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] clone: error specifically with --local and symlinked objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T17:13:48Z","receivedAt":"2023-04-11T17:13:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> A quick reroll to fix the style issue Taylor spotted in v2 (good\n> catch!). I didn't spot this patch in 'next' or 'seen', so hopefully I'm\n> still in time :)\n\nWhat I locally have, which is an amended version of v2, and this one\ndiffer like so:\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ae2db8535a..b42231758c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -335,7 +335,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \t\tif (errno == ENOTDIR) {\n \t\t\tint saved_errno = errno;\n \t\t\tstruct stat st;\n-\n \t\t\tif (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n \t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n \t\t\t\t src->buf);\n\nSo I'll keep my copy.\n\nThanks anyway.\n"},{"id":"475163","messageId":"kl6lr0sqjmce.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59544","inReplyTo":"xmqqmt3e9wab.fsf@gitster.g","subject":"Re: [PATCH v3] clone: error specifically with --local and symlinked objects","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-11T18:38:25Z","receivedAt":"2023-04-11T18:38:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> What I locally have, which is an amended version of v2, and this one\n> differ like so:\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index ae2db8535a..b42231758c 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -335,7 +335,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n> \t\tif (errno == ENOTDIR) {\n> \t\t\tint saved_errno = errno;\n> \t\t\tstruct stat st;\n> -\n> \t\t\tif (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))\n> \t\t\t\tdie(_(\"'%s' is a symlink, refusing to clone with --local\"),\n> \t\t\t\t src->buf);\n>\n> So I'll keep my copy.\n\nAh, sharp eye. Sounds good.\n"}]}