Merge branch 'ps/apply-beyond-symlink' into maint-2.30
Fix a vulnerability (CVE-2023-23946) that allows crafted input to trick
`git apply` into writing files outside of the working tree.
* ps/apply-beyond-symlink:
dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
diff --git a/builtin/clone.c b/builtin/clone.c
index e626073..c042b2e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1201,10 +1201,6 @@
refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
branch_top.buf);
- transport = transport_get(remote, remote->url[0]);
- transport_set_verbosity(transport, option_verbosity, option_progress);
- transport->family = family;
-
path = get_repo_path(remote->url[0], &is_bundle);
is_local = option_local != 0 && path && !is_bundle;
if (is_local) {
@@ -1224,6 +1220,10 @@
}
if (option_local > 0 && !is_local)
warning(_("--local is ignored"));
+
+ transport = transport_get(remote, path ? path : remote->url[0]);
+ transport_set_verbosity(transport, option_verbosity, option_progress);
+ transport->family = family;
transport->cloning = 1;
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
diff --git a/dir-iterator.c b/dir-iterator.c
index b17e9f9..3764dd8 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -203,7 +203,7 @@
{
struct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));
struct dir_iterator *dir_iterator = &iter->base;
- int saved_errno;
+ int saved_errno, err;
strbuf_init(&iter->base.path, PATH_MAX);
strbuf_addstr(&iter->base.path, path);
@@ -213,10 +213,15 @@
iter->flags = flags;
/*
- * Note: stat already checks for NULL or empty strings and
- * inexistent paths.
+ * Note: stat/lstat already checks for NULL or empty strings and
+ * nonexistent paths.
*/
- if (stat(iter->base.path.buf, &iter->base.st) < 0) {
+ if (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)
+ err = stat(iter->base.path.buf, &iter->base.st);
+ else
+ err = lstat(iter->base.path.buf, &iter->base.st);
+
+ if (err < 0) {
saved_errno = errno;
goto error_out;
}
diff --git a/dir-iterator.h b/dir-iterator.h
index 0822915..e3b6ff2 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -61,6 +61,11 @@
* not the symlinks themselves, which is the default behavior. Broken
* symlinks are ignored.
*
+ * Note: setting DIR_ITERATOR_FOLLOW_SYMLINKS affects resolving the
+ * starting path as well (e.g., attempting to iterate starting at a
+ * symbolic link pointing to a directory without FOLLOW_SYMLINKS will
+ * result in an error).
+ *
* Warning: circular symlinks are also followed when
* DIR_ITERATOR_FOLLOW_SYMLINKS is set. The iteration may end up with
* an ELOOP if they happen and DIR_ITERATOR_PEDANTIC is set.
diff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh
index 92910e4..c826f60 100755
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -109,7 +109,9 @@
mkdir -p dir5/a/c &&
ln -s ../c dir5/a/b/d &&
ln -s ../ dir5/a/b/e &&
- ln -s ../../ dir5/a/b/f
+ ln -s ../../ dir5/a/b/f &&
+
+ ln -s dir4 dir6
'
test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '
@@ -145,4 +147,27 @@
test_cmp expected-follow-sorted-output actual-follow-sorted-output
'
+test_expect_success SYMLINKS 'dir-iterator does not resolve top-level symlinks' '
+ test_must_fail test-tool dir-iterator ./dir6 >out &&
+
+ grep "ENOTDIR" out
+'
+
+test_expect_success SYMLINKS 'dir-iterator resolves top-level symlinks w/ follow flag' '
+ cat >expected-follow-sorted-output <<-EOF &&
+ [d] (a) [a] ./dir6/a
+ [d] (a/f) [f] ./dir6/a/f
+ [d] (a/f/c) [c] ./dir6/a/f/c
+ [d] (b) [b] ./dir6/b
+ [d] (b/c) [c] ./dir6/b/c
+ [f] (a/d) [d] ./dir6/a/d
+ [f] (a/e) [e] ./dir6/a/e
+ EOF
+
+ test-tool dir-iterator --follow-symlinks ./dir6 >out &&
+ sort out >actual-follow-sorted-output &&
+
+ test_cmp expected-follow-sorted-output actual-follow-sorted-output
+'
+
test_done
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 9d32f1c..4ff21d7 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -341,4 +341,20 @@
test_must_be_empty T--shared.objects-symlinks.raw
'
+test_expect_success SYMLINKS 'clone repo with symlinked objects directory' '
+ test_when_finished "rm -fr sensitive malicious" &&
+
+ mkdir -p sensitive &&
+ echo "secret" >sensitive/file &&
+
+ git init malicious &&
+ rm -fr malicious/.git/objects &&
+ ln -s "$(pwd)/sensitive" ./malicious/.git/objects &&
+
+ test_must_fail git clone --local malicious clone 2>err &&
+
+ test_path_is_missing clone &&
+ grep "failed to start iterator over" err
+'
+
test_done
diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh
new file mode 100755
index 0000000..cce62bf
--- /dev/null
+++ b/t/t5619-clone-local-ambiguous-transport.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='test local clone with ambiguous transport'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-httpd.sh"
+
+if ! test_have_prereq SYMLINKS
+then
+ skip_all='skipping test, symlink support unavailable'
+ test_done
+fi
+
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/sub.git"
+URI="$HTTPD_URL/dumb/sub.git"
+
+test_expect_success 'setup' '
+ mkdir -p sensitive &&
+ echo "secret" >sensitive/secret &&
+
+ git init --bare "$REPO" &&
+ test_commit_bulk -C "$REPO" --ref=main 1 &&
+
+ git -C "$REPO" update-ref HEAD main &&
+ git -C "$REPO" update-server-info &&
+
+ git init malicious &&
+ (
+ cd malicious &&
+
+ git submodule add "$URI" &&
+
+ mkdir -p repo/refs &&
+ touch repo/refs/.gitkeep &&
+ printf "ref: refs/heads/a" >repo/HEAD &&
+ ln -s "$(cd .. && pwd)/sensitive" repo/objects &&
+
+ mkdir -p "$HTTPD_URL/dumb" &&
+ ln -s "../../../.git/modules/sub/../../../repo/" "$URI" &&
+
+ git add . &&
+ git commit -m "initial commit"
+ ) &&
+
+ # Delete all of the references in our malicious submodule to
+ # avoid the client attempting to checkout any objects (which
+ # will be missing, and thus will cause the clone to fail before
+ # we can trigger the exploit).
+ git -C "$REPO" for-each-ref --format="delete %(refname)" >in &&
+ git -C "$REPO" update-ref --stdin <in &&
+ git -C "$REPO" update-server-info
+'
+
+test_expect_success 'ambiguous transport does not lead to arbitrary file-inclusion' '
+ git clone malicious clone &&
+ test_must_fail git -C clone submodule update --init 2>err &&
+
+ test_path_is_missing clone/.git/modules/sub/objects/secret &&
+ # We would actually expect "transport .file. not allowed" here,
+ # but due to quirks of the URL detection in Git, we mis-parse
+ # the absolute path as a bogus URL and die before that step.
+ #
+ # This works for now, and if we ever fix the URL detection, it
+ # is OK to change this to detect the transport error.
+ grep "protocol .* is not supported" err
+'
+
+test_done
