Commit a9f1ca09178af0640963e069a2142d5ced53f0b4
2018-08-09T11:01:00
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)
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72
diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c
index 91a709e..1066bc3 100644
--- a/src/transports/smart_pkt.c
+++ b/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)
diff --git a/tests/transports/smart/packet.c b/tests/transports/smart/packet.c
index 8bbb0bb..f887563 100644
--- a/tests/transports/smart/packet.c
+++ b/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)