Re: Git clone reads safe.directory differently?
From: Jeff King <hidden>
Date: 2024-08-01 06:14:19
On Wed, Jul 31, 2024 at 03:08:55PM -0700, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:quoted
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 ;-).
This looks roughly like I'd expect, but...
quoted hunk ↗ jump to hunk
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()) {
...this is doing that loosening for upload-pack by default. I'm not sure if that is OK or not. My mental model has remained "it is OK to run upload-pack on an untrusted repository", but it would make sense to get input from folks who looked at this in the past, like Dscho, and/or to reassess the threat model from scratch. In particular I did not follow all of the potential issues with linked local files. Are we good now after other fixes (in which case this patch is OK)? Are we good only for non-local clones (so this patch is OK only combined with a fix for clone to check ownership for --local mode)? Or are there still problems if an attacker controls the repo paths, in which case upload-pack should remain conservative? -Peff