Re: [PATCH v10 8/8] cat-file: add remote-object-info to batch-command

From: Jeff King <peff@peff.net>
Date: Feb 1, 2025, 02:03 UTC
Message-ID: <20250201020321.GA4081169@coredump.intra.peff.net>
In-Reply-To: <20250114021502.41499-9-eric.peijian@gmail.com>

On Mon, Jan 13, 2025 at 09:15:00PM -0500, Eric Ju wrote:

Show 11 quoted lines

> +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