smart_pkt: fix buffer overflow when parsing "ok" packets

There are two different buffer overflows present when parsing "ok"
packets. First, we never verify whether the line already ends after
"ok", but directly go ahead and also try to skip the expected space
after "ok". Second, we then go ahead and use `strchr` to scan for the
terminating newline character. But in case where the line isn't
terminated correctly, this can overflow the line buffer.

Fix the issues by using `git__prefixncmp` to check for the "ok " prefix
and only checking for a trailing '\n' instead of using `memchr`. This
also fixes the issue of us always requiring a trailing '\n'.

Reported by oss-fuzz, issue 9749:

Crash Type: Heap-buffer-overflow READ {*}
Crash Address: 0x6310000389c0
Crash State:
  ok_pkt
  git_pkt_parse_line
  git_smart__store_refs

Sanitizer: address (ASAN)
(cherry picked from commit a9f1ca09)

src/transports/smart_pkt.c

@@ -262,21 +262,19 @@ out_err:
 static int ok_pkt(git_pkt **out, const char *line, size_t len)
 {
 	git_pkt_ok *pkt;
-	const char *ptr;
 	size_t alloc_len;
 
 	pkt = git__malloc(sizeof(*pkt));
 	GITERR_CHECK_ALLOC(pkt);
-
 	pkt->type = GIT_PKT_OK;
 
-	line += 3; /* skip "ok " */
-	if (!(ptr = strchr(line, '\n'))) {
-		giterr_set(GITERR_NET, "invalid packet line");
-		git__free(pkt);
-		return -1;
-	}
-	len = ptr - line;
+	if (git__prefixncmp(line, len, "ok "))
+		goto out_err;
+	line += 3;
+	len -= 3;
+
+	if (line[len - 1] == '\n')
+		--len;
 
 	GITERR_CHECK_ALLOC_ADD(&alloc_len, len, 1);
 	pkt->ref = git__malloc(alloc_len);
@@ -287,6 +285,11 @@ static int ok_pkt(git_pkt **out, const char *line, size_t len)
 
 	*out = (git_pkt *)pkt;
 	return 0;
+
+out_err:
+	giterr_set(GITERR_NET, "error parsing OK pkt-line");
+	git__free(pkt);
+	return -1;
 }
 
 static int ng_pkt(git_pkt **out, const char *line, size_t len)

tests/transports/smart/packet.c

@@ -280,18 +280,15 @@ void test_transports_smart_packet__comment_pkt(void)
 
 void test_transports_smart_packet__ok_pkt(void)
 {
-	/*
-	 * TODO: the trailing slash is currently mandatory. Check
-	 * if we should really enforce this. As this test is part
-	 * of a security release, I feel uneasy to change
-	 * behaviour right now and leave it for a later point.
-	 */
 	assert_pkt_fails("0007ok\n");
+	assert_ok_parses("0007ok ", "");
 	assert_ok_parses("0008ok \n", "");
+	assert_ok_parses("0008ok x", "x");
 	assert_ok_parses("0009ok x\n", "x");
+	assert_pkt_fails("001OK ref/foo/bar");
+	assert_ok_parses("0012ok ref/foo/bar", "ref/foo/bar");
 	assert_pkt_fails("0013OK ref/foo/bar\n");
 	assert_ok_parses("0013ok ref/foo/bar\n", "ref/foo/bar");
-	assert_ok_parses("0012ok ref/foo/bar\n", "ref/foo/bar");
 }
 
 void test_transports_smart_packet__ng_pkt(void)