diff mbox series

[v3] clone: error specifically with --local and symlinked objects

Message ID pull.1488.v3.git.git.1681232918484.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v3] clone: error specifically with --local and symlinked objects | expand

Commit Message

Glen Choo April 11, 2023, 5:08 p.m. UTC 
From: Glen Choo <chooglen@google.com>

6f054f9fb3 (builtin/clone.c: disallow --local clones with
symlinks, 2022-07-28) gives a good error message when "git clone
--local" fails when the repo to clone has symlinks in
"$GIT_DIR/objects". In bffc762f87 (dir-iterator: prevent top-level
symlinks without FOLLOW_SYMLINKS, 2023-01-24), we later extended this
restriction to the case where "$GIT_DIR/objects" is itself a symlink,
but we didn't update the error message then - bffc762f87's tests show
that we print a generic "failed to start iterator over" message.

This is exacerbated by the fact that Documentation/git-clone.txt
mentions neither restriction, so users are left wondering if this is
intentional behavior or not.

Fix this by adding a check to builtin/clone.c: when doing a local clone,
perform an extra check to see if "$GIT_DIR/objects" is a symlink, and if
so, assume that that was the reason for the failure and report the
relevant information. Ideally, dir_iterator_begin() would tell us that
the real failure reason is the presence of the symlink, but (as far as I
can tell) there isn't an appropriate errno value for that.

Also, update Documentation/git-clone.txt to reflect that this
restriction exists.

Signed-off-by: Glen Choo <chooglen@google.com>
---
    clone: error specifically with --local and symlinked objects
    
    A quick reroll to fix the style issue Taylor spotted in v2 (good
    catch!). I didn't spot this patch in 'next' or 'seen', so hopefully I'm
    still in time :)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1488%2Fchooglen%2Fpush-nymnqqnttzxz-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1488/chooglen/push-nymnqqnttzxz-v3
Pull-Request: https://github.com/git/git/pull/1488

Range-diff vs v2:

 1:  574cea062cf ! 1:  4c5fc42594b clone: error specifically with --local and symlinked objects
     @@ builtin/clone.c: static void copy_or_link_directory(struct strbuf *src, struct s
      +		if (errno == ENOTDIR) {
      +			int saved_errno = errno;
      +			struct stat st;
     -+			if (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))
     ++			if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
      +				die(_("'%s' is a symlink, refusing to clone with --local"),
      +				    src->buf);
      +			errno = saved_errno;


 Documentation/git-clone.txt |  5 +++++
 builtin/clone.c             | 11 ++++++++++-
 t/t5604-clone-reference.sh  |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)


base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e

Comments

Junio C Hamano April 11, 2023, 5:13 p.m. UTC | #1 
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     A quick reroll to fix the style issue Taylor spotted in v2 (good
>     catch!). I didn't spot this patch in 'next' or 'seen', so hopefully I'm
>     still in time :)

What I locally have, which is an amended version of v2, and this one
differ like so:

diff --git a/builtin/clone.c b/builtin/clone.c
index ae2db8535a..b42231758c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -335,7 +335,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		if (errno == ENOTDIR) {
 			int saved_errno = errno;
 			struct stat st;
-
 			if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
 				die(_("'%s' is a symlink, refusing to clone with --local"),
 				    src->buf);

So I'll keep my copy.

Thanks anyway.
Glen Choo April 11, 2023, 6:38 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> What I locally have, which is an amended version of v2, and this one
> differ like so:
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ae2db8535a..b42231758c 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -335,7 +335,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  		if (errno == ENOTDIR) {
>  			int saved_errno = errno;
>  			struct stat st;
> -
>  			if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
>  				die(_("'%s' is a symlink, refusing to clone with --local"),
>  				    src->buf);
>
> So I'll keep my copy.

Ah, sharp eye. Sounds good.
diff mbox series

Patch

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index d6434d262d6..c37c4a37f74 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -58,6 +58,11 @@  never use the local optimizations).  Specifying `--no-local` will
 override the default when `/path/to/repo` is given, using the regular
 Git transport instead.
 +
+If the repository's `$GIT_DIR/objects` has symbolic links or is a
+symbolic link, the clone will fail. This is a security measure to
+prevent the unintentional copying of files by dereferencing the symbolic
+links.
++
 *NOTE*: this operation can race with concurrent modification to the
 source repository, similar to running `cp -r src dst` while modifying
 `src`.
diff --git a/builtin/clone.c b/builtin/clone.c
index 462c286274c..dd2b4e130e6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -327,8 +327,17 @@  static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 
 	iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);
 
-	if (!iter)
+	if (!iter) {
+		if (errno == ENOTDIR) {
+			int saved_errno = errno;
+			struct stat st;
+			if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
+				die(_("'%s' is a symlink, refusing to clone with --local"),
+				    src->buf);
+			errno = saved_errno;
+		}
 		die_errno(_("failed to start iterator over '%s'"), src->buf);
+	}
 
 	strbuf_addch(src, '/');
 	src_len = src->len;
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 83e3c97861d..9845fc04d59 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -358,7 +358,7 @@  test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '
 	test_must_fail git clone --local malicious clone 2>err &&
 
 	test_path_is_missing clone &&
-	grep "failed to start iterator over" err
+	grep "is a symlink, refusing to clone with --local" err
 '
 
 test_done