Re: [PATCH v10 8/8] cat-file: add remote-object-info to batch-command
- From
Jeff King <peff@peff.net>
- Date
- Feb 24, 2025, 23:45 UTC
- Message-ID
- <20250224234525.GB729825@coredump.intra.peff.net>
- In-Reply-To
- <CAN2LT1AFA8AG58NNVvW2nvWR27qisPXDzSTR-tUycWGi96kcaw@mail.gmail.com>
On Fri, Feb 21, 2025 at 10:34:44AM -0500, Peijian Ju wrote:
Show 26 quoted lines
> 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.
That said, I'm not sure why you need split_cmdline() at all. The format seems to be:
remote-object-info <url> <oid>...
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 <oid> has to be a real full oid, I think, because the object-info on the server side insists on that.
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). -Peff