dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS

When using the dir_iterator API, we first stat(2) the base path, and
then use that as a starting point to enumerate the directory's contents.

If the directory contains symbolic links, we will immediately die() upon
encountering them without the `FOLLOW_SYMLINKS` flag. The same is not
true when resolving the top-level directory, though.

As explained in a previous commit, this oversight in 6f054f9fb3
(builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28)
can be used as an attack vector to include arbitrary files on a victim's
filesystem from outside of the repository.

Prevent resolving top-level symlinks unless the FOLLOW_SYMLINKS flag is
given, which will cause clones of a repository with a symlink'd
"$GIT_DIR/objects" directory to fail.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/dir-iterator.c b/dir-iterator.c
index b17e9f970a747a3ac85aae81a92c38d2a6491546..3764dd81a185b296a5d4c0a9fc8c0ae2bd4085ec 100644 (file)
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -203,7 +203,7 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
 {
        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 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
        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 08229157c638040cb19f7dd32b680fbea45f3965..e3b6ff28007366568d981631d5b312bd849486c0 100644 (file)
--- 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 92910e4e6c13ce381dddefcf197dea26a07efd88..c826f60f6d6f897a1151f4ce39e35c88297a25e2 100755 (executable)
--- a/t/t0066-dir-iterator.sh
+++ b/t/t0066-dir-iterator.sh
@@ -109,7 +109,9 @@ test_expect_success SYMLINKS 'setup dirs with symlinks' '
        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_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag
        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 9d32f1c4a4962c50d22a3f51dc752dd96278faf9..4ff21d7ccf3f4106d8b6ae1c13baed5e4747ca80 100755 (executable)
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -341,4 +341,20 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje
        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