On Fri, Jan 31, 2025 at 9:03 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Mon, Jan 13, 2025 at 09:15:00PM -0500, Eric Ju wrote:
>
> > +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 = xstrdup_or_null(line);
> > + count = split_cmdline(line_to_split, &argv);
> > + if (get_remote_info(opt, count, argv))
> > + goto cleanup;
>
> Coverity complains that split_cmdline() can return a negative value when
> the input is malformed, which we then feed to get_remote_info(). If I
> understand correctly (from my very brief glance at the series), that
> string would be under the control of the untrusted client?
>
> I _think_ an attacker can't do anything too bad here, since
> get_remote_info() also takes a signed int, and so iterating from 0 will
> just find no entries. But probably we should explicitly check for error
> and bail.
>
An explicit check is added to make sure if a negative value is returned, we
will error and bail.
> While just looking at this code from a security perspective, two other
> things occur to me:
>
> 1. Calling xstrdup_or_null() implies that "line" may be NULL, which
> would make "line_to_split" also NULL. But I think split_cmdline()
> would segfault in that case. Should it just be xstrdup()?
>
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"));
}
> -Peff
