Pull request #3872: libasan: fix out-of-bounds issues

Merge in SNORT/snort3 from ~ANOROKH/snort3:asan_invest to master

Squashed commit of the following:

commit 10d928de831b99b2fc6063cf5dc640dc83c4f5b6
Author: Anna Norokh <anorokh@cisco.com>
Date:   Mon May 29 11:31:43 2023 +0300

    analyzer: poison memory segment after msg->data

    This will work only for regtests, memory will be poisoned for 16 bytes
    to provide libasan possibility to sanitize memory that was allocated in DAQ.

commit 11e64eabf0d8fe3845f8cc3e85d040537ddf9103
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 22:31:03 2023 +0300

    log: fix out-of-bounds read access

    The source structure is over the packet raw data.
    The structure declares an array of maximum possible size.
    The default assign/copy operator may go out of bounds if underlying raw data is shorter.

commit dc558bab687ffc779af2ca285240aa34ceb8c2a2
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 15:39:19 2023 +0300

    codecs: fix tcp options parsing

commit bda86b5636c95909ed151c013adc481edde815f8
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 14:51:25 2023 +0300

    codecs: fix ipv6_mobility parsing

    Check data availability before accessing the structure.

commit d3d9b96e273c130e53637246d07ae367912719ff
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 14:39:33 2023 +0300

    appid: fix FTP parsing

    Prevent offset going beyond the buffer.

commit 6bbb52ff4333c6f0222d6fb05e6ac736d93b5a86
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 13:12:55 2023 +0300

    rna: fix icmpv6 decoding

    IPv6 payload length may include extenstion headers,
    which should be accounted when looking for the end of ICMPv6.

commit 91f70f976963b9229259f11fabd561fcf5c5c269
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 24 09:22:29 2023 +0300

    netflow: fix raw data conversion

    Netflow dedicates 4 bytes for a time record.
    Field size is better to be compared to the type size directly rather than
    to an external variable size.

commit 761afb8d664b7314c4225a3699f1b0bfe95bde3f
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Fri May 19 15:58:56 2023 +0300

    utils: fix out-of-bound access

    Before the change the function accepted a limit for the destination buffer,
    which may cause out-of-bounds reading from the source buffer.

commit e936d5b47d672e7ac7f6c03afdd55af0d34e04a7
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Thu May 4 13:54:05 2023 +0300

    appid: check size boundaries before header validation

commit 3708040ec8e130a365cff68b25fb2776db3ae98c
Author: Oleksii Shumeiko <oshumeik@cisco.com>
Date:   Wed May 3 14:56:40 2023 +0300

    protocols: remove of unnecessary old_opt check

diff --git a/src/codecs/ip/cd_mobility.cc b/src/codecs/ip/cd_mobility.cc
index 8cd19a831d5b988faea0ce68bb8c5e97c55ebf1e..292d9d5359a0a27e96813fed8fb5d1910547d9bb 100644 (file)
--- a/src/codecs/ip/cd_mobility.cc
+++ b/src/codecs/ip/cd_mobility.cc
@@ -66,7 +66,14 @@ bool MobilityCodec::decode(const RawData& raw, CodecData& codec, DecodeData&)
         return false;
     }
 
+    if ( raw.len < ip::MIN_EXT_LEN )
+    {
+        codec_event(codec, DECODE_IPV6_TRUNCATED_EXT);
+        return false;
+    }
+
     codec.lyr_len = ip::MIN_EXT_LEN + (mip6->header_len << 3);
