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

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