CVE-2024-32004: Insufficient Information (NVD-CWE-noinfo)
value: Git is a revision control system. Prior to versions 2.45.1, 2.44.1, 2.43.4, 2.42.2, 2.41.1, 2.40.2, and 2.39.4, an attacker can prepare a local repository in such a way that, when cloned, will execute arbitrary code during the operation. The problem has been patched in versions 2.45.1, 2.44.1, 2.43.4, 2.42.2, 2.41.1, 2.40.2, and 2.39.4. As a workaround, avoid cloning repositories from untrusted sources.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-05-14T19:15Z
Repository: https://github.com/git/git
Language: C
Files changed: cache.h, path.c, setup.c, t/t0411-clone-from-partial.sh
Commit date: 2024-04-17 22:29:54 +0200
Commit message: 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 <johannes.schindelin@gmx.de>

Diff: diff --git a/cache.h b/cache.h
index fcf49706ad5..a46a3e4b6b1 100644
--- a/cache.h
+++ b/cache.h
@@ -604,10 +604,22 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 
 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
  * working directory, without changing the working directory or other global
  * state. The result is appended to commondir and gitdir.  If the discovered
diff --git a/path.c b/path.c
index 492e17ad121..d61f70e87d4 100644
--- a/path.c
+++ b/path.c
@@ -838,20 +838,22 @@ const char *enter_repo(const char *path, int strict)
 			strbuf_setlen(&used_path, baselen);
 		}
 		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);
 		}
 		if (chdir(used_path.buf))
 			return NULL;
 		path = validated_path.buf;
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
+		die_upon_dubious_ownership(gitfile, NULL, path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
 			return NULL;
 	}
diff --git a/setup.c b/setup.c
index cefd5f63c46..9d401ae4c8f 100644
--- a/setup.c
+++ b/setup.c
@@ -1163,10 +1163,31 @@ static int ensure_valid_ownership(const char *gitfile,
 	git_protected_config(safe_directory_cb, &data);
 
 	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(&quoted, 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);
+}
+
 static int allowed_bare_repo_cb(const char *key, const char *value, void *d)
 {
 	enum allowed_bare_repo *allowed_bare_repo = d;
 
 	if (strcasecmp(key, "safe.bareRepository"))
diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh
index fb72a0a9fff..eb3360dbca0 100755
--- a/t/t0411-clone-from-partial.sh
+++ b/t/t0411-clone-from-partial.sh
@@ -21,29 +21,29 @@ test_expect_success 'create evil repo' '
 
 	# empty shallow file disables local clone optimization
 	>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" \
 		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' '
+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" \
 		"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' '
+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" \
 		"file://$(pwd)/evil" 2>err &&
 	! grep "fake-upload-pack running" err &&

Vulnerable code: test_expect_failure 'local clone must not fetch from promisor remote and execute script' '
test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' '
test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' '
Fixed code: /*
 * 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);

		die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
		die_upon_dubious_ownership(gitfile, NULL, path);
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(&quoted, 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);
}

test_expect_success 'local clone must not fetch from promisor remote and execute script' '
test_expect_success 'clone 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' '