Commit bc349045b1be8fb3af2b02d8554483869e54d5b8
2018-08-09T10:38:10
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.
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 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103
diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c
index 6590665..91a709e 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 2b9b38f..8bbb0bb 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)