CVE-2016-10129: NULL Pointer Dereference (CWE-476)
value: The Git Smart Protocol support in libgit2 before 0.24.6 and 0.25.x before 0.25.1 allows remote attackers to cause a denial of service (NULL pointer dereference) via an empty packet line.
CWE description: The product dereferences a pointer that it expects to be valid but is NULL.
CVSS v3 base score: 7.5
Published: 2017-03-24T15:59Z
Repository: https://github.com/libgit2/libgit2
Language: C
Files changed: src/transports/smart_pkt.c, src/transports/smart_protocol.c
Commit date: 2017-01-06 17:11:58 +0000
Commit message: 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.

Diff: 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
@@ -435,17 +435,17 @@ int git_pkt_parse_line(
 	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
-	 * 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 */
 		*out = line;
 		return flush_pkt(head);
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
@@ -761,18 +761,10 @@ static int add_push_report_sideband_pkt(git_push *push, git_pkt_data *data_pkt,
 
 		/* Advance in the buffer */
 		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);
 
 		if (error < 0 && error != GIT_ITEROVER)
@@ -823,13 +815,10 @@ static int parse_report(transport_smart *transport, git_push *push)
 
 		gitno_consume(buf, line_end);
 
 		error = 0;
 
-		if (pkt == NULL)
-			continue;
-
 		switch (pkt->type) {
 			case GIT_PKT_DATA:
 				/* This is a sideband packet which contains other packets */
 				error = add_push_report_sideband_pkt(push, (git_pkt_data *)pkt, &data_pkt_buf);
 				break;

Vulnerable code: 	 * TODO: How do we deal with empty lines? Try again? with the next
	 * line?
		*head = NULL;
		*out = line;
		return 0;
		/* 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;

		if (pkt == NULL)
			continue;

Fixed code: 	 * 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.
		giterr_set_str(GITERR_NET, "Invalid empty packet");
		return GIT_ERROR;