smart_pkt: fix buffer overflow when parsing "ACK" packets

We are being quite lenient when parsing "ACK" packets. First, we didn't
correctly verify that we're not overrunning the provided buffer length,
which we fix here by using `git__prefixncmp` instead of
`git__prefixcmp`. Second, we do not verify that the actual contents make
any sense at all, as we simply ignore errors when parsing the ACKs OID
and any unknown status strings. This may result in a parsed packet
structure with invalid contents, which is being silently passed to the
caller. This is being fixed by performing proper input validation and
checking of return codes.

Author: pks-t

src/transports/smart_pkt.c

@@ -43,34 +43,43 @@ static int flush_pkt(git_pkt **out)
 static int ack_pkt(git_pkt **out, const char *line, size_t len)
 {
 	git_pkt_ack *pkt;
-	GIT_UNUSED(line);
-	GIT_UNUSED(len);
 
 	pkt = git__calloc(1, sizeof(git_pkt_ack));
 	GITERR_CHECK_ALLOC(pkt);
-
 	pkt->type = GIT_PKT_ACK;
-	line += 3;
-	len -= 3;
 
-	if (len >= GIT_OID_HEXSZ) {
-		git_oid_fromstr(&pkt->oid, line + 1);
-		line += GIT_OID_HEXSZ + 1;
-		len -= GIT_OID_HEXSZ + 1;
-	}
+	if (git__prefixncmp(line, len, "ACK "))
+		goto out_err;
+	line += 4;
+	len -= 4;
 
-	if (len >= 7) {
-		if (!git__prefixcmp(line + 1, "continue"))
+	if (len < GIT_OID_HEXSZ || git_oid_fromstr(&pkt->oid, line) < 0)
+		goto out_err;
+	line += GIT_OID_HEXSZ;
+	len -= GIT_OID_HEXSZ;
+
+	if (len && line[0] == ' ') {
+		line++;
+		len--;
+
+		if (!git__prefixncmp(line, len, "continue"))
 			pkt->status = GIT_ACK_CONTINUE;
-		if (!git__prefixcmp(line + 1, "common"))
+		else if (!git__prefixncmp(line, len, "common"))
 			pkt->status = GIT_ACK_COMMON;
-		if (!git__prefixcmp(line + 1, "ready"))
+		else if (!git__prefixncmp(line, len, "ready"))
 			pkt->status = GIT_ACK_READY;
+		else
+			goto out_err;
 	}
 
 	*out = (git_pkt *) pkt;
 
 	return 0;
+
+out_err:
+	giterr_set(GITERR_NET, "error parsing ACK pkt-line");
+	git__free(pkt);
+	return -1;
 }
 
 static int nak_pkt(git_pkt **out)

tests/transports/smart/packet.c

@@ -236,25 +236,20 @@ void test_transports_smart_packet__ack_pkt(void)
 			  "0000000000000000000000000000000000000000",
 			  GIT_ACK_READY);
 
-	/* TODO: these should fail as they don't have OIDs */
-	assert_ack_parses("0007ACK", "0000000000000000000000000000000000000000", 0);
-	assert_ack_parses("0008ACK ", "0000000000000000000000000000000000000000", 0);
+	/* these should fail as they don't have OIDs */
+	assert_pkt_fails("0007ACK");
+	assert_pkt_fails("0008ACK ");
 
-	/* TODO: this one is missing a space and should thus fail */
-	assert_ack_parses("0036ACK00000000000000000x0000000000000000000000 ready",
-			  "0000000000000000000000000000000000000000", 0);
+	/* this one is missing a space and should thus fail */
+	assert_pkt_fails("0036ACK00000000000000000x0000000000000000000000 ready");
 
-	/* TODO: the following ones have invalid OIDs and should thus fail */
-	assert_ack_parses("0037ACK 00000000000000000x0000000000000000000000 ready",
-			  "0000000000000000000000000000000000000000", GIT_ACK_READY);
-	assert_ack_parses("0036ACK 000000000000000000000000000000000000000 ready",
-			  "0000000000000000000000000000000000000000", 0);
-	assert_ack_parses("0036ACK 00000000000000000x0000000000000000000000ready",
-			  "0000000000000000000000000000000000000000", 0);
+	/* the following ones have invalid OIDs and should thus fail */
+	assert_pkt_fails("0037ACK 00000000000000000x0000000000000000000000 ready");
+	assert_pkt_fails("0036ACK 000000000000000000000000000000000000000 ready");
+	assert_pkt_fails("0036ACK 00000000000000000x0000000000000000000000ready");
 
-	/* TODO: this one has an invalid status and should thus fail */
-	assert_ack_parses("0036ACK 0000000000000000000000000000000000000000 read",
-			  "0000000000000000000000000000000000000000", 0);
+	/* this one has an invalid status and should thus fail */
+	assert_pkt_fails("0036ACK 0000000000000000000000000000000000000000 read");
 }
 
 void test_transports_smart_packet__nak_pkt(void)