@matheusbsbs / matheus-bagliano / commits / 31572dc420

clone: when symbolic links collide with directories, keep the latter

When recursively cloning a repository with submodules, we must ensure that the submodules paths do not suddenly contain symbolic links that would let Git write into unintended locations. We just plugged that vulnerability, but let's add some more defense-in-depth. Since we can only keep one item on disk if multiple index entries' paths collide, we may just as well avoid keeping a symbolic link (because that would allow attack vectors where Git follows those links by mistake). Technically, we handle more situations than cloning submodules into paths that were (partially) replaced by symbolic links. This provides defense-in-depth in case someone finds a case-folding confusion vulnerability in the future that does not even involve submodules. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Johannes Schindelin committed Mar 28, 2024 at 10:55 UTC 31572dc420afee36db8fbbbe060dd78c9a48778c
3 files changed +31 -2
entry.c
+14
@@ -541,6 +541,20 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,
541 /* If it is a gitlink, leave it alone! */
542 if (S_ISGITLINK(ce->ce_mode))
543 return 0;
544 + /*
545 + * We must avoid replacing submodules' leading
546 + * directories with symbolic links, lest recursive
547 + * clones can write into arbitrary locations.
548 + *
549 + * Technically, this logic is not limited
550 + * to recursive clones, or for that matter to
551 + * submodules' paths colliding with symbolic links'
552 + * paths. Yet it strikes a balance in favor of
553 + * simplicity, and if paths are colliding, we might
554 + * just as well keep the directories during a clone.
555 + */
556 + if (state->clone && S_ISLNK(ce->ce_mode))
557 + return 0;
558 remove_subtree(&path);
559 } else if (unlink(path.buf))
560 return error_errno("unable to unlink old '%s'", path.buf);
t/t5601-clone.sh
+15
@@ -633,6 +633,21 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '
633 test_i18ngrep "the following paths have collided" icasefs/warning
634 '
635
636 +test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \
637 + 'colliding symlink/directory keeps directory' '
638 + git init icasefs-colliding-symlink &&
639 + (
640 + cd icasefs-colliding-symlink &&
641 + a=$(printf a | git hash-object -w --stdin) &&
642 + printf "100644 %s 0\tA/dir/b\n120000 %s 0\ta\n" $a $a >idx &&
643 + git update-index --index-info <idx &&
644 + test_tick &&
645 + git commit -m initial
646 + ) &&
647 + git clone icasefs-colliding-symlink icasefs-colliding-symlink-clone &&
648 + test_file_not_empty icasefs-colliding-symlink-clone/A/dir/b
649 +'
650 +
651 test_expect_success 'clone with GIT_DEFAULT_HASH' '
652 (
653 sane_unset GIT_DEFAULT_HASH &&
t/t7406-submodule-update.sh
+2 -2
@@ -1222,8 +1222,8 @@ test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \
1222 ) &&
1223
1224 test_path_is_missing "$tell_tale_path" &&
1225 - test_must_fail git clone --recursive captain hooked 2>err &&
1226 - grep "directory not empty" err &&
1225 + git clone --recursive captain hooked 2>err &&
1226 + ! grep HOOK-RUN err &&
1227 test_path_is_missing "$tell_tale_path"
1228 '
1229