110 lines
4.4 KiB
Diff
110 lines
4.4 KiB
Diff
From 1204e1a824c34071019fe106348eaa6d88f9528d Mon Sep 17 00:00:00 2001
|
|
From: Patrick Steinhardt <ps@pks.im>
|
|
Date: Mon, 15 Apr 2024 13:30:41 +0200
|
|
Subject: [PATCH] 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>
|
|
---
|
|
builtin/clone.c | 14 ++++++++++++++
|
|
t/t0033-safe-directory.sh | 24 ++++++++++++++++++++++++
|
|
2 files changed, 38 insertions(+)
|
|
|
|
diff --git a/builtin/clone.c b/builtin/clone.c
|
|
index 4b80fa087..9ec500d42 100644
|
|
--- a/builtin/clone.c
|
|
+++ b/builtin/clone.c
|
|
@@ -321,6 +321,20 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
|
|
struct dir_iterator *iter;
|
|
int iter_status;
|
|
|
|
+ /*
|
|
+ * Refuse copying directories by default which aren't owned by us. The
|
|
+ * code that performs either the copying or hardlinking is not prepared
|
|
+ * to handle various edge cases where an adversary may for example
|
|
+ * racily swap out files for symlinks. This can cause us to
|
|
+ * inadvertently use the wrong source file.
|
|
+ *
|
|
+ * Furthermore, even if we were prepared to handle such races safely,
|
|
+ * creating hardlinks across user boundaries is an inherently unsafe
|
|
+ * operation as the hardlinked files can be rewritten at will by the
|
|
+ * potentially-untrusted user. We thus refuse to do so by default.
|
|
+ */
|
|
+ die_upon_dubious_ownership(NULL, NULL, src_repo);
|
|
+
|
|
mkdir_if_missing(dest->buf, 0777);
|
|
|
|
iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);
|
|
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
|
|
index dc3496897..11c3e8f28 100755
|
|
--- a/t/t0033-safe-directory.sh
|
|
+++ b/t/t0033-safe-directory.sh
|
|
@@ -80,4 +80,28 @@ test_expect_success 'safe.directory in included file' '
|
|
git status
|
|
'
|
|
|
|
+test_expect_success 'local clone of unowned repo refused in unsafe directory' '
|
|
+ test_when_finished "rm -rf source" &&
|
|
+ git init source &&
|
|
+ (
|
|
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
|
|
+ test_commit -C source initial
|
|
+ ) &&
|
|
+ test_must_fail git clone --local source target &&
|
|
+ test_path_is_missing target
|
|
+'
|
|
+
|
|
+test_expect_success 'local clone of unowned repo accepted in safe directory' '
|
|
+ test_when_finished "rm -rf source" &&
|
|
+ git init source &&
|
|
+ (
|
|
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
|
|
+ test_commit -C source initial
|
|
+ ) &&
|
|
+ test_must_fail git clone --local source target &&
|
|
+ git config --global --add safe.directory "$(pwd)/source/.git" &&
|
|
+ git clone --local source target &&
|
|
+ test_path_is_dir target
|
|
+'
|
|
+
|
|
test_done
|
|
--
|
|
2.33.0
|
|
|