From bffc762f87ae8d18c6001bf0044a76004245754c Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 24 Jan 2023 19:43:51 -0500 Subject: [PATCH 3/3] 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 6f054f9 (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 Signed-off-by: Junio C Hamano Upstream-Status: Backport [https://github.com/git/git/commit/bffc762f87ae8d18c6001bf0044a76004245754c] CVE: CVE-2023-22490 Signed-off-by: Vijay Anusuri --- dir-iterator.c | 13 +++++++++---- dir-iterator.h | 5 +++++ t/t0066-dir-iterator.sh | 27 ++++++++++++++++++++++++++- t/t5604-clone-reference.sh | 16 ++++++++++++++++ 4 files changed, 56 insertions(+), 5 deletions(-) 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 *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 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 @@ 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 4894237..615b981 100755 --- a/t/t5604-clone-reference.sh +++ b/t/t5604-clone-reference.sh @@ -354,4 +354,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 -- 2.25.1