@matheusbsbs / matheus-bagliano / commits / 1204e1a824

builtin/clone: refuse local clones of unsafe repositories

When performing a local clone of a repository we end up either copying or hardlinking the source repository into the target repository. This is significantly more performant than if we were to use git-upload-pack(1) and git-fetch-pack(1) to create the new repository and preserves both disk space and compute time. Unfortunately though, performing such a local clone of a repository that is not owned by the current user is inherently unsafe: - It is possible that source files get swapped out underneath us while we are copying or hardlinking them. While we do perform some checks here to assert that we hardlinked the expected file, they cannot reliably thwart time-of-check-time-of-use (TOCTOU) style races. It is thus possible for an adversary to make us copy or hardlink unexpected files into the target directory. Ideally, we would address this by starting to use openat(3P), fstatat(3P) and friends. Due to platform compatibility with Windows we cannot easily do that though. Furthermore, the scope of these fixes would likely be quite broad and thus not fit for an embargoed security release. - Even if we handled TOCTOU-style races perfectly, hardlinking files owned by a different user into the target repository is not a good idea in general. It is possible for an adversary to rewrite those files to contain whatever data they want even after the clone has completed. Address these issues by completely refusing local clones of a repository that is not owned by the current user. This reuses our existing infra we have in place via `ensure_valid_ownership()` and thus allows a user to override the safety guard by adding the source repository path to the "safe.directory" configuration. This addresses CVE-2024-32020. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Patrick Steinhardt committed Apr 15, 2024 at 13:30 UTC 1204e1a824c34071019fe106348eaa6d88f9528d
2 files changed +38
builtin/clone.c
+14
@@ -321,6 +321,20 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
321 struct dir_iterator *iter;
322 int iter_status;
323
324 + /*
325 + * Refuse copying directories by default which aren't owned by us. The
326 + * code that performs either the copying or hardlinking is not prepared
327 + * to handle various edge cases where an adversary may for example
328 + * racily swap out files for symlinks. This can cause us to
329 + * inadvertently use the wrong source file.
330 + *
331 + * Furthermore, even if we were prepared to handle such races safely,
332 + * creating hardlinks across user boundaries is an inherently unsafe
333 + * operation as the hardlinked files can be rewritten at will by the
334 + * potentially-untrusted user. We thus refuse to do so by default.
335 + */
336 + die_upon_dubious_ownership(NULL, NULL, src_repo);
337 +
338 mkdir_if_missing(dest->buf, 0777);
339
340 iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);
t/t0033-safe-directory.sh
+24
@@ -80,4 +80,28 @@ test_expect_success 'safe.directory in included file' '
80 git status
81 '
82
83 +test_expect_success 'local clone of unowned repo refused in unsafe directory' '
84 + test_when_finished "rm -rf source" &&
85 + git init source &&
86 + (
87 + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
88 + test_commit -C source initial
89 + ) &&
90 + test_must_fail git clone --local source target &&
91 + test_path_is_missing target
92 +'
93 +
94 +test_expect_success 'local clone of unowned repo accepted in safe directory' '
95 + test_when_finished "rm -rf source" &&
96 + git init source &&
97 + (
98 + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
99 + test_commit -C source initial
100 + ) &&
101 + test_must_fail git clone --local source target &&
102 + git config --global --add safe.directory "$(pwd)/source/.git" &&
103 + git clone --local source target &&
104 + test_path_is_dir target
105 +'
106 +
107 test_done