From 66e3774d279672ee51c3b54545a79d20d1ada834 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 15 Nov 2016 11:36:27 +0100 Subject: [PATCH 1/2] smart_pkt: verify packet length exceeds PKT_LEN_SIZE Each packet line in the Git protocol is prefixed by a four-byte length of how much data will follow, which we parse in `git_pkt_parse_line`. The transmitted length can either be equal to zero in case of a flush packet or has to be at least of length four, as it also includes the encoded length itself. Not checking this may result in a buffer overflow as we directly pass the length to functions which accept a `size_t` length as parameter. Fix the issue by verifying that non-flush packets have at least a length of `PKT_LEN_SIZE`. --- src/transports/smart_pkt.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 2297cc94f..6fe53b931 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -427,6 +427,14 @@ int git_pkt_parse_line( if (bufflen > 0 && bufflen < (size_t)len) return GIT_EBUFS; + /* + * The length has to be exactly 0 in case of a flush + * packet or greater than PKT_LEN_SIZE, as the decoded + * length includes its own encoded length of four bytes. + */ + if (len != 0 && len < PKT_LEN_SIZE) + return GIT_ERROR; + line += PKT_LEN_SIZE; /* * TODO: How do we deal with empty lines? Try again? with the next From 2fdef641fd0dd2828bd948234ae86de75221a11a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 15 Nov 2016 11:44:51 +0100 Subject: [PATCH 2/2] smart_pkt: treat empty packet lines as error The Git protocol does not specify what should happen in the case of an empty packet line (that is a packet line "0004"). We currently indicate success, but do not return a packet in the case where we hit an empty line. The smart protocol was not prepared to handle such packets in all cases, though, resulting in a `NULL` pointer dereference. Fix the issue by returning an error instead. As such kind of packets is not even specified by upstream, this is the right thing to do. --- src/transports/smart_pkt.c | 10 +++++----- src/transports/smart_protocol.c | 11 ----------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 6fe53b931..e05196cd8 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -437,13 +437,13 @@ int git_pkt_parse_line( line += PKT_LEN_SIZE; /* - * TODO: How do we deal with empty lines? Try again? with the next - * line? + * The Git protocol does not specify empty lines as part + * of the protocol. Not knowing what to do with an empty + * line, we should return an error upon hitting one. */ if (len == PKT_LEN_SIZE) { - *head = NULL; - *out = line; - return 0; + giterr_set_str(GITERR_NET, "Invalid empty packet"); + return GIT_ERROR; } if (len == 0) { /* Flush pkt */ diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c index 53c0b089e..db6a8b9c8 100644 --- a/src/transports/smart_protocol.c +++ b/src/transports/smart_protocol.c @@ -763,14 +763,6 @@ static int add_push_report_sideband_pkt(git_push *push, git_pkt_data *data_pkt, line_len -= (line_end - line); line = line_end; - /* When a valid packet with no content has been - * read, git_pkt_parse_line does not report an - * error, but the pkt pointer has not been set. - * Handle this by skipping over empty packets. - */ - if (pkt == NULL) - continue; - error = add_push_report_pkt(push, pkt); git_pkt_free(pkt); @@ -825,9 +817,6 @@ static int parse_report(transport_smart *transport, git_push *push) error = 0; - if (pkt == NULL) - continue; - switch (pkt->type) { case GIT_PKT_DATA: /* This is a sideband packet which contains other packets */