Changeset c171463

Timestamp:

06/30/18 20:26:46 (2 years ago)

Author:

Richard Sanger <rsanger@…>

Branches:

cachetimestamps, develop, master, rc-4.0.4, ringdecrementfix, ringperformance

Children:

e17bad6

Parents:

7bb2fd4

git-author:

Richard Sanger <rsanger@…> (06/30/18 19:48:48)

git-committer:

Richard Sanger <rsanger@…> (06/30/18 20:26:46)

Message:

Fixes pcapng bug where pcapng_parse_next_option would read invalid memory

pcapng_parse_next_option was being accidentally called on the trailing
block size in many parts of the code. Which caused invalid memory reads
off the end of the buffer. This commit fixes this by hardening
pcapng_parse_next_option.

pcapng_parse_next_option now checks that it has not read off the end
of a block.
pcapng_parse_next_option will now return a 'endofopt' if the end
of the option list is not found without encountering a 'endofopt'
option type.

I've also added a check to ensure the block size will not overflow the
packet buffer, which it is read into.

File:

• lib/format_pcapng.c (modified) (8 diffs)

lib/format_pcapng.c

@@ -173 +173 @@
 };
 
+typedef struct pcapng_peeker pcapng_hdr_t;
+
 
 #define DATA(x) ((struct pcapng_format_data_t *)((x)->format_data))
@@ -287 +289 @@
 
 static char *pcapng_parse_next_option(libtrace_t *libtrace, char **pktbuf,
-                uint16_t *code, uint16_t *length) {
+                uint16_t *code, uint16_t *length, pcapng_hdr_t *blockhdr) {
 
         struct pcapng_optheader *opthdr = (struct pcapng_optheader *)*pktbuf;
         int to_skip;
         int padding = 0;
+        char *eob; //end of block
         char *optval;
+        if (DATA(libtrace)->byteswapped) {
+                eob = ((char *) blockhdr) + byteswap32(blockhdr->blocklen);
+        } else {
+                eob = ((char *) blockhdr) + blockhdr->blocklen;
+        }
+
+        assert((char *)blockhdr < *pktbuf);
+        // Check if we have reached the end of the block, +4 for trailing block-size
+        // We cannot assume a endofopt, so we add one
+        if (eob == (*pktbuf) + 4) {
+                *code = 0;
+                *length = 0;
+                return *pktbuf;
+        }
+        // If there is not enough space for another header we've encountered an error
+        if (eob < (*pktbuf) + 4 + sizeof(struct pcapng_optheader)) {
+                return NULL;
+        }
 
         if (DATA(libtrace)->byteswapped) {
@@ -311 +332 @@
 
         to_skip = (*length) + padding;
+        // Check the value we return is within the block length
+        if (eob < optval + to_skip + 4) {
+                return NULL;
+        }
         *pktbuf = optval + to_skip;
 
@@ -588 +613 @@
         do {
                 optval = pcapng_parse_next_option(libtrace, &bodyptr,
-                                &optcode, &optlen);
+                                &optcode, &optlen, (pcapng_hdr_t *) packet->buffer);
                 if (optval == NULL) {
                         trace_set_err(libtrace, TRACE_ERR_BAD_PACKET,
@@ -799 +824 @@
         do {
                 optval = pcapng_parse_next_option(packet->trace, &bodyptr,
-                                &optcode, &optlen);
+                                &optcode, &optlen, (pcapng_hdr_t *) packet->buffer);
                 if (optval == NULL) {
                         trace_set_err(libtrace, TRACE_ERR_BAD_PACKET,
@@ -992 +1017 @@
         do {
                 optval = pcapng_parse_next_option(packet->trace, &bodyptr,
-                                &optcode, &optlen);
+                                &optcode, &optlen, (pcapng_hdr_t *) packet->buffer);
                 if (optval == NULL) {
                         trace_set_err(libtrace, TRACE_ERR_BAD_PACKET,
@@ -1055 +1080 @@
                 if (DATA(libtrace)->byteswapped) {
                         btype = byteswap32(peeker.blocktype);
+                        to_read = byteswap32(peeker.blocklen);
                 } else {
                         btype = peeker.blocktype;
+                        to_read = peeker.blocklen;
+                }
+
+                // Check we won't read off the end of the packet buffer. Assuming corruption.
+                // Exclude the SECTION header, as this is used to identify the byteorder
+                if (to_read > LIBTRACE_PACKET_BUFSIZE && btype != PCAPNG_SECTION_TYPE) {
+                        trace_set_err(libtrace, TRACE_ERR_BAD_PACKET,
+                                      "Oversized pcapng block found, is the trace corrupted?");
+                        return -1;
                 }
 
@@ -1106 +1141 @@
                         /* Everything else -- don't care, skip it */
                         default:
-                                if (DATA(libtrace)->byteswapped) {
-                                        to_read = byteswap32(peeker.blocklen);
-                                } else {
-                                        to_read = peeker.blocklen;
-                                }
                                 err = skip_block(libtrace, to_read);
                                 break;