[PATCH v3 04/19] fetch-pack: fix out-of-bounds buffer offset in get_ack

When we read acks from the remote, we expect either:

  ACK <sha1>

or

  ACK <sha1> <multi-ack-flag>

We parse the "ACK <sha1>" bit from the line, and then start looking for the flag strings at "line+45"; if we don't have them, we assume it's of the first type. But if we do have the first type, then line+45 is not necessarily inside our string at all!

It turns out that this works most of the time due to the way we parse the packets. They should come in with a newline, and packet_read puts an extra NUL into the buffer, so we end up with:

  ACK <sha1>\n\0

with the newline at offset 44 and the NUL at offset 45. We then strip the newline, putting a NUL at offset 44. So when we look at "line+45", we are looking past the end of our string; but it's OK, because we hit the terminator from the original string.

This breaks down, however, if the other side does not terminate their packets with a newline. In that case, our packet is one character shorter, and we start looking through uninitialized memory for the flag. No known implementation sends such a packet, so it has never come up in practice.

This patch tightens the check by looking for a short, flagless ACK before trying to parse the flag.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the absolute minimal fix, which just checks for the no-flag case
early; we still treat arbitrary crud in the flag field as just an ACK.
From my understanding of the protocol, a saner parsing scheme would be:

  const char *flag = line + 44; /* we already parsed "ACK <sha1>" */
  if (!*flag)
          return ACK;
  if (!strcmp(flag, " continue"))
          return ACK_continue;
  if (!strcmp(flag, " common"))
          return ACK_continue;
  if (!strcmp(flag, " ready"))
          return ACK_ready;
  die("fetch-pack expected multi-ack flag, got: %s", line);

But that is much tighter, and I wasn't sure if the looseness was there to facilitate future expansion or something (though I'd think we would need a new capability for that).

 fetch-pack.c | 2 ++
 1 file changed, 2 insertions(+)