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.
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()?
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.
-Peff
