On Wed, Jul 31, 2024 at 03:08:55PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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
> ;-).
This looks roughly like I'd expect, but...
> 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
