CVE-2016-5357: Improper Input Validation (CWE-20)
value: wiretap/netscreen.c in the NetScreen file parser in Wireshark 1.12.x before 1.12.12 and 2.x before 2.0.4 mishandles sscanf unsigned-integer processing, which allows remote attackers to cause a denial of service (application crash) via a crafted file.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 5.9
Published: 2016-08-07T16:59Z
Repository: https://github.com/wireshark/wireshark
Language: C
Files changed: wiretap/netscreen.c, wiretap/netscreen.h
Commit date: 2016-04-30 00:08:48 +0000
Commit message: Fix packet length handling.

Treat the packet length as unsigned - it shouldn't be negative in the
file.  If it is, that'll probably cause the sscanf to fail, so we'll
report the file as bad.

Check it against WTAP_MAX_PACKET_SIZE to make sure we don't try to
allocate a huge amount of memory, just as we do in other file readers.

Use the now-validated packet size as the length in
ws_buffer_assure_space(), so we are certain to have enough space, and
don't allocate too much space.

Merge the header and packet data parsing routines while we're at it.

Bug: 12396
Change-Id: I7f981f9cdcbea7ecdeb88bfff2f12d875de2244f
Reviewed-on: https://code.wireshark.org/review/15176
Reviewed-by: Guy Harris <guy@alum.mit.edu>

