From: Denis Ovsienko <denis@ovsienko.info>
Date: Wed, 30 Aug 2017 12:42:22 +0000 (+0100)
Subject: NTP: Improve length checks.
X-Git-Tag: tcpdump-4.99-bp~2012
X-Git-Url: https://git.tcpdump.org/tcpdump/commitdiff_plain/0559dc98748bac47c062c0c33faec2cae44f6fa0

NTP: Improve length checks.

In ntp_print() add a missing length check to reject packets that are
declared too short, make the existing length checks easier to follow and
add a catch-all block after the known message layouts.

This fixes a bug where an invalid packet could be erroneously printed
like it is valid so long as the provided buffer was large enough. That
said, the bounds checks were done correctly so there was no over-read.
---

diff --git a/print-ntp.c b/print-ntp.c
index 5b6771b4..177eed22 100644
--- a/print-ntp.c
+++ b/print-ntp.c
@@ -113,6 +113,12 @@ struct s_fixedpt {
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  */
 
+/* Length of the NTP message with the mandatory fields ("the header")
+ * and without any optional fields (extension, Key Identifier,
+ * Message Digest).
+ */
+#define NTP_MSG_MINLEN 48
+
 struct ntpdata {
 	u_char status;		/* status of local clock and leap info */
 	u_char stratum;		/* Stratum level */
@@ -211,6 +217,11 @@ ntp_print(netdissect_options *ndo,
 	register const struct ntpdata *bp;
 	int mode, version, leapind;
 
+	if (length < NTP_MSG_MINLEN) {
+		ND_PRINT((ndo, "NTP, length %u", length));
+		goto invalid;
+	}
+
 	bp = (const struct ntpdata *)cp;
 
 	ND_TCHECK(bp->status);
@@ -310,10 +321,11 @@ ntp_print(netdissect_options *ndo,
 	ND_PRINT((ndo, "\n\t    Originator - Transmit Timestamp: "));
 	p_ntp_delta(ndo, &(bp->org_timestamp), &(bp->xmt_timestamp));
 
-	if ( (sizeof(struct ntpdata) - length) == 20) { 	/* Optional: key-id (crypto-NAK) */
+	/* FIXME: this code is not aware of any extension fields */
+	if (length == NTP_MSG_MINLEN + 4) { 	/* Optional: key-id (crypto-NAK) */
 		ND_TCHECK(bp->key_id);
 		ND_PRINT((ndo, "\n\tKey id: %u", EXTRACT_32BITS(&bp->key_id)));
-	} else if ( (sizeof(struct ntpdata) - length) == 4) { 	/* Optional: key-id + 128-bit digest */
+	} else if (length == NTP_MSG_MINLEN + 4 + 16) { 	/* Optional: key-id + 128-bit digest */
 		ND_TCHECK(bp->key_id);
 		ND_PRINT((ndo, "\n\tKey id: %u", EXTRACT_32BITS(&bp->key_id)));
 		ND_TCHECK2(bp->message_digest, 16);
@@ -322,7 +334,7 @@ ntp_print(netdissect_options *ndo,
 		               EXTRACT_32BITS(bp->message_digest + 4),
 		               EXTRACT_32BITS(bp->message_digest + 8),
 		               EXTRACT_32BITS(bp->message_digest + 12)));
-	} else if ( (sizeof(struct ntpdata) - length) == 0) { 	/* Optional: key-id + 160-bit digest */
+	} else if (length == NTP_MSG_MINLEN + 4 + 20) { 	/* Optional: key-id + 160-bit digest */
 		ND_TCHECK(bp->key_id);
 		ND_PRINT((ndo, "\n\tKey id: %u", EXTRACT_32BITS(&bp->key_id)));
 		ND_TCHECK2(bp->message_digest, 20);
@@ -332,7 +344,14 @@ ntp_print(netdissect_options *ndo,
 		               EXTRACT_32BITS(bp->message_digest + 8),
 		               EXTRACT_32BITS(bp->message_digest + 12),
 		               EXTRACT_32BITS(bp->message_digest + 16)));
-        }
+	} else if (length > NTP_MSG_MINLEN) {
+		ND_PRINT((ndo, "\n\t(%u more bytes after the header)", length - NTP_MSG_MINLEN));
+	}
+	return;
+
+invalid:
+	ND_PRINT((ndo, " %s", istr));
+	ND_TCHECK2(*cp, length);
 	return;
 
 trunc: