Re: Git clone reads safe.directory differently?

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2024, 22:08 UTC
Message-ID: <xmqq1q391afc.fsf@gitster.g>
In-Reply-To: <xmqqo76d7coa.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> So we probably need to add another axis to the "strict" parameter
> enter_repo() takes to selectively disable the ownership checks only
> for upload-pack, or something like that.

So, here is a rough sketch for the above. Interested parties may build on top of it, perhaps by adding separate knobs to loosen or tighten the second parameter given to enter_repo() at different callsites, by writing tests to make sure they work as intended, and by documenting the security story around it none of which I do here ;-).

The two bits are

 - ENTER_REPO_STRICT: callers that require exact paths (as opposed
   to allowing known suffixes like ".git", ".git/.git" to be
   omitted) can set this bit.  Corresponds to the "strict" parameter
   that the flags wordreplaces.

 - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without
   ownership check can set this bit.

The former is --strict-paths option of "git daemon". The latter is set only by upload-pack, but you may want to add configuration knobs in protected configuration files to loosen it per callsites.

 builtin/upload-pack.c |  5 ++++-
 daemon.c              |  6 ++++--
 path.c                | 10 ++++++----
 path.h                |  7 ++++++-
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git c/builtin/upload-pack.c w/builtin/upload-pack.c
index 46d93278d9..fe50ce3eed 100644
--- c/builtin/upload-pack.c
+++ w/builtin/upload-pack.c
@@ -36,6 +36,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 			    N_("interrupt transfer after <n> seconds of inactivity")),
 		OPT_END()
 	};
+	unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
 
 	packet_trace_identity("upload-pack");
 	disable_replace_refs();
@@ -51,7 +52,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 
 	dir = argv[0];
 
-	if (!enter_repo(dir, strict))
+	if (strict)
+		enter_repo_flags |= ENTER_REPO_STRICT;
+	if (!enter_repo(dir, enter_repo_flags))
 		die("'%s' does not appear to be a git repository", dir);
 
 	switch (determine_protocol_version_server()) {
diff --git c/daemon.c w/daemon.c
index 17d331b2f3..fb37135521 100644
--- c/daemon.c
+++ w/daemon.c
@@ -149,6 +149,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 	size_t rlen;
 	const char *path;
 	const char *dir;
+	unsigned enter_repo_flags;
 
 	dir = directory;
 
@@ -239,14 +240,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		dir = rpath;
 	}
 
-	path = enter_repo(dir, strict_paths);
+	enter_repo_flags = strict_paths ? ENTER_REPO_STRICT : 0;
+	path = enter_repo(dir, enter_repo_flags);
 	if (!path && base_path && base_path_relaxed) {
 		/*
 		 * if we fail and base_path_relaxed is enabled, try without
 		 * prefixing the base path
 		 */
 		dir = directory;
-		path = enter_repo(dir, strict_paths);
+		path = enter_repo(dir, enter_repo_flags);
 	}
 
 	if (!path) {
diff --git c/path.c w/path.c
index 19f7684f38..df5aefdb8f 100644
--- c/path.c
+++ w/path.c
@@ -727,7 +727,7 @@ char *interpolate_path(const char *path, int real_home)
  * links.  User relative paths are also returned as they are given,
  * except DWIM suffixing.
  */
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, unsigned flags)
 {
 	static struct strbuf validated_path = STRBUF_INIT;
 	static struct strbuf used_path = STRBUF_INIT;
@@ -735,7 +735,7 @@ const char *enter_repo(const char *path, int strict)
 	if (!path)
 		return NULL;
 
-	if (!strict) {
+	if (!(flags & ENTER_REPO_STRICT)) {
 		static const char *suffix[] = {
 			"/.git", "", ".git/.git", ".git", NULL,
 		};
@@ -779,7 +779,8 @@ 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 (!(flags & ENTER_REPO_ANY_OWNER_OK))
+			die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
@@ -790,7 +791,8 @@ const char *enter_repo(const char *path, int strict)
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
-		die_upon_dubious_ownership(gitfile, NULL, path);
+		if (!(flags & ENTER_REPO_ANY_OWNER_OK))
+			die_upon_dubious_ownership(gitfile, NULL, path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))
diff --git c/path.h w/path.h
index a6f0b70692..a0021a1425 100644
--- c/path.h
+++ w/path.h
@@ -187,7 +187,12 @@ int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
-const char *enter_repo(const char *path, int strict);
+
+enum {
+	ENTER_REPO_STRICT = (1<<0),
+	ENTER_REPO_ANY_OWNER_OK = (1<<1),
+};
+const char *enter_repo(const char *path, unsigned flags);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);