Diff: diff --git a/wiretap/netscreen.c b/wiretap/netscreen.c
index c447a9a214d..e10b1d9fe3a 100644
--- a/wiretap/netscreen.c
+++ b/wiretap/netscreen.c
@@ -67,16 +67,12 @@ static gboolean netscreen_check_file_type(wtap *wth, int *err,
 static gboolean netscreen_read(wtap *wth, int *err, gchar **err_info,
 	gint64 *data_offset);
 static gboolean netscreen_seek_read(wtap *wth, gint64 seek_off,
 	struct wtap_pkthdr *phdr, Buffer *buf,
 	int *err, gchar **err_info);
-static int parse_netscreen_rec_hdr(struct wtap_pkthdr *phdr, const char *line,
-	char *cap_int, gboolean *cap_dir, char *cap_dst,
-	int *err, gchar **err_info);
-static gboolean parse_netscreen_hex_dump(FILE_T fh, int pkt_len,
-	const char *cap_int, const char *cap_dst, struct wtap_pkthdr *phdr,
-	Buffer* buf, int *err, gchar **err_info);
+static gboolean parse_netscreen_packet(FILE_T fh, struct wtap_pkthdr *phdr,
+	Buffer* buf, char *line, int *err, gchar **err_info);
 static int parse_single_hex_dump_line(char* rec, guint8 *buf,
 	guint byte_offset);
 
 /* Returns TRUE if the line appears to be a line with protocol info.
    Otherwise it returns FALSE. */
@@ -189,31 +185,20 @@ wtap_open_return_val netscreen_open(wtap *wth, int *err, gchar **err_info)
 /* Find the next packet and parse it; called from wtap_read(). */
 static gboolean netscreen_read(wtap *wth, int *err, gchar **err_info,
     gint64 *data_offset)
 {
 	gint64		offset;
-	int		pkt_len;
 	char		line[NETSCREEN_LINE_LENGTH];
-	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
-	gboolean	cap_dir;
-	char		cap_dst[13];
 
 	/* Find the next packet */
 	offset = netscreen_seek_next_packet(wth, err, err_info, line);
 	if (offset < 0)
 		return FALSE;
 
-	/* Parse the header */
-	pkt_len = parse_netscreen_rec_hdr(&wth->phdr, line, cap_int, &cap_dir,
-	    cap_dst, err, err_info);
-	if (pkt_len == -1)
-		return FALSE;
-
-	/* Convert the ASCII hex dump to binary data, and fill in some
-	   struct wtap_pkthdr fields */
-	if (!parse_netscreen_hex_dump(wth->fh, pkt_len, cap_int,
-	    cap_dst, &wth->phdr, wth->frame_buffer, err, err_info))
+	/* Parse the header and convert the ASCII hex dump to binary data */
+	if (!parse_netscreen_packet(wth->fh, &wth->phdr,
+	    wth->frame_buffer, line, err, err_info))
 		return FALSE;
 
 	/*
 	 * If the per-file encapsulation isn't known, set it to this
 	 * packet's encapsulation.
@@ -237,15 +222,11 @@ static gboolean netscreen_read(wtap *wth, int *err, gchar **err_info,
 static gboolean
 netscreen_seek_read(wtap *wth, gint64 seek_off,
 	struct wtap_pkthdr *phdr, Buffer *buf,
 	int *err, gchar **err_info)
 {
-	int		pkt_len;
 	char		line[NETSCREEN_LINE_LENGTH];
-	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
-	gboolean	cap_dir;
-	char		cap_dst[13];
 
 	if (file_seek(wth->random_fh, seek_off, SEEK_SET, err) == -1) {
 		return FALSE;
 	}
 
@@ -255,19 +236,12 @@ netscreen_seek_read(wtap *wth, gint64 seek_off,
 			*err = WTAP_ERR_SHORT_READ;
 		}
 		return FALSE;
 	}
 
-	pkt_len = parse_netscreen_rec_hdr(phdr, line, cap_int, &cap_dir,
-	    cap_dst, err, err_info);
-	if (pkt_len == -1)
-		return FALSE;
-
-	if (!parse_netscreen_hex_dump(wth->random_fh, pkt_len, cap_int,
-	    cap_dst, phdr, buf, err, err_info))
-		return FALSE;
-	return TRUE;
+	return parse_netscreen_packet(wth->random_fh, phdr, buf, line,
+	    err, err_info);
 }
 
 /* Parses a packet record header. There are a few possible formats:
  *
  * XXX list extra formats here!
@@ -283,53 +257,58 @@ netscreen_seek_read(wtap *wth, gint64 seek_off,
               2c 21 b6 d3 20 60 0c 8c 35 98 88 cf 20 91 0e a9     ,!...`..5.......
               1d 0b                                               ..
 
 
  */
-static int
-parse_netscreen_rec_hdr(struct wtap_pkthdr *phdr, const char *line, char *cap_int,
-    gboolean *cap_dir, char *cap_dst, int *err, gchar **err_info)
+static gboolean
+parse_netscreen_packet(FILE_T fh, struct wtap_pkthdr *phdr, Buffer* buf,
+    char *line, int *err, gchar **err_info)
 {
-	int	sec;
-	int	dsec, pkt_len;
-	char	direction[2];
-	char	cap_src[13];
+	int		sec;
+	int		dsec;
+	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
+	char		direction[2];
+	guint		pkt_len;
+	char		cap_src[13];
+	char		cap_dst[13];
+	guint8		*pd;
+	gchar		*p;
+	int		n, i = 0;
+	guint		offset = 0;
+	gchar		dststr[13];
 
 	phdr->rec_type = REC_TYPE_PACKET;
 	phdr->presence_flags = WTAP_HAS_TS|WTAP_HAS_CAP_LEN;
 
-	if (sscanf(line, "%9d.%9d: %15[a-z0-9/:.-](%1[io]) len=%9d:%12s->%12s/",
+	if (sscanf(line, "%9d.%9d: %15[a-z0-9/:.-](%1[io]) len=%9u:%12s->%12s/",
 		   &sec, &dsec, cap_int, direction, &pkt_len, cap_src, cap_dst) < 5) {
 		*err = WTAP_ERR_BAD_FILE;
 		*err_info = g_strdup("netscreen: Can't parse packet-header");
 		return -1;
 	}
+	if (pkt_len > WTAP_MAX_PACKET_SIZE) {
+		/*
+		 * Probably a corrupt capture file; don't blow up trying
+		 * to allocate space for an immensely-large packet.
+		 */
+		*err = WTAP_ERR_BAD_FILE;
+		*err_info = g_strdup_printf("netscreen: File has %u-byte packet, bigger than maximum of %u",
+		    pkt_len, WTAP_MAX_PACKET_SIZE);
+		return FALSE;
+	}
 
-	*cap_dir = (direction[0] == 'o' ? NETSCREEN_EGRESS : NETSCREEN_INGRESS);
+	/*
+	 * If direction[0] is 'o', the direction is NETSCREEN_EGRESS,
+	 * otherwise it's NETSCREEN_INGRESS.
+	 */
 
 	phdr->ts.secs  = sec;
 	phdr->ts.nsecs = dsec * 100000000;
 	phdr->len = pkt_len;
 
-	return pkt_len;
-}
-
-/* Converts ASCII hex dump to binary data, and fills in some struct
-   wtap_pkthdr fields.  Returns TRUE on success and FALSE on any error. */
-static gboolean
-parse_netscreen_hex_dump(FILE_T fh, int pkt_len, const char *cap_int,
-    const char *cap_dst, struct wtap_pkthdr *phdr, Buffer* buf,
-    int *err, gchar **err_info)
-{
-	guint8	*pd;
-	gchar	line[NETSCREEN_LINE_LENGTH];
-	gchar	*p;
-	int	n, i = 0, offset = 0;
-	gchar	dststr[13];
-
 	/* Make sure we have enough room for the packet */
-	ws_buffer_assure_space(buf, NETSCREEN_MAX_PACKET_LEN);
+	ws_buffer_assure_space(buf, pkt_len);
 	pd = ws_buffer_start_ptr(buf);
 
 	while(1) {
 
 		/* The last packet is not delimited by an empty line, but by EOF
@@ -371,11 +350,11 @@ parse_netscreen_hex_dump(FILE_T fh, int pkt_len, const char *cap_int,
 		}
 
 		/* If there is no more data and the line was not empty,
 		 * then there must be an error in the file
 		 */
-		if(n == -1) {
+		if (n == -1) {
 			*err = WTAP_ERR_BAD_FILE;
 			*err_info = g_strdup("netscreen: cannot parse hex-data");
 			return FALSE;
 		}
 
@@ -383,11 +362,11 @@ parse_netscreen_hex_dump(FILE_T fh, int pkt_len, const char *cap_int,
 		offset += n;
 
 		/* If there was more hex-data than was announced in the len=x
 		 * header, then then there must be an error in the file
 		 */
-		if(offset > pkt_len) {
+		if (offset > pkt_len) {
 			*err = WTAP_ERR_BAD_FILE;
 			*err_info = g_strdup("netscreen: too much hex-data");
 			return FALSE;
 		}
 	}
diff --git a/wiretap/netscreen.h b/wiretap/netscreen.h
index 7609e9f6c17..b3b84135947 100644
--- a/wiretap/netscreen.h
+++ b/wiretap/netscreen.h
@@ -44,11 +44,8 @@
 #define NETSCREEN_MAX_INT_NAME_LENGTH	16
 
 #define NETSCREEN_INGRESS		FALSE
 #define NETSCREEN_EGRESS		TRUE
 
-
-#define NETSCREEN_MAX_PACKET_LEN	65536
-
 wtap_open_return_val netscreen_open(wtap *wth, int *err, gchar **err_info);
 
 #endif

Vulnerable code: static int parse_netscreen_rec_hdr(struct wtap_pkthdr *phdr, const char *line,
	char *cap_int, gboolean *cap_dir, char *cap_dst,
	int *err, gchar **err_info);
static gboolean parse_netscreen_hex_dump(FILE_T fh, int pkt_len,
	const char *cap_int, const char *cap_dst, struct wtap_pkthdr *phdr,
	Buffer* buf, int *err, gchar **err_info);
	int		pkt_len;
	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
	gboolean	cap_dir;
	char		cap_dst[13];
	/* Parse the header */
	pkt_len = parse_netscreen_rec_hdr(&wth->phdr, line, cap_int, &cap_dir,
	    cap_dst, err, err_info);
	if (pkt_len == -1)
		return FALSE;

	/* Convert the ASCII hex dump to binary data, and fill in some
	   struct wtap_pkthdr fields */
	if (!parse_netscreen_hex_dump(wth->fh, pkt_len, cap_int,
	    cap_dst, &wth->phdr, wth->frame_buffer, err, err_info))
	int		pkt_len;
	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
	gboolean	cap_dir;
	char		cap_dst[13];
	pkt_len = parse_netscreen_rec_hdr(phdr, line, cap_int, &cap_dir,
	    cap_dst, err, err_info);
	if (pkt_len == -1)
		return FALSE;

	if (!parse_netscreen_hex_dump(wth->random_fh, pkt_len, cap_int,
	    cap_dst, phdr, buf, err, err_info))
		return FALSE;
	return TRUE;
static int
parse_netscreen_rec_hdr(struct wtap_pkthdr *phdr, const char *line, char *cap_int,
    gboolean *cap_dir, char *cap_dst, int *err, gchar **err_info)
	int	sec;
	int	dsec, pkt_len;
	char	direction[2];
	char	cap_src[13];
	if (sscanf(line, "%9d.%9d: %15[a-z0-9/:.-](%1[io]) len=%9d:%12s->%12s/",
	*cap_dir = (direction[0] == 'o' ? NETSCREEN_EGRESS : NETSCREEN_INGRESS);
	return pkt_len;
}

/* Converts ASCII hex dump to binary data, and fills in some struct
   wtap_pkthdr fields.  Returns TRUE on success and FALSE on any error. */
static gboolean
parse_netscreen_hex_dump(FILE_T fh, int pkt_len, const char *cap_int,
    const char *cap_dst, struct wtap_pkthdr *phdr, Buffer* buf,
    int *err, gchar **err_info)
{
	guint8	*pd;
	gchar	line[NETSCREEN_LINE_LENGTH];
	gchar	*p;
	int	n, i = 0, offset = 0;
	gchar	dststr[13];

	ws_buffer_assure_space(buf, NETSCREEN_MAX_PACKET_LEN);
		if(n == -1) {
		if(offset > pkt_len) {

#define NETSCREEN_MAX_PACKET_LEN	65536

Fixed code: static gboolean parse_netscreen_packet(FILE_T fh, struct wtap_pkthdr *phdr,
	Buffer* buf, char *line, int *err, gchar **err_info);
	/* Parse the header and convert the ASCII hex dump to binary data */
	if (!parse_netscreen_packet(wth->fh, &wth->phdr,
	    wth->frame_buffer, line, err, err_info))
	return parse_netscreen_packet(wth->random_fh, phdr, buf, line,
	    err, err_info);
static gboolean
parse_netscreen_packet(FILE_T fh, struct wtap_pkthdr *phdr, Buffer* buf,
    char *line, int *err, gchar **err_info)
	int		sec;
	int		dsec;
	char		cap_int[NETSCREEN_MAX_INT_NAME_LENGTH];
	char		direction[2];
	guint		pkt_len;
	char		cap_src[13];
	char		cap_dst[13];
	guint8		*pd;
	gchar		*p;
	int		n, i = 0;
	guint		offset = 0;
	gchar		dststr[13];
	if (sscanf(line, "%9d.%9d: %15[a-z0-9/:.-](%1[io]) len=%9u:%12s->%12s/",
	if (pkt_len > WTAP_MAX_PACKET_SIZE) {
		/*
		 * Probably a corrupt capture file; don't blow up trying
		 * to allocate space for an immensely-large packet.
		 */
		*err = WTAP_ERR_BAD_FILE;
		*err_info = g_strdup_printf("netscreen: File has %u-byte packet, bigger than maximum of %u",
		    pkt_len, WTAP_MAX_PACKET_SIZE);
		return FALSE;
	}
	/*
	 * If direction[0] is 'o', the direction is NETSCREEN_EGRESS,
	 * otherwise it's NETSCREEN_INGRESS.
	 */
	ws_buffer_assure_space(buf, pkt_len);
		if (n == -1) {
		if (offset > pkt_len) {