Re: [PATCH v2 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities

From: Jeff King <peff@peff.net>
Date: Apr 12, 2023, 07:26 UTC
Message-ID: <20230412072622.GA1695073@coredump.intra.peff.net>
In-Reply-To: <20230412072510.GA1694877@coredump.intra.peff.net>

On Wed, Apr 12, 2023 at 03:25:10AM -0400, Jeff King wrote:

Show 8 quoted lines

> So I think it's worth fixing, and even worth beefing up the test to
> include this case, but sadly there's no specific case to check for. So I
> just rolled it into the fix in patch 1.
> 
> Here's that patch (and I'll send a range-diff in a second). The rest are
> the same, modulo a small textual conflict in patch 7 that touches a
> nearby line, but I'll refrain from re-sending the rest of them until
> I've gotten more review.

And here's the range-diff.

1:  c379efdcc0 ! 1:  3958a89100 v0 protocol: fix infinite loop when parsing multi-valued capabilities
    @@ Commit message
         feature_list only inside our function, and not the caller, that
         increment is lost next time the function is called.
     
    -    The simplest solution is to account for those skipped bytes by
    -    incrementing *offset, rather than assigning to it. (The other possible
    -    solution is to add an extra level of pointer indirection to feature_list
    -    so that the caller knows we moved it forward, but that seems more
    -    complicated).
    +    One solution would be to account for those skipped bytes by incrementing
    +    *offset, rather than assigning to it. But wait, there's more!
    +
    +    We also increment feature_list if we have a near-miss. Say we are
    +    looking for "symref" and find "almost-symref". In that case we'll point
    +    feature_list to the "y" in "almost-symref" and restart our search. But
    +    that again means our offset won't be correct, as it won't account for
    +    the bytes between the start of the string and that "y".
    +
    +    So instead, let's just record the beginning of the feature_list string
    +    in a separate pointer that we never touch. That offset we take in and
    +    return is meant to be using that point as a base, and now we'll do so
    +    consistently.
     
         Since the bug can't be reproduced using the current version of
         git-upload-pack, we'll instead hard-code an input which triggers the
    @@ Commit message
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## connect.c ##
    +@@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
    + 
    + const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)
    + {
    ++	const char *orig_start = feature_list;
    + 	int len;
    + 
    + 	if (!feature_list)
     @@ connect.c: const char *parse_feature_value(const char *feature_list, const char *feature, i
      				if (lenp)
      					*lenp = 0;
      				if (offset)
     -					*offset = found + len - feature_list;
    -+					*offset += found + len - feature_list;
    ++					*offset = found + len - orig_start;
      				return value;
      			}
      			/* feature with a value (e.g., "agent=git/1.2.3") */
    @@ connect.c: const char *parse_feature_value(const char *feature_list, const char
      					*lenp = end;
      				if (offset)
     -					*offset = value + end - feature_list;
    -+					*offset += value + end - feature_list;
    ++					*offset = value + end - orig_start;
      				return value;
      			}
      			/*
    @@ t/t5512-ls-remote.sh: test_expect_success 'ls-remote prefixes work with all prot
     +	symrefs="symref=refs/remotes/origin/HEAD:refs/remotes/origin/main" &&
     +	symrefs="$symrefs symref=HEAD:refs/heads/main" &&
     +
    ++	# Likewise we want to make sure our parser is not fooled by the string
    ++	# "symref" appearing as part of an earlier cap. But there is no way to
    ++	# do that via upload-pack, as arbitrary strings can appear only in a
    ++	# "symref" value itself (where we skip past the values as a whole)
    ++	# and "agent" (which always appears after "symref", so putting our
    ++	# parser in a confused state is less interesting).
    ++	caps="some other caps including a-fake-symref-cap" &&
    ++
     +	test-tool pkt-line pack >input.q <<-EOF &&
    -+	$oid HEADQ$symrefs
    ++	$oid HEADQ$caps $symrefs
     +	$oid refs/heads/main
     +	$oid refs/remotes/origin/HEAD
     +	$oid refs/remotes/origin/main
2:  2a8e6943e5 = 2:  4cb0739770 t5512: stop referring to "v1" protocol
3:  7e411ed56b = 3:  4fe3be0476 t5512: stop using jgit for capabilities^{} test
4:  65387a02bf = 4:  77875ff7f6 t5512: add v2 support for "ls-remote --symref" test
5:  3409bd94b0 = 5:  642608ac2d t5512: allow any protocol version for filtered symref test
6:  c42a25ceca = 6:  20c78c9e41 t5512: test "ls-remote --heads --symref" filtering with v0 and v2
7:  86067753cb ! 7:  48a46fc6fc v0 protocol: use size_t for capability length/offset
    @@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader
     -const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)
     +const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset)
      {
    + 	const char *orig_start = feature_list;
     -	int len;
     +	size_t len;