From: Peijian Ju Date: Wed, 12 Mar 2025 19:53:44 GMT Subject: Re: [PATCH v10 8/8] cat-file: add remote-object-info to batch-command Message-ID: In-Reply-To: <20250224234525.GB729825@coredump.intra.peff.net> On Mon, Feb 24, 2025 at 6:45 PM Jeff King wrote: > > On Fri, Feb 21, 2025 at 10:34:44AM -0500, Peijian Ju wrote: > > > Thank you. Revised to use xstrdup() in v11. > > > > > 2. Are there any bounds on the size of "line"? E.g., is it coming in > > > as a single pkt, or can it be arbitrarily large if an attacker > > > wants (it looks like maybe the latter, since it comes from a strbuf > > > in batch_objects_command(), but I didn't look at how network data > > > gets passed in to that). At any rate, I think we ran into problems > > > before with split_cmdline() and integer overflow, since it returns > > > an int (CVE-2022-39260). I thought we fixed it by rejecting long > > > lines in git-shell, but it looks like we also hardened > > > split_cmdline() in 0ca6ead81e (alias.c: reject too-long cmdline > > > strings in split_cmdline(), 2022-09-28). > > > > > > So we are maybe OK, but I wonder if we should punt on absurd lines. > > > Related, can an attacker just flood input into that strbuf, making > > > it grow forever and waste memory? That's just a simple resource > > > attack, but we have tried to avoid those elsewhere in upload-pack, > > > etc. > > > > > > > Thank you. Adding a check in v11 for the length of `lines`. Please let > > me know if something like this makes sense: > > > > if (strlen(line) >= INT_MAX) { > > die(_("remote-object-info command input overflow")); > > } > > I took a look at what you ended up with in v11, and...I think I totally > misunderstood what was going on in your series, or when this code would > be run. > > I had thought the cat-file here was running on the server side, and that > we needed to protect ourselves against malicious clients. But your new > parse_cmd_remote_object_info() is purely a client-side function that > will then access the server behind the scenes. And its input will be > coming from the stdin of cat-file locally. > > So I'm not sure that we need to protect it unless we think there's some > way that an attacker can automatically trigger arbitrary > remote-object-info requests. > Thank you. Yes, remote-object-info is purely a client-side command.If an attacker is able to automatically trigger arbitrary remote-object-info requests, it likely means they already have control over that system. From my understanding, Git generally trusts its clients. So unless there are strong objections, I will revert those input length checks. > That said, I'm not sure why you need split_cmdline() at all. The format > seems to be: > > remote-object-info ... > > The only thing that _might_ need quoting is the url, but is shell > quoting a reasonable thing there? I'd think that it would be > URL-encoded, and thus contain no spaces. The has to be a real full > oid, I think, because the object-info on the server side insists on > that. > Thank you! We hadn’t given much thought to the URL format earlier, but I agree that it’s reasonable to require the URL in remote-object-info to be properly URL-encoded. With that assumption, splitting on spaces makes sense. I’ll update this in the next patch and also revise the documentation to clarify that URL parameters must be URL-encoded. > So why not just split on space? Something like this: > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 9de1016acd..aedbcba347 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -597,7 +597,7 @@ static void batch_one_object(const char *obj_name, > object_context_release(&ctx); > } > > -static int get_remote_info(struct batch_options *opt, int argc, const char **argv) > +static int get_remote_info(struct batch_options *opt, const char *url, const char *oid_list) > { > int retval = 0; > struct remote *remote = NULL; > @@ -613,16 +613,19 @@ static int get_remote_info(struct batch_options *opt, int argc, const char **arg > if (!opt->format) > opt->format = "%(objectname) %(objectsize)"; > > - remote = remote_get(argv[0]); > + remote = remote_get(url); > if (!remote) > die(_("must supply valid remote when using remote-object-info")); > > oid_array_clear(&object_info_oids); > - for (size_t i = 1; i < argc; i++) { > - if (get_oid_hex(argv[i], &oid)) > - die(_("Not a valid object name %s"), argv[i]); > + while (*oid_list) { > + if (parse_oid_hex(oid_list, &oid, &oid_list)) > + die(_("Not a valid object name %s"), oid_list); > oid_array_append(&object_info_oids, &oid); > + while (*oid_list == ' ') > + oid_list++; > } > + > if (!object_info_oids.nr) > die(_("remote-object-info requires objects")); > > @@ -747,21 +750,15 @@ static void parse_cmd_remote_object_info(struct batch_options *opt, > const char *line, struct strbuf *output, > struct expand_data *data) > { > - int count; > - const char **argv; > - char *line_to_split; > - > - if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE) > - die(_("remote-object-info command input overflow " > - "(no more than %d objects are allowed)"), > - MAX_ALLOWED_OBJ_LIMIT); > + char *url; > + const char *space; > > - line_to_split = xstrdup(line); > - count = split_cmdline(line_to_split, &argv); > - if (count < 0) > - die(_("split remote-object-info command")); > + space = strchr(line, ' '); > + if (!space) > + return; /* report error somehow? */ > + url = xmemdupz(line, space - line); > > - if (get_remote_info(opt, count, argv)) > + if (get_remote_info(opt, url, space + 1)) > goto cleanup; > > data->skip_object_info = 1; > @@ -774,16 +771,15 @@ static void parse_cmd_remote_object_info(struct batch_options *opt, > */ > data->size = *remote_object_info[i].sizep; > opt->batch_mode = BATCH_MODE_INFO; > - batch_object_write(argv[i+1], output, opt, data, NULL, 0); > + batch_object_write(oid_to_hex(&data->oid), output, opt, data, NULL, 0); > } > } > data->skip_object_info = 0; > > cleanup: > for (size_t i = 0; i < object_info_oids.nr; i++) > free_object_info_contents(&remote_object_info[i]); > - free(line_to_split); > - free(argv); > + free(url); > free(remote_object_info); > } > > > You'd need to adjust t1017 to remote the quotes from the inputs, and I > think you'd have to correctly url-encoded the file:// one to avoid > spaces (but that is technically true already! If the filesystem path has > a "%" in it, it would be misinterpreted). > Thank you. Tests will be adjusted. > -Peff