Merge pull request #1744 in SNORT/snort3 from ~BRASTULT/snort3:ber_fix to master

Squashed commit of the following:

commit c365ed5d5002bd72805b213179b379a536595dfa
Author: Brandon Stultz <brastult@cisco.com>
Date:   Fri Sep 13 15:29:17 2019 -0400

    utils: prevent integer overflow/underflow when reading BER elements

diff --git a/src/ips_options/ips_ber_data.cc b/src/ips_options/ips_ber_data.cc
index f483fef73506aa7db146d91e2c0292c62ae993f1..8f2e46edcc4ceaba1e8cacc8a7b7a53484cd09ac 100644 (file)
--- a/src/ips_options/ips_ber_data.cc
+++ b/src/ips_options/ips_ber_data.cc
@@ -89,7 +89,10 @@ IpsOption::EvalStatus BerDataOption::eval(Cursor& c, Packet*)
     if ( e.type != type )
         return NO_MATCH;
 
-    if ( !c.add_pos(e.total_length - e.length) )
+    if ( e.header_length > c.length() )
+        return NO_MATCH;
+
+    if ( !c.add_pos(e.header_length) )
         return NO_MATCH;
 
     return MATCH;

diff --git a/src/ips_options/ips_ber_skip.cc b/src/ips_options/ips_ber_skip.cc
index b95e678e41af88500372d7bbfcdeac387b8a4615..7fad9a1ef6ae741aa36dc81618e05c32e32c9f2e 100644 (file)
--- a/src/ips_options/ips_ber_skip.cc
+++ b/src/ips_options/ips_ber_skip.cc
@@ -98,6 +98,9 @@ IpsOption::EvalStatus BerSkipOption::eval(Cursor& c, Packet*)
             return NO_MATCH;
     }
 
+    if ( e.total_length > c.length() )
+        return NO_MATCH;
+
     if ( !c.add_pos(e.total_length) )
         return NO_MATCH;
 

diff --git a/src/utils/util_ber.cc b/src/utils/util_ber.cc
index 9b79157b8cca29b2c678b5f83dfdd7f16c07f757..598d82914e67357fc67cf8abcb9ae55752c8f311 100644 (file)
--- a/src/utils/util_ber.cc
+++ b/src/utils/util_ber.cc
@@ -32,8 +32,12 @@ bool BerReader::read_int(uint32_t size, uint32_t& intval)
 
     intval = 0;
 
+    // cursor must be valid
+    if ( cursor < beg || cursor > end )
+        return false;
+
     // check if we can read int data
-    if ( cursor + size > end )
+    if ( size > end - cursor )
         return false;
 
     for ( unsigned i = 0; i < size; i++ )
@@ -63,7 +67,8 @@ bool BerReader::read_type(uint32_t& type)
 
     type = 0;
 
-    if ( cursor + 1 > end )
+    // cursor must be valid
+    if ( cursor < beg || cursor + 1 > end )
         return false;
 
     b = *cursor++;
@@ -110,7 +115,8 @@ bool BerReader::read_length(uint32_t& length)
 
     length = 0;
 
-    if ( cursor + 1 > end )
+    // cursor must be valid
+    if ( cursor < beg || cursor + 1 > end )
         return false;
 
     b = *cursor++;
@@ -138,9 +144,6 @@ bool BerReader::read(const uint8_t* c, BerElement& e)
 {
     const uint8_t* start = c;
 
-    if ( c < beg || c > end )
-        return false;
-
     cursor = c;
 
     if ( !read_type(e.type) )
@@ -152,15 +155,19 @@ bool BerReader::read(const uint8_t* c, BerElement& e)
     // set BER data pointer
     e.data = cursor;
 
-    // jump BER data
-    cursor += e.length;
-
-    // cursor must be > start
-    if ( cursor <= start )
+    // integer underflow check
+    if ( start > cursor )
         return false;
 
+    // calculate BER header length
+    e.header_length = cursor - start;
+
     // calculate total BER length
-    e.total_length = cursor - start;
+    e.total_length = e.header_length + e.length;
+
+    // integer overflow check
+    if ( e.total_length < e.header_length )
+        return false;
 
     return true;
 }
@@ -170,9 +177,6 @@ bool BerReader::convert(const BerElement& e, uint32_t& intval)
     if ( e.type != BerType::INTEGER )
         return false;
 
-    if ( e.data < beg || e.data > end )
-        return false;
-
     // set cursor to int data
     cursor = e.data;
 
@@ -189,12 +193,12 @@ bool BerReader::extract(const uint8_t*& c, uint32_t& intval)
     if ( !read(c, e) )
         return false;
 
-    // save end of element position
-    c = cursor;
-
     if ( !convert(e, intval) )
         return false;
 
+    // save end of element position
+    c = cursor;
+
     return true;
 }
 
@@ -208,6 +212,18 @@ bool BerReader::skip(const uint8_t*& c, uint32_t type)
     if ( e.type != type )
         return false;
 
+    // integer underflow check
+    if ( cursor > end )
+        return false;
+
+    // check if we can jump BER data
+    if ( e.length > end - cursor )
+        return false;
+
+    // jump BER data
+    cursor += e.length;
+
+    // save end of element position
     c = cursor;
 
     return true;

diff --git a/src/utils/util_ber.h b/src/utils/util_ber.h
index ccd9b9de3739c6661e033bf5773ac7314a296604..d3f0799f0190c09905e0b8ebff5d0ed3d14c3073 100644 (file)
--- a/src/utils/util_ber.h
+++ b/src/utils/util_ber.h
@@ -38,7 +38,8 @@ struct BerElement
 {
     uint32_t type;
     uint32_t length;
-    uint32_t total_length;
+    uint64_t header_length;
+    uint64_t total_length;
     const uint8_t* data;
 };
 
@@ -48,6 +49,7 @@ public:
     BerReader(const Cursor& c)
     {
         beg = c.buffer();
+        cursor = c.start();
         end = c.endo();
     }