diff --git a/backport-CVE-2024-32004-fetch-clone-detect-dubious-ownership-of-local-reposi.patch b/backport-CVE-2024-32004-fetch-clone-detect-dubious-ownership-of-local-reposi.patch new file mode 100644 index 0000000000000000000000000000000000000000..edd07467df6913100a4666731b24a796f76fb891 --- /dev/null +++ b/backport-CVE-2024-32004-fetch-clone-detect-dubious-ownership-of-local-reposi.patch @@ -0,0 +1,153 @@ +From f4aa8c8bb11dae6e769cd930565173808cbb69c8 Mon Sep 17 00:00:00 2001 +From: Johannes Schindelin +Date: Wed, 10 Apr 2024 14:39:37 +0200 +Subject: [PATCH] fetch/clone: detect dubious ownership of local repositories + +When cloning from somebody else's repositories, it is possible that, +say, the `upload-pack` command is overridden in the repository that is +about to be cloned, which would then be run in the user's context who +started the clone. + +To remind the user that this is a potentially unsafe operation, let's +extend the ownership checks we have already established for regular +gitdir discovery to extend also to local repositories that are about to +be cloned. + +This protection extends also to file:// URLs. + +The fixes in this commit address CVE-2024-32004. + +Note: This commit does not touch the `fetch`/`clone` code directly, but +instead the function used implicitly by both: `enter_repo()`. This +function is also used by `git receive-pack` (i.e. pushes), by `git +upload-archive`, by `git daemon` and by `git http-backend`. In setups +that want to serve repositories owned by different users than the +account running the service, this will require `safe.*` settings to be +configured accordingly. + +Also note: there are tiny time windows where a time-of-check-time-of-use +("TOCTOU") race is possible. The real solution to those would be to work +with `fstat()` and `openat()`. However, the latter function is not +available on Windows (and would have to be emulated with rather +expensive low-level `NtCreateFile()` calls), and the changes would be +quite extensive, for my taste too extensive for the little gain given +that embargoed releases need to pay extra attention to avoid introducing +inadvertent bugs. + +Signed-off-by: Johannes Schindelin +--- + cache.h | 12 ++++++++++++ + path.c | 2 ++ + setup.c | 21 +++++++++++++++++++++ + t/t0411-clone-from-partial.sh | 6 +++--- + 4 files changed, 38 insertions(+), 3 deletions(-) + +diff --git a/cache.h b/cache.h +index fcf49706a..a46a3e4b6 100644 +--- a/cache.h ++++ b/cache.h +@@ -606,6 +606,18 @@ void set_git_work_tree(const char *tree); + + #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" + ++/* ++ * Check if a repository is safe and die if it is not, by verifying the ++ * ownership of the worktree (if any), the git directory, and the gitfile (if ++ * any). ++ * ++ * Exemptions for known-safe repositories can be added via `safe.directory` ++ * config settings; for non-bare repositories, their worktree needs to be ++ * added, for bare ones their git directory. ++ */ ++void die_upon_dubious_ownership(const char *gitfile, const char *worktree, ++ const char *gitdir); ++ + void setup_work_tree(void); + /* + * Find the commondir and gitdir of the repository that contains the current +diff --git a/path.c b/path.c +index 492e17ad1..d61f70e87 100644 +--- a/path.c ++++ b/path.c +@@ -840,6 +840,7 @@ const char *enter_repo(const char *path, int strict) + if (!suffix[i]) + return NULL; + gitfile = read_gitfile(used_path.buf); ++ die_upon_dubious_ownership(gitfile, NULL, used_path.buf); + if (gitfile) { + strbuf_reset(&used_path); + strbuf_addstr(&used_path, gitfile); +@@ -850,6 +851,7 @@ const char *enter_repo(const char *path, int strict) + } + else { + const char *gitfile = read_gitfile(path); ++ die_upon_dubious_ownership(gitfile, NULL, path); + if (gitfile) + path = gitfile; + if (chdir(path)) +diff --git a/setup.c b/setup.c +index cefd5f63c..9d401ae4c 100644 +--- a/setup.c ++++ b/setup.c +@@ -1165,6 +1165,27 @@ static int ensure_valid_ownership(const char *gitfile, + return data.is_safe; + } + ++void die_upon_dubious_ownership(const char *gitfile, const char *worktree, ++ const char *gitdir) ++{ ++ struct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT; ++ const char *path; ++ ++ if (ensure_valid_ownership(gitfile, worktree, gitdir, &report)) ++ return; ++ ++ strbuf_complete(&report, '\n'); ++ path = gitfile ? gitfile : gitdir; ++ sq_quote_buf_pretty("ed, path); ++ ++ die(_("detected dubious ownership in repository at '%s'\n" ++ "%s" ++ "To add an exception for this directory, call:\n" ++ "\n" ++ "\tgit config --global --add safe.directory %s"), ++ path, report.buf, quoted.buf); ++} ++ + enum discovery_result { + GIT_DIR_NONE = 0, + GIT_DIR_EXPLICIT, +diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh +index fb72a0a9f..eb3360dbc 100755 +--- a/t/t0411-clone-from-partial.sh ++++ b/t/t0411-clone-from-partial.sh +@@ -23,7 +23,7 @@ test_expect_success 'create evil repo' ' + >evil/.git/shallow + ' + +-test_expect_failure 'local clone must not fetch from promisor remote and execute script' ' ++test_expect_success 'local clone must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ +@@ -32,7 +32,7 @@ test_expect_failure 'local clone must not fetch from promisor remote and execute + test_path_is_missing script-executed + ' + +-test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' ' ++test_expect_success 'clone from file://... must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ +@@ -41,7 +41,7 @@ test_expect_failure 'clone from file://... must not fetch from promisor remote a + test_path_is_missing script-executed + ' + +-test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' ' ++test_expect_success 'fetch from file://... must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git fetch \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ +-- +2.33.0 + diff --git a/backport-CVE-2024-32004-setup-fix-some-formatting.patch b/backport-CVE-2024-32004-setup-fix-some-formatting.patch new file mode 100644 index 0000000000000000000000000000000000000000..cce68311803123ffd37d3f38729590a60997275d --- /dev/null +++ b/backport-CVE-2024-32004-setup-fix-some-formatting.patch @@ -0,0 +1,47 @@ +From d51e1dff980b9fc87002436b6ab36120a39816b1 Mon Sep 17 00:00:00 2001 +From: Johannes Schindelin +Date: Mon, 8 Aug 2022 13:27:46 +0000 +Subject: [PATCH] setup: fix some formatting + +In preparation for touching code that was introduced in 3b0bf2704980 +(setup: tighten ownership checks post CVE-2022-24765, 2022-05-10) and +that was formatted differently than preferred in the Git project, fix +the indentation before actually modifying the code. + +Signed-off-by: Johannes Schindelin +Signed-off-by: Junio C Hamano +--- + setup.c | 9 +++++---- + 1 file changed, 5 insertions(+), 4 deletions(-) + +diff --git a/setup.c b/setup.c +index 7f64f3447..0fdecec32 100644 +--- a/setup.c ++++ b/setup.c +@@ -1138,7 +1138,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d) + * added, for bare ones their git directory. + */ + static int ensure_valid_ownership(const char *gitfile, +- const char *worktree, const char *gitdir) ++ const char *worktree, const char *gitdir) + { + struct safe_directory_data data = { + .path = worktree ? worktree : gitdir +@@ -1271,10 +1271,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, + strbuf_setlen(dir, offset); + if (gitdirenv) { + enum discovery_result ret; ++ const char *gitdir_candidate = ++ gitdir_path ? gitdir_path : gitdirenv; + +- if (ensure_valid_ownership(gitfile, +- dir->buf, +- (gitdir_path ? gitdir_path : gitdirenv))) { ++ if (ensure_valid_ownership(gitfile, dir->buf, ++ gitdir_candidate)) { + strbuf_addstr(gitdir, gitdirenv); + ret = GIT_DIR_DISCOVERED; + } else +-- +2.33.0 + diff --git a/backport-CVE-2024-32004-setup-prepare-for-more-detailed-dubious-ownership-me.patch b/backport-CVE-2024-32004-setup-prepare-for-more-detailed-dubious-ownership-me.patch new file mode 100644 index 0000000000000000000000000000000000000000..a28bd510b07159c7ffc73f3a1153720f89786292 --- /dev/null +++ b/backport-CVE-2024-32004-setup-prepare-for-more-detailed-dubious-ownership-me.patch @@ -0,0 +1,182 @@ +From 17d3883fe9c88b823002ad9fafb42313ddc3d3d5 Mon Sep 17 00:00:00 2001 +From: Johannes Schindelin +Date: Mon, 8 Aug 2022 13:27:47 +0000 +Subject: [PATCH] setup: prepare for more detailed "dubious ownership" messages + +When verifying the ownership of the Git directory, we sometimes would +like to say a bit more about it, e.g. when using a platform-dependent +code path (think: Windows has the permission model that is so different +from Unix'), but only when it is a appropriate to actually say +something. + +To allow for that, collect that information and hand it back to the +caller (whose responsibility it is to show it or not). + +Note: We do not actually fill in any platform-dependent information yet, +this commit just adds the infrastructure to be able to do so. + +Based-on-an-idea-by: Junio C Hamano +Signed-off-by: Johannes Schindelin +Signed-off-by: Junio C Hamano +--- + compat/mingw.c | 2 +- + compat/mingw.h | 2 +- + git-compat-util.h | 5 ++++- + setup.c | 25 +++++++++++++++---------- + 4 files changed, 21 insertions(+), 13 deletions(-) + +diff --git a/compat/mingw.c b/compat/mingw.c +index 2607de93a..f12b7df16 100644 +--- a/compat/mingw.c ++++ b/compat/mingw.c +@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void) + return result; + } + +-int is_path_owned_by_current_sid(const char *path) ++int is_path_owned_by_current_sid(const char *path, struct strbuf *report) + { + WCHAR wpath[MAX_PATH]; + PSID sid = NULL; +diff --git a/compat/mingw.h b/compat/mingw.h +index a74da68f3..209cf7ceb 100644 +--- a/compat/mingw.h ++++ b/compat/mingw.h +@@ -463,7 +463,7 @@ char *mingw_query_user_email(void); + * Verifies that the specified path is owned by the user running the + * current process. + */ +-int is_path_owned_by_current_sid(const char *path); ++int is_path_owned_by_current_sid(const char *path, struct strbuf *report); + #define is_path_owned_by_current_user is_path_owned_by_current_sid + + /** +diff --git a/git-compat-util.h b/git-compat-util.h +index 58d770829..36a25ae25 100644 +--- a/git-compat-util.h ++++ b/git-compat-util.h +@@ -23,6 +23,9 @@ + #include + #endif + ++struct strbuf; ++ ++ + #define _FILE_OFFSET_BITS 64 + + +@@ -487,7 +490,7 @@ static inline void extract_id_from_env(const char *env, uid_t *id) + #endif + + #ifndef is_path_owned_by_current_user +-static inline int is_path_owned_by_current_uid(const char *path) ++static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report) + { + struct stat st; + if (lstat(path, &st)) +diff --git a/setup.c b/setup.c +index 0fdecec32..ddcf6eb60 100644 +--- a/setup.c ++++ b/setup.c +@@ -1138,16 +1138,17 @@ static int safe_directory_cb(const char *key, const char *value, void *d) + * added, for bare ones their git directory. + */ + static int ensure_valid_ownership(const char *gitfile, +- const char *worktree, const char *gitdir) ++ const char *worktree, const char *gitdir, ++ struct strbuf *report) + { + struct safe_directory_data data = { + .path = worktree ? worktree : gitdir + }; + + if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) && +- (!gitfile || is_path_owned_by_current_user(gitfile)) && +- (!worktree || is_path_owned_by_current_user(worktree)) && +- (!gitdir || is_path_owned_by_current_user(gitdir))) ++ (!gitfile || is_path_owned_by_current_user(gitfile, report)) && ++ (!worktree || is_path_owned_by_current_user(worktree, report)) && ++ (!gitdir || is_path_owned_by_current_user(gitdir, report))) + return 1; + + /* +@@ -1187,6 +1188,7 @@ enum discovery_result { + */ + static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, + struct strbuf *gitdir, ++ struct strbuf *report, + int die_on_error) + { + const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT); +@@ -1275,7 +1277,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, + gitdir_path ? gitdir_path : gitdirenv; + + if (ensure_valid_ownership(gitfile, dir->buf, +- gitdir_candidate)) { ++ gitdir_candidate, report)) { + strbuf_addstr(gitdir, gitdirenv); + ret = GIT_DIR_DISCOVERED; + } else +@@ -1298,7 +1300,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, + } + + if (is_git_directory(dir->buf)) { +- if (!ensure_valid_ownership(NULL, NULL, dir->buf)) ++ if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) + return GIT_DIR_INVALID_OWNERSHIP; + strbuf_addstr(gitdir, "."); + return GIT_DIR_BARE; +@@ -1331,7 +1333,7 @@ int discover_git_directory(struct strbuf *commondir, + return -1; + + cwd_len = dir.len; +- if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) { ++ if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) { + strbuf_release(&dir); + return -1; + } +@@ -1378,7 +1380,7 @@ int discover_git_directory(struct strbuf *commondir, + const char *setup_git_directory_gently(int *nongit_ok) + { + static struct strbuf cwd = STRBUF_INIT; +- struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT; ++ struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT; + const char *prefix = NULL; + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; + +@@ -1403,7 +1405,7 @@ const char *setup_git_directory_gently(int *nongit_ok) + die_errno(_("Unable to read current working directory")); + strbuf_addbuf(&dir, &cwd); + +- switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) { ++ switch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) { + case GIT_DIR_EXPLICIT: + prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok); + break; +@@ -1435,12 +1437,14 @@ const char *setup_git_directory_gently(int *nongit_ok) + if (!nongit_ok) { + struct strbuf quoted = STRBUF_INIT; + ++ strbuf_complete(&report, '\n'); + sq_quote_buf_pretty("ed, dir.buf); + die(_("detected dubious ownership in repository at '%s'\n" ++ "%s" + "To add an exception for this directory, call:\n" + "\n" + "\tgit config --global --add safe.directory %s"), +- dir.buf, quoted.buf); ++ dir.buf, report.buf, quoted.buf); + } + *nongit_ok = 1; + break; +@@ -1519,6 +1523,7 @@ const char *setup_git_directory_gently(int *nongit_ok) + + strbuf_release(&dir); + strbuf_release(&gitdir); ++ strbuf_release(&report); + clear_repository_format(&repo_fmt); + + return prefix; +-- +2.33.0 + diff --git a/backport-CVE-2024-32004-t0411-add-tests-for-cloning-from-partial-repo.patch b/backport-CVE-2024-32004-t0411-add-tests-for-cloning-from-partial-repo.patch new file mode 100644 index 0000000000000000000000000000000000000000..905f71d484c90d756b45df1025be0a7c7fe472b8 --- /dev/null +++ b/backport-CVE-2024-32004-t0411-add-tests-for-cloning-from-partial-repo.patch @@ -0,0 +1,90 @@ +From 5c5a4a1c05932378d259b1fdd9526cab971656a2 Mon Sep 17 00:00:00 2001 +From: Filip Hejsek +Date: Sun, 28 Jan 2024 04:29:33 +0100 +Subject: [PATCH] t0411: add tests for cloning from partial repo + +Cloning from a partial repository must not fetch missing objects into +the partial repository, because that can lead to arbitrary code +execution. + +Add a couple of test cases, pretending to the `upload-pack` command (and +to that command only) that it is working on a repository owned by +someone else. + +Helped-by: Jeff King +Signed-off-by: Filip Hejsek +Signed-off-by: Johannes Schindelin +--- + t/t0411-clone-from-partial.sh | 60 +++++++++++++++++++++++++++++++++++ + 1 file changed, 60 insertions(+) + create mode 100755 t/t0411-clone-from-partial.sh + +diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh +new file mode 100755 +index 000000000..fb72a0a9f +--- /dev/null ++++ b/t/t0411-clone-from-partial.sh +@@ -0,0 +1,60 @@ ++#!/bin/sh ++ ++test_description='check that local clone does not fetch from promisor remotes' ++ ++. ./test-lib.sh ++ ++test_expect_success 'create evil repo' ' ++ git init tmp && ++ test_commit -C tmp a && ++ git -C tmp config uploadpack.allowfilter 1 && ++ git clone --filter=blob:none --no-local --no-checkout tmp evil && ++ rm -rf tmp && ++ ++ git -C evil config remote.origin.uploadpack \"\$TRASH_DIRECTORY/fake-upload-pack\" && ++ write_script fake-upload-pack <<-\EOF && ++ echo >&2 "fake-upload-pack running" ++ >"$TRASH_DIRECTORY/script-executed" ++ exit 1 ++ EOF ++ export TRASH_DIRECTORY && ++ ++ # empty shallow file disables local clone optimization ++ >evil/.git/shallow ++' ++ ++test_expect_failure 'local clone must not fetch from promisor remote and execute script' ' ++ rm -f script-executed && ++ test_must_fail git clone \ ++ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ ++ evil clone1 2>err && ++ ! grep "fake-upload-pack running" err && ++ test_path_is_missing script-executed ++' ++ ++test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' ' ++ rm -f script-executed && ++ test_must_fail git clone \ ++ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ ++ "file://$(pwd)/evil" clone2 2>err && ++ ! grep "fake-upload-pack running" err && ++ test_path_is_missing script-executed ++' ++ ++test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' ' ++ rm -f script-executed && ++ test_must_fail git fetch \ ++ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ ++ "file://$(pwd)/evil" 2>err && ++ ! grep "fake-upload-pack running" err && ++ test_path_is_missing script-executed ++' ++ ++test_expect_success 'pack-objects should fetch from promisor remote and execute script' ' ++ rm -f script-executed && ++ echo "HEAD" | test_must_fail git -C evil pack-objects --revs --stdout >/dev/null 2>err && ++ grep "fake-upload-pack running" err && ++ test_path_is_file script-executed ++' ++ ++test_done +-- +2.33.0 + diff --git a/backport-CVE-2024-32020-builtin-clone-refuse-local-clones-of-unsafe-reposito.patch b/backport-CVE-2024-32020-builtin-clone-refuse-local-clones-of-unsafe-reposito.patch new file mode 100644 index 0000000000000000000000000000000000000000..d3a9d9730340742ccd7a0492f6b72d37bd016bfa --- /dev/null +++ b/backport-CVE-2024-32020-builtin-clone-refuse-local-clones-of-unsafe-reposito.patch @@ -0,0 +1,109 @@ +From 1204e1a824c34071019fe106348eaa6d88f9528d Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +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 +Signed-off-by: Johannes Schindelin +--- + 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=*, but is reset' ' + expect_rejected_dir + ' + ++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 + diff --git a/backport-CVE-2024-32021-builtin-clone-abort-when-hardlinked-source-and-targe.patch b/backport-CVE-2024-32021-builtin-clone-abort-when-hardlinked-source-and-targe.patch new file mode 100644 index 0000000000000000000000000000000000000000..2afafc77e526234934533027185f38c1f8483242 --- /dev/null +++ b/backport-CVE-2024-32021-builtin-clone-abort-when-hardlinked-source-and-targe.patch @@ -0,0 +1,60 @@ +From d1bb66a546b4bb46005d17ba711caaad26f26c1e Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Mon, 15 Apr 2024 13:30:31 +0200 +Subject: [PATCH] builtin/clone: abort when hardlinked source and target file + differ + +When performing local clones with hardlinks we refuse to copy source +files which are symlinks as a mitigation for CVE-2022-39253. This check +can be raced by an adversary though by changing the file to a symlink +after we have checked it. + +Fix the issue by checking whether the hardlinked destination file +matches the source file and abort in case it doesn't. + +This addresses CVE-2024-32021. + +Reported-by: Apple Product Security +Suggested-by: Linus Torvalds +Signed-off-by: Patrick Steinhardt +Signed-off-by: Johannes Schindelin +--- + builtin/clone.c | 21 ++++++++++++++++++++- + 1 file changed, 20 insertions(+), 1 deletion(-) + +diff --git a/builtin/clone.c b/builtin/clone.c +index 073e6323d..4b80fa087 100644 +--- a/builtin/clone.c ++++ b/builtin/clone.c +@@ -357,8 +357,27 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, + if (unlink(dest->buf) && errno != ENOENT) + die_errno(_("failed to unlink '%s'"), dest->buf); + if (!option_no_hardlinks) { +- if (!link(src->buf, dest->buf)) ++ if (!link(src->buf, dest->buf)) { ++ struct stat st; ++ ++ /* ++ * Sanity-check whether the created hardlink ++ * actually links to the expected file now. This ++ * catches time-of-check-time-of-use bugs in ++ * case the source file was meanwhile swapped. ++ */ ++ if (lstat(dest->buf, &st)) ++ die(_("hardlink cannot be checked at '%s'"), dest->buf); ++ if (st.st_mode != iter->st.st_mode || ++ st.st_ino != iter->st.st_ino || ++ st.st_dev != iter->st.st_dev || ++ st.st_size != iter->st.st_size || ++ st.st_uid != iter->st.st_uid || ++ st.st_gid != iter->st.st_gid) ++ die(_("hardlink different from source at '%s'"), dest->buf); ++ + continue; ++ } + if (option_local > 0) + die_errno(_("failed to create link '%s'"), dest->buf); + option_no_hardlinks = 1; +-- +2.33.0 + diff --git a/backport-CVE-2024-32021-builtin-clone-stop-resolving-symlinks-when-copying-f.patch b/backport-CVE-2024-32021-builtin-clone-stop-resolving-symlinks-when-copying-f.patch new file mode 100644 index 0000000000000000000000000000000000000000..fb0fc5e6c3b52a13a0ab63cfdacea911085a9486 --- /dev/null +++ b/backport-CVE-2024-32021-builtin-clone-stop-resolving-symlinks-when-copying-f.patch @@ -0,0 +1,84 @@ +From 150e6b0aedf57d224c3c49038c306477fa159886 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Mon, 15 Apr 2024 13:30:26 +0200 +Subject: [PATCH] builtin/clone: stop resolving symlinks when copying files + +When a user performs a local clone without `--no-local`, then we end up +copying the source repository into the target repository directly. To +optimize this even further, we try to hardlink files into place instead +of copying data over, which helps both disk usage and speed. + +There is an important edge case in this context though, namely when we +try to hardlink symlinks from the source repository into the target +repository. Depending on both platform and filesystem the resulting +behaviour here can be different: + + - On macOS and NetBSD, calling link(3P) with a symlink target creates + a hardlink to the file pointed to by the symlink. + + - On Linux, calling link(3P) instead creates a hardlink to the symlink + itself. + +To unify this behaviour, 36596fd2df (clone: better handle symlinked +files at .git/objects/, 2019-07-10) introduced logic to resolve symlinks +before we try to link(3P) files. Consequently, the new behaviour was to +always create a hard link to the target of the symlink on all platforms. + +Eventually though, we figured out that following symlinks like this can +cause havoc when performing a local clone of a malicious repository, +which resulted in CVE-2022-39253. This issue was fixed via 6f054f9fb3 +(builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28), +by refusing symlinks in the source repository. + +But even though we now shouldn't ever link symlinks anymore, the code +that resolves symlinks still exists. In the best case the code does not +end up doing anything because there are no symlinks anymore. In the +worst case though this can be abused by an adversary that rewrites the +source file after it has been checked not to be a symlink such that it +actually is a symlink when we call link(3P). Thus, it is still possible +to recreate CVE-2022-39253 due to this time-of-check-time-of-use bug. + +Remove the call to `realpath()`. This doesn't yet address the actual +vulnerability, which will be handled in a subsequent commit. + +Reported-by: Apple Product Security +Signed-off-by: Patrick Steinhardt +Signed-off-by: Johannes Schindelin +--- + builtin/clone.c | 6 +----- + 1 file changed, 1 insertion(+), 5 deletions(-) + +diff --git a/builtin/clone.c b/builtin/clone.c +index 3c2ae31a5..073e6323d 100644 +--- a/builtin/clone.c ++++ b/builtin/clone.c +@@ -320,7 +320,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, + int src_len, dest_len; + struct dir_iterator *iter; + int iter_status; +- struct strbuf realpath = STRBUF_INIT; + + mkdir_if_missing(dest->buf, 0777); + +@@ -358,8 +357,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, + if (unlink(dest->buf) && errno != ENOENT) + die_errno(_("failed to unlink '%s'"), dest->buf); + if (!option_no_hardlinks) { +- strbuf_realpath(&realpath, src->buf, 1); +- if (!link(realpath.buf, dest->buf)) ++ if (!link(src->buf, dest->buf)) + continue; + if (option_local > 0) + die_errno(_("failed to create link '%s'"), dest->buf); +@@ -373,8 +371,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, + strbuf_setlen(src, src_len); + die(_("failed to iterate over '%s'"), src->buf); + } +- +- strbuf_release(&realpath); + } + + static void clone_local(const char *src_repo, const char *dest_repo) +-- +2.33.0 + diff --git a/backport-CVE-2024-32465-upload-pack-disable-lazy-fetching-by-default.patch b/backport-CVE-2024-32465-upload-pack-disable-lazy-fetching-by-default.patch new file mode 100644 index 0000000000000000000000000000000000000000..3bb92e3b0eaeeee34c7376db097209f39be93dad --- /dev/null +++ b/backport-CVE-2024-32465-upload-pack-disable-lazy-fetching-by-default.patch @@ -0,0 +1,201 @@ +From 7b70e9efb18c2cc3f219af399bd384c5801ba1d7 Mon Sep 17 00:00:00 2001 +From: Jeff King +Date: Tue, 16 Apr 2024 04:35:33 -0400 +Subject: [PATCH] upload-pack: disable lazy-fetching by default + +The upload-pack command tries to avoid trusting the repository in which +it's run (e.g., by not running any hooks and not using any config that +contains arbitrary commands). But if the server side of a fetch or a +clone is a partial clone, then either upload-pack or its child +pack-objects may run a lazy "git fetch" under the hood. And it is very +easy to convince fetch to run arbitrary commands. + +The "server" side can be a local repository owned by someone else, who +would be able to configure commands that are run during a clone with the +current user's permissions. This issue has been designated +CVE-2024-32004. + +The fix in this commit's parent helps in this scenario, as well as in +related scenarios using SSH to clone, where the untrusted .git directory +is owned by a different user id. But if you received one as a zip file, +on a USB stick, etc, it may be owned by your user but still untrusted. + +This has been designated CVE-2024-32465. + +To mitigate the issue more completely, let's disable lazy fetching +entirely during `upload-pack`. While fetching from a partial repository +should be relatively rare, it is certainly not an unreasonable workflow. +And thus we need to provide an escape hatch. + +This commit works by respecting a GIT_NO_LAZY_FETCH environment variable +(to skip the lazy-fetch), and setting it in upload-pack, but only when +the user has not already done so (which gives us the escape hatch). + +The name of the variable is specifically chosen to match what has +already been added in 'master' via e6d5479e7a (git: extend +--no-lazy-fetch to work across subprocesses, 2024-02-27). Since we're +building this fix as a backport for older versions, we could cherry-pick +that patch and its earlier steps. However, we don't really need the +niceties (like a "--no-lazy-fetch" option) that it offers. By using the +same name, everything should just work when the two are eventually +merged, but here are a few notes: + + - the blocking of the fetch in e6d5479e7a is incomplete! It sets + fetch_if_missing to 0 when we setup the repository variable, but + that isn't enough. pack-objects in particular will call + prefetch_to_pack() even if that variable is 0. This patch by + contrast checks the environment variable at the lowest level before + we call the lazy fetch, where we can be sure to catch all code + paths. + + Possibly the setting of fetch_if_missing from e6d5479e7a can be + reverted, but it may be useful to have. For example, some code may + want to use that flag to change behavior before it gets to the point + of trying to start the fetch. At any rate, that's all outside the + scope of this patch. + + - there's documentation for GIT_NO_LAZY_FETCH in e6d5479e7a. We can + live without that here, because for the most part the user shouldn't + need to set it themselves. The exception is if they do want to + override upload-pack's default, and that requires a separate + documentation section (which is added here) + + - it would be nice to use the NO_LAZY_FETCH_ENVIRONMENT macro added by + e6d5479e7a, but those definitions have moved from cache.h to + environment.h between 2.39.3 and master. I just used the raw string + literals, and we can replace them with the macro once this topic is + merged to master. + +At least with respect to CVE-2024-32004, this does render this commit's +parent commit somewhat redundant. However, it is worth retaining that +commit as defense in depth, and because it may help other issues (e.g., +symlink/hardlink TOCTOU races, where zip files are not really an +interesting attack vector). + +The tests in t0411 still pass, but now we have _two_ mechanisms ensuring +that the evil command is not run. Let's beef up the existing ones to +check that they failed for the expected reason, that we refused to run +upload-pack at all with an alternate user id. And add two new ones for +the same-user case that both the restriction and its escape hatch. + +Signed-off-by: Jeff King +Signed-off-by: Johannes Schindelin +--- + Documentation/git-upload-pack.txt | 16 ++++++++++++++++ + builtin/upload-pack.c | 2 ++ + promisor-remote.c | 10 ++++++++++ + t/t0411-clone-from-partial.sh | 18 ++++++++++++++++++ + 4 files changed, 46 insertions(+) + +diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt +index b656b4756..fc4c62d7b 100644 +--- a/Documentation/git-upload-pack.txt ++++ b/Documentation/git-upload-pack.txt +@@ -55,6 +55,22 @@ --advertise-refs:: + :: + The repository to sync from. + ++`GIT_NO_LAZY_FETCH`:: ++ When cloning or fetching from a partial repository (i.e., one ++ itself cloned with `--filter`), the server-side `upload-pack` ++ may need to fetch extra objects from its upstream in order to ++ complete the request. By default, `upload-pack` will refuse to ++ perform such a lazy fetch, because `git fetch` may run arbitrary ++ commands specified in configuration and hooks of the source ++ repository (and `upload-pack` tries to be safe to run even in ++ untrusted `.git` directories). +++ ++This is implemented by having `upload-pack` internally set the ++`GIT_NO_LAZY_FETCH` variable to `1`. If you want to override it ++(because you are fetching from a partial clone, and you are sure ++you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to ++`0`. ++ + SEE ALSO + -------- + linkgit:gitnamespaces[7] +diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c +index 25b69da2b..f446ff04f 100644 +--- a/builtin/upload-pack.c ++++ b/builtin/upload-pack.c +@@ -35,6 +35,8 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix) + + packet_trace_identity("upload-pack"); + read_replace_refs = 0; ++ /* TODO: This should use NO_LAZY_FETCH_ENVIRONMENT */ ++ xsetenv("GIT_NO_LAZY_FETCH", "1", 0); + + argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0); + +diff --git a/promisor-remote.c b/promisor-remote.c +index faa761294..550a38f75 100644 +--- a/promisor-remote.c ++++ b/promisor-remote.c +@@ -20,6 +20,16 @@ static int fetch_objects(struct repository *repo, + int i; + FILE *child_in; + ++ /* TODO: This should use NO_LAZY_FETCH_ENVIRONMENT */ ++ if (git_env_bool("GIT_NO_LAZY_FETCH", 0)) { ++ static int warning_shown; ++ if (!warning_shown) { ++ warning_shown = 1; ++ warning(_("lazy fetching disabled; some objects may not be available")); ++ } ++ return -1; ++ } ++ + child.git_cmd = 1; + child.in = -1; + if (repo != the_repository) +diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh +index eb3360dbc..b3d6ddc4b 100755 +--- a/t/t0411-clone-from-partial.sh ++++ b/t/t0411-clone-from-partial.sh +@@ -28,6 +28,7 @@ test_expect_success 'local clone must not fetch from promisor remote and execute + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + evil clone1 2>err && ++ grep "detected dubious ownership" err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed + ' +@@ -37,6 +38,7 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + "file://$(pwd)/evil" clone2 2>err && ++ grep "detected dubious ownership" err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed + ' +@@ -46,6 +48,7 @@ test_expect_success 'fetch from file://... must not fetch from promisor remote a + test_must_fail git fetch \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + "file://$(pwd)/evil" 2>err && ++ grep "detected dubious ownership" err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed + ' +@@ -57,4 +60,19 @@ test_expect_success 'pack-objects should fetch from promisor remote and execute + test_path_is_file script-executed + ' + ++test_expect_success 'clone from promisor remote does not lazy-fetch by default' ' ++ rm -f script-executed && ++ test_must_fail git clone evil no-lazy 2>err && ++ grep "lazy fetching disabled" err && ++ test_path_is_missing script-executed ++' ++ ++test_expect_success 'promisor lazy-fetching can be re-enabled' ' ++ rm -f script-executed && ++ test_must_fail env GIT_NO_LAZY_FETCH=0 \ ++ git clone evil lazy-ok 2>err && ++ grep "fake-upload-pack running" err && ++ test_path_is_file script-executed ++' ++ + test_done +-- +2.33.0 + diff --git a/backport-CVE-2024-32465-wrapper.c-add-x-un-setenv-and-use-xsetenv-in.patch b/backport-CVE-2024-32465-wrapper.c-add-x-un-setenv-and-use-xsetenv-in.patch new file mode 100644 index 0000000000000000000000000000000000000000..d615b149006e3b16152fa7c3dc533dd8bce693ed --- /dev/null +++ b/backport-CVE-2024-32465-wrapper.c-add-x-un-setenv-and-use-xsetenv-in.patch @@ -0,0 +1,108 @@ +From 3540c71ea5ddffff6e473249866cbc7abb8ce509 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= + +Date: Tue, 21 Sep 2021 15:12:59 +0200 +Subject: [PATCH] wrapper.c: add x{un,}setenv(), and use xsetenv() in + environment.c +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add +set_git_dir() function, 2007-08-01) we started checking its return +value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails, +2018-03-30) we've had set_git_dir_1() die if we couldn't set it. + +Let's provide a wrapper for both, this will be useful in many other +places, a subsequent patch will make another use of xsetenv(). + +The checking of the return value here is over-eager according to +setenv(3) and POSIX. It's documented as returning just -1 or 0, so +perhaps we should be checking -1 explicitly. + +Let's just instead die on any non-zero, if our C library is so broken +as to return something else than -1 on error (and perhaps not set +errno?) the worst we'll do is die with a nonsensical errno value, but +we'll want to die in either case. + +Let's make these return "void" instead of "int". As far as I can tell +there's no other x*() wrappers that needed to make the decision of +deviating from the signature in the C library, but since their return +value is only used to indicate errors (so we'd die here), we can catch +unreachable code such as + + if (xsetenv(...) < 0) + [...]; + +I think it would be OK skip the NULL check of the "name" here for the +calls to die_errno(). Almost all of our setenv() callers are taking a +constant string hardcoded in the source as the first argument, and for +the rest we can probably assume they've done the NULL check +themselves. Even if they didn't, modern C libraries are forgiving +about it (e.g. glibc formatting it as "(null)"), on those that aren't, +well, we were about to die anyway. But let's include the check anyway +for good measure. + +1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html + +Signed-off-by: Ævar Arnfjörð Bjarmason +Signed-off-by: Junio C Hamano +--- + environment.c | 3 +-- + git-compat-util.h | 2 ++ + wrapper.c | 12 ++++++++++++ + 3 files changed, 15 insertions(+), 2 deletions(-) + +diff --git a/environment.c b/environment.c +index d6b22ede7ea288..7d8a949285c101 100644 +--- a/environment.c ++++ b/environment.c +@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r) + + static void set_git_dir_1(const char *path) + { +- if (setenv(GIT_DIR_ENVIRONMENT, path, 1)) +- die(_("could not set GIT_DIR to '%s'"), path); ++ xsetenv(GIT_DIR_ENVIRONMENT, path, 1); + setup_git_env(path); + } + +diff --git a/git-compat-util.h b/git-compat-util.h +index b46605300abf81..b3ee81602c28e0 100644 +--- a/git-compat-util.h ++++ b/git-compat-util.h +@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len); + char *xstrndup(const char *str, size_t len); + void *xrealloc(void *ptr, size_t size); + void *xcalloc(size_t nmemb, size_t size); ++void xsetenv(const char *name, const char *value, int overwrite); ++void xunsetenv(const char *name); + void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset); + const char *mmap_os_err(void); + void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset); +diff --git a/wrapper.c b/wrapper.c +index 7c6586af321000..1460d4e27b03cc 100644 +--- a/wrapper.c ++++ b/wrapper.c +@@ -145,6 +145,18 @@ void *xcalloc(size_t nmemb, size_t size) + return ret; + } + ++void xsetenv(const char *name, const char *value, int overwrite) ++{ ++ if (setenv(name, value, overwrite)) ++ die_errno(_("could not setenv '%s'"), name ? name : "(null)"); ++} ++ ++void xunsetenv(const char *name) ++{ ++ if (!unsetenv(name)) ++ die_errno(_("could not unsetenv '%s'"), name ? name : "(null)"); ++} ++ + /* + * Limit size of IO chunks, because huge chunks only cause pain. OS X + * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in +-- +2.33.0 + diff --git a/git.spec b/git.spec index 82cb85aa5ca87747d4d67f44d8a327a962cb061b..d59f88479a76a4a832e3b832993de3d6a7391f3b 100644 --- a/git.spec +++ b/git.spec @@ -1,7 +1,7 @@ %global gitexecdir %{_libexecdir}/git-core Name: git Version: 2.33.0 -Release: 13 +Release: 14 Summary: A popular and widely used Version Control System License: GPLv2+ or LGPLv2.1 URL: https://git-scm.com/ @@ -59,6 +59,15 @@ Patch43: backport-CVE-2023-23946-apply-fix-writing-behind-newly-created-symbo Patch44: backport-CVE-2023-25652-apply-reject-overwrite-existing-.rej-symlink-if-it-e.patch Patch45: backport-CVE-2023-29007.patch Patch46: backport-CVE-2023-25815-gettext-avoid-using-gettext-if-the-locale-dir-is-not.patch +Patch47: backport-CVE-2024-32021-builtin-clone-stop-resolving-symlinks-when-copying-f.patch +Patch48: backport-CVE-2024-32021-builtin-clone-abort-when-hardlinked-source-and-targe.patch +Patch49: backport-CVE-2024-32004-t0411-add-tests-for-cloning-from-partial-repo.patch +Patch50: backport-CVE-2024-32004-setup-fix-some-formatting.patch +Patch51: backport-CVE-2024-32004-setup-prepare-for-more-detailed-dubious-ownership-me.patch +Patch52: backport-CVE-2024-32004-fetch-clone-detect-dubious-ownership-of-local-reposi.patch +Patch53: backport-CVE-2024-32020-builtin-clone-refuse-local-clones-of-unsafe-reposito.patch +Patch54: backport-CVE-2024-32465-wrapper.c-add-x-un-setenv-and-use-xsetenv-in.patch +Patch55: backport-CVE-2024-32465-upload-pack-disable-lazy-fetching-by-default.patch BuildRequires: gcc gettext BuildRequires: openssl-devel libcurl-devel expat-devel systemd asciidoc xmlto glib2-devel libsecret-devel pcre2-devel desktop-file-utils @@ -342,6 +351,12 @@ make %{?_smp_mflags} test %{_mandir}/man7/git*.7.* %changelog +* Thu May 16 2024 fuanan - 2.33.0-14 +- Type:CVE +- ID:CVE-2024-32021 CVE-2024-32004 CVE-2024-32020 CVE-2024-32465 +- SUG:NA +- DESC:Fix CVE-2024-32021 CVE-2024-32004 CVE-2024-32020 CVE-2024-32465 + * Fri Jul 7 2023 huyubiao - 2.33.0-13 - Type:bugfix - ID:NA