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.

2 files changed, 34 insertions, 30 deletions

diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c
index 6590665e4..91a709e38 100644
--- a/src/transports/smart_pkt.c
+++ b/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)
diff --git a/tests/transports/smart/packet.c b/tests/transports/smart/packet.c
index 2b9b38f5c..8bbb0bbcf 100644
--- a/tests/transports/smart/packet.c
+++ b/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)