From: Anna Norokh -X (anorokh - SOFTSERVE INC at Cisco) Date: Wed, 21 Jun 2023 06:13:14 +0000 (+0000) Subject: Pull request #3872: libasan: fix out-of-bounds issues X-Git-Tag: 3.1.65.0~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=66c1ae477ac91a730fe86c8e6cfec502b8733c71;p=thirdparty%2Fsnort3.git 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 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 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 Date: Wed May 24 15:39:19 2023 +0300 codecs: fix tcp options parsing commit bda86b5636c95909ed151c013adc481edde815f8 Author: Oleksii Shumeiko 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 Date: Wed May 24 14:39:33 2023 +0300 appid: fix FTP parsing Prevent offset going beyond the buffer. commit 6bbb52ff4333c6f0222d6fb05e6ac736d93b5a86 Author: Oleksii Shumeiko 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 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 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 Date: Thu May 4 13:54:05 2023 +0300 appid: check size boundaries before header validation commit 3708040ec8e130a365cff68b25fb2776db3ae98c Author: Oleksii Shumeiko 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 8cd19a831..292d9d535 100644 --- 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 90442b0a7..5df64e66e 100644 --- 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(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(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 f01ea3643..fbcf42137 100644 --- 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 68496cb15..3dc861136 100644 --- 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 +#if 0 // defined (__SANITIZE_ADDRESS__) && defined (REG_TEST) + #include +#endif #include #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(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 c071f232a..ca6010d1a 100644 --- 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 d639a09f8..cac14d9b6 100644 --- 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 f54938c49..36428520d 100644 --- 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 546a20f2f..af994e460 100644 --- 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 c2d3e3ffe..6372ab5af 100644 --- 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 e3126f5b3..3652515ef 100644 --- 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)