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>

diff --git a/entry.c b/entry.c
index a4c18c56455574b8fac46ef49fb968d4dedeac89..1d78e541689c8032f5d1c00e16b43015eedce4f7 100644 (file)
--- a/entry.c
+++ b/entry.c
@@ -541,6 +541,20 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,
                        /* If it is a gitlink, leave it alone! */
                        if (S_ISGITLINK(ce->ce_mode))
                                return 0;
+                       /*
+                        * We must avoid replacing submodules' leading
+                        * directories with symbolic links, lest recursive
+                        * clones can write into arbitrary locations.
+                        *
+                        * Technically, this logic is not limited
+                        * to recursive clones, or for that matter to
+                        * submodules' paths colliding with symbolic links'
+                        * paths. Yet it strikes a balance in favor of
+                        * simplicity, and if paths are colliding, we might
+                        * just as well keep the directories during a clone.
+                        */
+                       if (state->clone && S_ISLNK(ce->ce_mode))
+                               return 0;
                        remove_subtree(&path);
                } else if (unlink(path.buf))
                        return error_errno("unable to unlink old '%s'", path.buf);

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index b2524a24c29796960bfd4dc59ec9a9650e53175e..fd02984330742322b45ab0eb20d740c443e62da5 100755 (executable)
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -633,6 +633,21 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '
        test_i18ngrep "the following paths have collided" icasefs/warning
 '
 
+test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \
+               'colliding symlink/directory keeps directory' '
+       git init icasefs-colliding-symlink &&
+       (
+               cd icasefs-colliding-symlink &&
+               a=$(printf a | git hash-object -w --stdin) &&
+               printf "100644 %s 0\tA/dir/b\n120000 %s 0\ta\n" $a $a >idx &&
+               git update-index --index-info <idx &&
+               test_tick &&
+               git commit -m initial
+       ) &&
+       git clone icasefs-colliding-symlink icasefs-colliding-symlink-clone &&
+       test_file_not_empty icasefs-colliding-symlink-clone/A/dir/b
+'
+
 test_expect_success 'clone with GIT_DEFAULT_HASH' '
        (
                sane_unset GIT_DEFAULT_HASH &&

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 63c24f7f7ca860177c4f5d0c4836ade8368f3407..dae87090e02ea57352c7f575a069e1bc402c9851 100755 (executable)
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1222,8 +1222,8 @@ test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \
        ) &&
 
        test_path_is_missing "$tell_tale_path" &&
-       test_must_fail git clone --recursive captain hooked 2>err &&
-       grep "directory not empty" err &&
+       git clone --recursive captain hooked 2>err &&
+       ! grep HOOK-RUN err &&
        test_path_is_missing "$tell_tale_path"
 '