+
     if (codec.lyr_len > raw.len)
     {
         codec_event(codec, DECODE_IPV6_TRUNCATED_EXT);

diff --git a/src/codecs/ip/cd_tcp.cc b/src/codecs/ip/cd_tcp.cc
index 90442b0a73ac5e1b9a6c4ecc9fa7a7d564b88811..5df64e66eeac1fd9689c5e983ae877d5249246f7 100644 (file)
--- a/src/codecs/ip/cd_tcp.cc
+++ b/src/codecs/ip/cd_tcp.cc
@@ -136,6 +136,8 @@ private:
 
     int validate_option(const tcp::TcpOption* const opt,
         const uint8_t* const end, const int expected_len);
+    int validate_option(const tcp::TcpOption* const opt,
+        const uint8_t* const end);
 
     void decode_options(const uint8_t*, uint32_t, CodecData&, DecodeData&);
 
@@ -394,7 +396,7 @@ void TcpCodec::decode_options(
             break;
 
         case tcp::TcpOptCode::AUTH:
-            code = validate_option(opt, end_ptr, -1);
+            code = validate_option(opt, end_ptr);
 
             /* Has to have at least 4 bytes - see RFC 5925, Section 2.2 */
             if (code >= 0 && opt->len < 4)
@@ -402,7 +404,7 @@ void TcpCodec::decode_options(
             break;
 
         case tcp::TcpOptCode::SACK:
-            code = validate_option(opt, end_ptr, -1);
+            code = validate_option(opt, end_ptr);
 
             if ((code >= 0) && (opt->len < 2))
                 code = tcp::OPT_BADLEN;
@@ -431,7 +433,7 @@ void TcpCodec::decode_options(
         case tcp::TcpOptCode::BUBBA:
         case tcp::TcpOptCode::UNASSIGNED:
             obsolete_option_found = true;
-            code = validate_option(opt, end_ptr, -1);
+            code = validate_option(opt, end_ptr);
             break;
 
         case tcp::TcpOptCode::SCPS:
@@ -444,7 +446,7 @@ void TcpCodec::decode_options(
         case tcp::TcpOptCode::SNAP:
         default:
             experimental_option_found = true;
-            code = validate_option(opt, end_ptr, -1);
+            code = validate_option(opt, end_ptr);
             break;
         }
 
@@ -479,31 +481,33 @@ void TcpCodec::decode_options(
 }
 
 int TcpCodec::validate_option(const tcp::TcpOption* const opt,
-    const uint8_t* const end,
-    const int expected_len)
+    const uint8_t* const end, const int expected_len)
 {
-    // case for pointer arithmetic
     const uint8_t* const opt_ptr = reinterpret_cast<const uint8_t*>(opt);
 
-    if (expected_len > 1)
-    {
-        /* not enough data to read in a perfect world */
-        if ((opt_ptr + expected_len) > end)
-            return tcp::OPT_TRUNC;
+    assert(expected_len > 1);
 
-        if (opt->len != expected_len)
-            return tcp::OPT_BADLEN;
-    }
-    else /* expected_len < 0 (i.e. variable length) */
-    {
-        /* RFC says that we MUST have at least this much data */
-        if (opt->len < 2)
-            return tcp::OPT_BADLEN;
+    /* not enough data to read in a perfect world */
+    if ((opt_ptr + expected_len) > end)
+        return tcp::OPT_TRUNC;
 
-        /* not enough data to read in a perfect world */
-        if ((opt_ptr + opt->len) > end)
-            return tcp::OPT_TRUNC;
-    }
+    if (opt->len != expected_len)
+        return tcp::OPT_BADLEN;
+
+    return 0;
+}
+
+int TcpCodec::validate_option(const tcp::TcpOption* const opt, const uint8_t* const end)
+{
+    const uint8_t* const opt_ptr = reinterpret_cast<const uint8_t*>(opt);
+
+    /* not enough data to read in a perfect world */
+    if ((opt_ptr + 2) > end or (opt_ptr + opt->len) > end)
+        return tcp::OPT_TRUNC;
+
+    /* RFC says that we MUST have at least this much data */
+    if (opt->len < 2)
+        return tcp::OPT_BADLEN;
 
     return 0;
 }

diff --git a/src/log/log_text.cc b/src/log/log_text.cc
index f01ea3643a779477fe2aa56d9510f603e46235bd..fbcf42137ca0d3a0bcc57505a5906dfc95af272e 100644 (file)
--- a/src/log/log_text.cc
+++ b/src/log/log_text.cc
@@ -178,7 +178,7 @@ static void LogIpOptions(TextLog* log, const ip::IpOptionIterator& options)
 
     TextLog_Print(log, "IP Options (%u) => ", c);
 
-    for (auto op : options)
+    for (const auto& op : options)
     {
         switch (op.code)
         {

diff --git a/src/main/analyzer.cc b/src/main/analyzer.cc
index 68496cb15fa158ef43064736ff273067087412ef..3dc861136fabc0c3b99e3e30b77a36115a85c074 100644 (file)
--- a/src/main/analyzer.cc
+++ b/src/main/analyzer.cc
@@ -21,11 +21,14 @@
 #ifdef HAVE_CONFIG_H
 #include "config.h"
 #endif
-
+ 
 #include "analyzer.h"
 
 #include <daq.h>
 
+#if 0 // defined (__SANITIZE_ADDRESS__) && defined (REG_TEST)
+    #include <sanitizer/asan_interface.h>
+#endif
 #include <thread>
 
 #include "detection/context_switcher.h"
@@ -413,7 +416,18 @@ void Analyzer::process_daq_pkt_msg(DAQ_Msg_h msg, bool retry)
     p->daq_msg = msg;
     p->daq_instance = daq_instance;
 
-    PacketManager::decode(p, pkthdr, daq_msg_get_data(msg), daq_msg_get_data_len(msg), false, retry);
+    const uint8_t* data = daq_msg_get_data(msg);
+    const uint32_t data_len = daq_msg_get_data_len(msg);
+
+#if 0 // defined (__SANITIZE_ADDRESS__) && defined (REG_TEST)
+    auto data_end = msg->data + msg->data_len;
+    unsigned long dist = static_cast<uint8_t*>(msg->hdr) - data_end;
+    auto size = std::min(16UL, dist);
+
+    ASAN_POISON_MEMORY_REGION(data_end, size);
+#endif
+
+    PacketManager::decode(p, pkthdr, data, data_len, false, retry);
 
     if (process_packet(p))
     {
@@ -421,6 +435,9 @@ void Analyzer::process_daq_pkt_msg(DAQ_Msg_h msg, bool retry)
         switcher->stop();
     }
 
+#if 0 // defined (__SANITIZE_ADDRESS__) && defined (REG_TEST)
+    ASAN_UNPOISON_MEMORY_REGION(data_end, size);
+#endif
     // Beyond this point, we don't have an active context, but e.g. calls to
     // get_current_packet() or get_current_wire_packet() require a context.
     // We must ensure that a context is available when one is needed.

diff --git a/src/network_inspectors/appid/detector_plugins/detector_dns.cc b/src/network_inspectors/appid/detector_plugins/detector_dns.cc
index c071f232a9eed7bd403ab6c7494605ae16bf32c7..ca6010d1a35bbe6ca37daff1b42bca2512d472bc 100644 (file)
--- a/src/network_inspectors/appid/detector_plugins/detector_dns.cc
+++ b/src/network_inspectors/appid/detector_plugins/detector_dns.cc
@@ -628,6 +628,10 @@ int DnsTcpServiceDetector::validate(AppIdDiscoveryArgs& args)
         const uint8_t* data = args.data + sizeof(DNSTCPHeader);
         uint16_t size = args.size - sizeof(DNSTCPHeader);
         uint16_t tmp = ntohs(hdr->length);
+
+        if (tmp > size)
+            goto not_compatible;
+
         if (tmp < sizeof(DNSHeader) || dns_validate_header(args.dir, (const DNSHeader*)data,
             args.asd.get_odp_ctxt().dns_host_reporting, args.asd))
         {
@@ -637,8 +641,6 @@ int DnsTcpServiceDetector::validate(AppIdDiscoveryArgs& args)
                 goto fail;
         }
 
-        if (tmp > size)
-            goto not_compatible;
         rval = validate_packet(data, size, args.dir,
             args.asd.get_odp_ctxt().dns_host_reporting, args.asd, args.change_bits);
         if (rval != APPID_SUCCESS)

diff --git a/src/network_inspectors/appid/service_plugins/service_ftp.cc b/src/network_inspectors/appid/service_plugins/service_ftp.cc
index d639a09f8e86498adf96fe8754a806b8bac87b29..cac14d9b69538af949aa58515892359970a50e52 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/service_ftp.cc
+++ b/src/network_inspectors/appid/service_plugins/service_ftp.cc
@@ -533,6 +533,7 @@ static int ftp_validate_reply(const uint8_t* data, uint16_t& offset, uint16_t si
                     return -1;
                 if (++offset >= size)
                 {
+                    offset = size;
                     fd.rstate = FTP_REPLY_BEGIN;
                     break;
                 }

diff --git a/src/network_inspectors/rna/rna_pnd.cc b/src/network_inspectors/rna/rna_pnd.cc
index f54938c49f53d6d28ea5e9b2427ff483344ce50a..36428520d6e71ec2646e0e63b46b27b7d80bfd59 100644 (file)
--- a/src/network_inspectors/rna/rna_pnd.cc
+++ b/src/network_inspectors/rna/rna_pnd.cc
@@ -1042,7 +1042,7 @@ int RnaPnd::discover_host_types_icmpv6_ndp(RnaTracker& ht, const Packet* p, uint
         return 1;
 
     const uint8_t* data = (const uint8_t*)p->ptrs.icmph;
-    int32_t data_len = p->ptrs.ip_api.pay_len();
+    int32_t data_len = std::min((int)p->ptrs.ip_api.pay_len(), (int)(p->pkt + p->pktlen - data));
 
     switch ( ((const icmp::Icmp6Hdr*)p->ptrs.icmph)->type )
     {

diff --git a/src/protocols/tcp_options.cc b/src/protocols/tcp_options.cc
index 546a20f2f6c7a276ba43486966935903899d8516..af994e460e59a9dc6a8283ab63bab25b5660a836 100644 (file)
--- a/src/protocols/tcp_options.cc
+++ b/src/protocols/tcp_options.cc
@@ -40,13 +40,11 @@ const TcpOption& TcpOptIteratorIter::operator*() const { return *opt; }
 
 const TcpOptIteratorIter& TcpOptIteratorIter::operator++()
 {
-    const auto* old_opt = opt;
-    opt = &opt->next();
-    if (opt == old_opt or opt->code == TcpOptCode::EOL)       // defend against option length = 0
-    {
+    if (opt->code != TcpOptCode::EOL)
+        opt = &opt->next();
+    else
         *this = iter->end();
-        tcpStats.zero_len_tcp_opt++;
-    }
+
     return *this;
 }
 

diff --git a/src/service_inspectors/netflow/netflow.cc b/src/service_inspectors/netflow/netflow.cc
index c2d3e3ffeabbc6d5db998f6dfc70b23c66f5b41a..6372ab5afe00ecc908d1a9b468789efff910ef79 100644 (file)
--- a/src/service_inspectors/netflow/netflow.cc
+++ b/src/service_inspectors/netflow/netflow.cc
@@ -236,10 +236,10 @@ static bool version_9_record_update(const unsigned char* data, uint32_t unix_sec
 
         case NETFLOW_LAST_PKT:
 
-            if( field_length != sizeof(record.last_pkt_second) )
+            if (field_length != sizeof(uint32_t))
                 return false;
 
-            last_pkt_time = ntohl(*(const time_t*)data)/1000;
+            last_pkt_time = ntohl(*(const uint32_t*)data)/1000;
             // last_pkt_time (LAST_SWITCHED) is defined as the system uptime
             // at which the flow was seen. If this is >= to the current uptime
             // something has gone wrong - use the NetFlow header unix time instead.
@@ -257,10 +257,10 @@ static bool version_9_record_update(const unsigned char* data, uint32_t unix_sec
 
         case NETFLOW_FIRST_PKT:
 
-            if( field_length != sizeof(record.first_pkt_second) )
+            if (field_length != sizeof(uint32_t))
                 return false;
 
-            first_pkt_time = ntohl(*(const time_t*)data)/1000;
+            first_pkt_time = ntohl(*(const uint32_t*)data)/1000;
             if (first_pkt_time >= sys_uptime)
                 record.first_pkt_second = unix_secs;
             else

diff --git a/src/utils/util.cc b/src/utils/util.cc
index e3126f5b37f2fa4e7ad86628707b12b630309edf..3652515ef779951e9a3315fac001ab3e59c2222e 100644 (file)
--- a/src/utils/util.cc
+++ b/src/utils/util.cc
@@ -505,17 +505,10 @@ const char* get_error(int errnum)
 #endif
 }
 
-char* snort_strndup(const char* src, size_t dst_size)
+char* snort_strndup(const char* src, size_t n)
 {
-    char* dup = (char*)snort_calloc(dst_size + 1);
-
-    if ( SnortStrncpy(dup, src, dst_size + 1) == SNORT_STRNCPY_ERROR )
-    {
-        snort_free(dup);
-        return nullptr;
-    }
-
-    return dup;
+    char* dst = (char*)snort_calloc(n + 1);
+    return strncpy(dst, src, n);
 }
 
 char* snort_strdup(const char* str)