From 684dd4c2b414bcf648505e74498a608f28de4592 Mon Sep 17 00:00:00 2001 From: Matheus Tavares Date: Thu, 10 Dec 2020 10:27:55 -0300 Subject: checkout: fix bug that makes checkout follow symlinks in leading path Before checking out a file, we have to confirm that all of its leading components are real existing directories. And to reduce the number of lstat() calls in this process, we cache the last leading path known to contain only directories. However, when a path collision occurs (e.g. when checking out case-sensitive files in case-insensitive file systems), a cached path might have its file type changed on disk, leaving the cache on an invalid state. Normally, this doesn't bring any bad consequences as we usually check out files in index order, and therefore, by the time the cached path becomes outdated, we no longer need it anyway (because all files in that directory would have already been written). But, there are some users of the checkout machinery that do not always follow the index order. In particular: checkout-index writes the paths in the same order that they appear on the CLI (or stdin); and the delayed checkout feature -- used when a long-running filter process replies with "status=delayed" -- postpones the checkout of some entries, thus modifying the checkout order. When we have to check out an out-of-order entry and the lstat() cache is invalid (due to a previous path collision), checkout_entry() may end up using the invalid data and thrusting that the leading components are real directories when, in reality, they are not. In the best case scenario, where the directory was replaced by a regular file, the user will get an error: "fatal: unable to create file 'foo/bar': Not a directory". But if the directory was replaced by a symlink, checkout could actually end up following the symlink and writing the file at a wrong place, even outside the repository. Since delayed checkout is affected by this bug, it could be used by an attacker to write arbitrary files during the clone of a maliciously crafted repository. Some candidate solutions considered were to disable the lstat() cache during unordered checkouts or sort the entries before passing them to the checkout machinery. But both ideas include some performance penalty and they don't future-proof the code against new unordered use cases. Instead, we now manually reset the lstat cache whenever we successfully remove a directory. Note: We are not even checking whether the directory was the same as the lstat cache points to because we might face a scenario where the paths refer to the same location but differ due to case folding, precomposed UTF-8 issues, or the presence of `..` components in the path. Two regression tests, with case-collisions and utf8-collisions, are also added for both checkout-index and delayed checkout. Note: to make the previously mentioned clone attack unfeasible, it would be sufficient to reset the lstat cache only after the remove_subtree() call inside checkout_entry(). This is the place where we would remove a directory whose path collides with the path of another entry that we are currently trying to check out (possibly a symlink). However, in the interest of a thorough fix that does not leave Git open to similar-but-not-identical attack vectors, we decided to intercept all `rmdir()` calls in one fell swoop. This addresses CVE-2021-21300. Co-authored-by: Johannes Schindelin Signed-off-by: Matheus Tavares --- cache.h | 1 + compat/mingw.c | 2 ++ git-compat-util.h | 5 +++++ symlinks.c | 24 +++++++++++++++++++++ t/t0021-conversion.sh | 45 ++++++++++++++++++++++++++++++++++++++++ t/t0021/rot13-filter.pl | 21 ++++++++++++++++--- t/t2006-checkout-index-basic.sh | 46 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 141 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 0323853c99..c530593971 100644 --- a/cache.h +++ b/cache.h @@ -1569,6 +1569,7 @@ extern int has_symlink_leading_path(const char *name, int len); extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int); extern int check_leading_path(const char *name, int len); extern int has_dirs_only_path(const char *name, int len, int prefix_len); +extern void invalidate_lstat_cache(void); extern void schedule_dir_for_removal(const char *name, int len); extern void remove_scheduled_dirs(void); diff --git a/compat/mingw.c b/compat/mingw.c index b047e21660..0c414d08b6 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -283,6 +283,8 @@ int mingw_rmdir(const char *pathname) ask_yes_no_if_possible("Deletion of directory '%s' failed. " "Should I try again?", pathname)) ret = _wrmdir(wpathname); + if (!ret) + invalidate_lstat_cache(); return ret; } diff --git a/git-compat-util.h b/git-compat-util.h index 37277494f9..6230f9aaf3 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -338,6 +338,11 @@ typedef uintmax_t timestamp_t; #define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin" #endif +int lstat_cache_aware_rmdir(const char *path); +#if !defined(__MINGW32__) && !defined(_MSC_VER) +#define rmdir lstat_cache_aware_rmdir +#endif + #ifndef has_dos_drive_prefix static inline int git_has_dos_drive_prefix(const char *path) { diff --git a/symlinks.c b/symlinks.c index 5261e8cf49..53b770be08 100644 --- a/symlinks.c +++ b/symlinks.c @@ -267,6 +267,13 @@ int has_dirs_only_path(const char *name, int len, int prefix_len) */ static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len) { + /* + * Note: this function is used by the checkout machinery, which also + * takes care to properly reset the cache when it performs an operation + * that would leave the cache outdated. If this function starts caching + * anything else besides FL_DIR, remember to also invalidate the cache + * when creating or deleting paths that might be in the cache. + */ return lstat_cache(cache, name, len, FL_DIR|FL_FULLPATH, prefix_len) & FL_DIR; @@ -321,3 +328,20 @@ void remove_scheduled_dirs(void) { do_remove_scheduled_dirs(0); } + +void invalidate_lstat_cache(void) +{ + reset_lstat_cache(&default_cache); +} + +#undef rmdir +int lstat_cache_aware_rmdir(const char *path) +{ + /* Any change in this function must be made also in `mingw_rmdir()` */ + int ret = rmdir(path); + + if (!ret) + invalidate_lstat_cache(); + + return ret; +} diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 46f8e583c3..8ff917fca6 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -817,4 +817,49 @@ test_expect_success PERL 'invalid file in delayed checkout' ' grep "error: external filter .* signaled that .unfiltered. is now available although it has not been delayed earlier" git-stderr.log ' +for mode in 'case' 'utf-8' +do + case "$mode" in + case) dir='A' symlink='a' mode_prereq='CASE_INSENSITIVE_FS' ;; + utf-8) + dir=$(printf "\141\314\210") symlink=$(printf "\303\244") + mode_prereq='UTF8_NFD_TO_NFC' ;; + esac + + test_expect_success PERL,SYMLINKS,$mode_prereq \ + "delayed checkout with $mode-collision don't write to the wrong place" ' + test_config_global filter.delay.process \ + "\"$TEST_ROOT/rot13-filter.pl\" --always-delay delayed.log clean smudge delay" && + test_config_global filter.delay.required true && + + git init $mode-collision && + ( + cd $mode-collision && + mkdir target-dir && + + empty_oid=$(printf "" | git hash-object -w --stdin) && + symlink_oid=$(printf "%s" "$PWD/target-dir" | git hash-object -w --stdin) && + attr_oid=$(echo "$dir/z filter=delay" | git hash-object -w --stdin) && + + cat >objs <<-EOF && + 100644 blob $empty_oid $dir/x + 100644 blob $empty_oid $dir/y + 100644 blob $empty_oid $dir/z + 120000 blob $symlink_oid $symlink + 100644 blob $attr_oid .gitattributes + EOF + + git update-index --index-info +# +# Log path defines a debug log file that the script writes to. The +# subsequent arguments define a list of supported protocol capabilities +# ("clean", "smudge", etc). +# +# When --always-delay is given all pathnames with the "can-delay" flag +# that don't appear on the list bellow are delayed with a count of 1 +# (see more below). # # This implementation supports special test cases: # (1) If data with the pathname "clean-write-fail.r" is processed with @@ -53,6 +59,13 @@ use IO::File; use Git::Packet; my $MAX_PACKET_CONTENT_SIZE = 65516; + +my $always_delay = 0; +if ( $ARGV[0] eq '--always-delay' ) { + $always_delay = 1; + shift @ARGV; +} + my $log_file = shift @ARGV; my @capabilities = @ARGV; @@ -134,6 +147,8 @@ while (1) { if ( $buffer eq "can-delay=1" ) { if ( exists $DELAY{$pathname} and $DELAY{$pathname}{"requested"} == 0 ) { $DELAY{$pathname}{"requested"} = 1; + } elsif ( !exists $DELAY{$pathname} and $always_delay ) { + $DELAY{$pathname} = { "requested" => 1, "count" => 1 }; } } else { die "Unknown message '$buffer'"; diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh index 57cbdfe9bc..19aada33a3 100755 --- a/t/t2006-checkout-index-basic.sh +++ b/t/t2006-checkout-index-basic.sh @@ -21,4 +21,50 @@ test_expect_success 'checkout-index -h in broken repository' ' test_i18ngrep "[Uu]sage" broken/usage ' +for mode in 'case' 'utf-8' +do + case "$mode" in + case) dir='A' symlink='a' mode_prereq='CASE_INSENSITIVE_FS' ;; + utf-8) + dir=$(printf "\141\314\210") symlink=$(printf "\303\244") + mode_prereq='UTF8_NFD_TO_NFC' ;; + esac + + test_expect_success SYMLINKS,$mode_prereq \ + "checkout-index with $mode-collision don't write to the wrong place" ' + git init $mode-collision && + ( + cd $mode-collision && + mkdir target-dir && + + empty_obj_hex=$(git hash-object -w --stdin objs <<-EOF && + 100644 blob ${empty_obj_hex} ${dir}/x + 100644 blob ${empty_obj_hex} ${dir}/y + 100644 blob ${empty_obj_hex} ${dir}/z + 120000 blob ${symlink_hex} ${symlink} + EOF + + git update-index --index-info Date: Tue, 2 Feb 2021 22:09:52 +0100 Subject: run-command: invalidate lstat cache after a command finished In the previous commit, we intercepted calls to `rmdir()` to invalidate the lstat cache in the successful case, so that the lstat cache could not have the idea that a directory exists where there is none. The same situation can arise, of course, when a separate process is spawned (most notably, this is the case in `submodule_move_head()`). Obviously, we cannot know whether a directory was removed in that process, therefore we must invalidate the lstat cache afterwards. Note: in contrast to `lstat_cache_aware_rmdir()`, we invalidate the lstat cache even in case of an error: the process might have removed a directory and still have failed afterwards. Co-authored-by: Matheus Tavares Signed-off-by: Johannes Schindelin --- run-command.c | 9 ++++++++- t/t0021-conversion.sh | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/run-command.c b/run-command.c index a483d5904a..c5c4d36671 100644 --- a/run-command.c +++ b/run-command.c @@ -953,6 +953,7 @@ int finish_command(struct child_process *cmd) { int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0); child_process_clear(cmd); + invalidate_lstat_cache(); return ret; } @@ -1239,13 +1240,19 @@ error: int finish_async(struct async *async) { #ifdef NO_PTHREADS - return wait_or_whine(async->pid, "child process", 0); + int ret = wait_or_whine(async->pid, "child process", 0); + + invalidate_lstat_cache(); + + return ret; #else void *ret = (void *)(intptr_t)(-1); if (pthread_join(async->tid, &ret)) error("pthread_join failed"); + invalidate_lstat_cache(); return (int)(intptr_t)ret; + #endif } diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 8ff917fca6..a714d376a3 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -862,4 +862,40 @@ do ' done +test_expect_success PERL,SYMLINKS,CASE_INSENSITIVE_FS \ +"delayed checkout with submodule collision don't write to the wrong place" ' + git init collision-with-submodule && + ( + cd collision-with-submodule && + git config filter.delay.process "\"$TEST_ROOT/rot13-filter.pl\" --always-delay delayed.log clean smudge delay" && + git config filter.delay.required true && + + # We need Git to treat the submodule "a" and the + # leading dir "A" as different paths in the index. + git config --local core.ignoreCase false && + + empty_oid=$(printf "" | git hash-object -w --stdin) && + attr_oid=$(echo "A/B/y filter=delay" | git hash-object -w --stdin) && + cat >objs <<-EOF && + 100644 blob $empty_oid A/B/x + 100644 blob $empty_oid A/B/y + 100644 blob $attr_oid .gitattributes + EOF + git update-index --index-info objs && + git -C a update-index --index-info Date: Tue, 2 Feb 2021 22:37:10 +0100 Subject: unpack_trees(): start with a fresh lstat cache We really want to avoid relying on stale information. Signed-off-by: Matheus Tavares Signed-off-by: Johannes Schindelin --- unpack-trees.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unpack-trees.c b/unpack-trees.c index 88a0b5d250..5914e7b8cc 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -349,6 +349,9 @@ static int check_updates(struct unpack_trees_options *o) progress = get_progress(o); + /* Start with clean cache to avoid using any possibly outdated info. */ + invalidate_lstat_cache(); + if (o->update) git_attr_set_direction(GIT_ATTR_CHECKOUT, index); -- cgit v1.2.3 From 6b82d3eea625d83af067dc0ed57e361711cfb8b7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Jan 2021 19:13:11 +0100 Subject: Git 2.17.6 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.17.6.txt | 16 ++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.17.6.txt diff --git a/Documentation/RelNotes/2.17.6.txt b/Documentation/RelNotes/2.17.6.txt new file mode 100644 index 0000000000..2f181e8064 --- /dev/null +++ b/Documentation/RelNotes/2.17.6.txt @@ -0,0 +1,16 @@ +Git v2.17.6 Release Notes +========================= + +This release addresses the security issues CVE-2021-21300. + +Fixes since v2.17.5 +------------------- + + * CVE-2021-21300: + On case-insensitive file systems with support for symbolic links, + if Git is configured globally to apply delay-capable clean/smudge + filters (such as Git LFS), Git could be fooled into running + remote code during a clone. + +Credit for finding and fixing this vulnerability goes to Matheus +Tavares, helped by Johannes Schindelin. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 85d9db5600..4675585069 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.17.5 +DEF_VER=v2.17.6 LF=' ' diff --git a/RelNotes b/RelNotes index 07012e884f..04bc17f6c5 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.17.5.txt \ No newline at end of file +Documentation/RelNotes/2.17.6.txt \ No newline at end of file -- cgit v1.2.3 From 6eed462c8f973719799691c4f51b97f7970f2ac4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 12 Feb 2021 15:47:43 +0100 Subject: Git 2.18.5 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.18.5.txt | 6 ++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.18.5.txt diff --git a/Documentation/RelNotes/2.18.5.txt b/Documentation/RelNotes/2.18.5.txt new file mode 100644 index 0000000000..dfb1de4ceb --- /dev/null +++ b/Documentation/RelNotes/2.18.5.txt @@ -0,0 +1,6 @@ +Git v2.18.5 Release Notes +========================= + +This release merges up the fixes that appear in v2.17.6 to address +the security issue CVE-2021-21300; see the release notes for that +version for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 4ed8c413e5..d522717b79 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.18.4 +DEF_VER=v2.18.5 LF=' ' diff --git a/RelNotes b/RelNotes index 70822c3866..e66448298a 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.18.4.txt \ No newline at end of file +Documentation/RelNotes/2.18.5.txt \ No newline at end of file -- cgit v1.2.3