From 40b1452905c0eed4d96300ce1aaf87a08166e396 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 18 Aug 2022 10:01:49 -0700 Subject: [PATCH 1/2] Remove reporter warning for bad IP protocols It turns out that this can be *very* spammy on networks where we're receiving lots of these packets, and can fill up the reporter log very quickly. Weirds are already reported in all of these cases anyways, so it doesn't make sense to log a reporter warning too. --- src/packet_analysis/protocol/ip/IP.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/packet_analysis/protocol/ip/IP.cc b/src/packet_analysis/protocol/ip/IP.cc index 067746dfa6..75e83c8048 100644 --- a/src/packet_analysis/protocol/ip/IP.cc +++ b/src/packet_analysis/protocol/ip/IP.cc @@ -310,8 +310,7 @@ int zeek::packet_analysis::IP::ParsePacket(int caplen, const u_char* const pkt, else { - zeek::reporter->InternalWarning("Bad IP protocol version in IP::ParsePacket"); - return -1; + return -2; } if ( (uint32_t)caplen != inner->TotalLen() ) From aa79356963c10140a9e120162b43b884c0d46d69 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 18 Aug 2022 10:07:55 -0700 Subject: [PATCH 2/2] Make result of IP::ParsePacket easier to understand --- NEWS | 6 ++++ src/packet_analysis/protocol/gtpv1/GTPv1.cc | 8 +++--- src/packet_analysis/protocol/ip/IP.cc | 20 ++++++------- src/packet_analysis/protocol/ip/IP.h | 28 +++++++++++++------ .../protocol/iptunnel/IPTunnel.cc | 10 +++---- src/packet_analysis/protocol/teredo/Teredo.cc | 23 ++++++++------- 6 files changed, 56 insertions(+), 39 deletions(-) diff --git a/NEWS b/NEWS index 4ebf50fe6a..56017ce6de 100644 --- a/NEWS +++ b/NEWS @@ -54,6 +54,12 @@ Breaking Changes - The unified2 analyzer and accompanying scripts have been removed without deprecation. +- The return value of ``packet_analysis::IP::ParsePacket`` has changed to return + enum values. This makes it easier to look at the result and immediately know + what it means. Unfortunately, because we can't overload a method on the return + value alone, we aren't able to deprecate the original version of the method. + This may cause build of packages to fail if they were using this method. + New Functionality ----------------- diff --git a/src/packet_analysis/protocol/gtpv1/GTPv1.cc b/src/packet_analysis/protocol/gtpv1/GTPv1.cc index a382317a57..5b063c5398 100644 --- a/src/packet_analysis/protocol/gtpv1/GTPv1.cc +++ b/src/packet_analysis/protocol/gtpv1/GTPv1.cc @@ -55,9 +55,9 @@ bool GTPv1_Analyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack // but on the other hand we duplicate this work here. maybe this header could just be stored // and reused in the IP analyzer somehow? std::shared_ptr inner = nullptr; - int result = packet_analysis::IP::ParsePacket(len, data, next_header, inner); + auto result = packet_analysis::IP::ParsePacket(len, data, next_header, inner); - if ( result == 0 ) + if ( result == packet_analysis::IP::ParseResult::Ok ) { cm_it->second->set_valid(packet->is_orig, true); @@ -72,13 +72,13 @@ bool GTPv1_Analyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack gtp_hdr_val = nullptr; } } - else if ( result == -2 ) + else if ( result == packet_analysis::IP::ParseResult::BadProtocol ) { AnalyzerViolation("Invalid IP version in wrapped packet", packet->session); gtp_hdr_val = nullptr; return false; } - else if ( result < 0 ) + else if ( result == packet_analysis::IP::ParseResult::CaplenTooSmall ) { AnalyzerViolation("Truncated GTPv1", packet->session); gtp_hdr_val = nullptr; diff --git a/src/packet_analysis/protocol/ip/IP.cc b/src/packet_analysis/protocol/ip/IP.cc index 75e83c8048..e739cc3e31 100644 --- a/src/packet_analysis/protocol/ip/IP.cc +++ b/src/packet_analysis/protocol/ip/IP.cc @@ -283,38 +283,38 @@ bool IPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) return return_val; } -int zeek::packet_analysis::IP::ParsePacket(int caplen, const u_char* const pkt, int proto, - std::shared_ptr& inner) +ParseResult zeek::packet_analysis::IP::ParsePacket(int caplen, const u_char* const pkt, int proto, + std::shared_ptr& inner) { if ( proto == IPPROTO_IPV6 ) { if ( caplen < (int)sizeof(struct ip6_hdr) ) - return -1; + return ParseResult::CaplenTooSmall; const struct ip6_hdr* ip6 = (const struct ip6_hdr*)pkt; inner = std::make_shared(ip6, false, caplen); if ( (ip6->ip6_ctlun.ip6_un2_vfc & 0xF0) != 0x60 ) - return -2; + return ParseResult::BadProtocol; } else if ( proto == IPPROTO_IPV4 ) { if ( caplen < (int)sizeof(struct ip) ) - return -1; + return ParseResult::BadProtocol; const struct ip* ip4 = (const struct ip*)pkt; inner = std::make_shared(ip4, false); if ( ip4->ip_v != 4 ) - return -2; + return ParseResult::BadProtocol; } - else { - return -2; + return ParseResult::BadProtocol; } if ( (uint32_t)caplen != inner->TotalLen() ) - return (uint32_t)caplen < inner->TotalLen() ? -1 : 1; + return (uint32_t)caplen < inner->TotalLen() ? ParseResult::CaplenTooSmall + : ParseResult::CaplenTooLarge; - return 0; + return ParseResult::Ok; } diff --git a/src/packet_analysis/protocol/ip/IP.h b/src/packet_analysis/protocol/ip/IP.h index 9aa00d99f3..24e86565ec 100644 --- a/src/packet_analysis/protocol/ip/IP.h +++ b/src/packet_analysis/protocol/ip/IP.h @@ -35,6 +35,14 @@ private: zeek::detail::Discarder* discarder = nullptr; }; +enum class ParseResult + { + Ok = 0, + CaplenTooSmall = -1, + BadProtocol = -2, + CaplenTooLarge = 1 + }; + /** * Returns a wrapper IP_Hdr object if \a pkt appears to be a valid IPv4 * or IPv6 header based on whether it's long enough to contain such a header, @@ -49,13 +57,17 @@ private: * @param inner The inner IP packet wrapper pointer to be allocated/assigned * if \a pkt looks like a valid IP packet or at least long enough * to hold an IP header. - * @return 0 If the inner IP packet appeared valid, else -1 if \a caplen - * is greater than the supposed IP packet's payload length field, -2 - * if the version of the inner header does not match proto or - * 1 if \a caplen is less than the supposed packet's payload length. - * In the -1 case, \a inner may still be non-null if \a caplen was - * long enough to be an IP header, and \a inner is always non-null - * for other return values. + * @return ParseResult::Ok if the inner IP packet appeared valid. + * ParseResult::CaplenTooSmall if \a caplen is greater than the + * supposed packet's payload length field. \a inner may still be + * non-null if \a caplen is too small but still large enough to + * be an IP header. ParseResult::CaplenTooLarge if \a caplen is + * larger than the supposed packet's payload length field. + * ParseResult::BadProtocol if either \a proto isn't IPPROTO_IPV4 + * or IPPROTO_IPV6 or if \a proto does not match the protocol + * in the header's version field. */ -int ParsePacket(int caplen, const u_char* const pkt, int proto, std::shared_ptr& inner); +ParseResult ParsePacket(int caplen, const u_char* const pkt, int proto, + std::shared_ptr& inner); + } diff --git a/src/packet_analysis/protocol/iptunnel/IPTunnel.cc b/src/packet_analysis/protocol/iptunnel/IPTunnel.cc index b16b2dd00c..f1e44b19a4 100644 --- a/src/packet_analysis/protocol/iptunnel/IPTunnel.cc +++ b/src/packet_analysis/protocol/iptunnel/IPTunnel.cc @@ -50,15 +50,15 @@ bool IPTunnelAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pa if ( gre_version != 0 ) { // Check for a valid inner packet first. - int result = packet_analysis::IP::ParsePacket(len, data, proto, inner); - if ( result == -2 ) + auto result = packet_analysis::IP::ParsePacket(len, data, proto, inner); + if ( result == packet_analysis::IP::ParseResult::BadProtocol ) Weird("invalid_inner_IP_version", packet); - else if ( result < 0 ) + else if ( result < packet_analysis::IP::ParseResult::CaplenTooSmall ) Weird("truncated_inner_IP", packet); - else if ( result > 0 ) + else if ( result > packet_analysis::IP::ParseResult::CaplenTooLarge ) Weird("inner_IP_payload_length_mismatch", packet); - if ( result != 0 ) + if ( result != packet_analysis::IP::ParseResult::Ok ) return false; } diff --git a/src/packet_analysis/protocol/teredo/Teredo.cc b/src/packet_analysis/protocol/teredo/Teredo.cc index d1a284d263..078294a13e 100644 --- a/src/packet_analysis/protocol/teredo/Teredo.cc +++ b/src/packet_analysis/protocol/teredo/Teredo.cc @@ -192,8 +192,8 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack // but on the other hand we duplicate this work here. maybe this header could just be stored // and reused in the IP analyzer somehow? std::shared_ptr inner = nullptr; - int rslt = packet_analysis::IP::ParsePacket(len, te.InnerIP(), IPPROTO_IPV6, inner); - if ( rslt > 0 ) + auto result = packet_analysis::IP::ParsePacket(len, te.InnerIP(), IPPROTO_IPV6, inner); + if ( result == packet_analysis::IP::ParseResult::CaplenTooLarge ) { if ( inner->NextProto() == IPPROTO_NONE && inner->PayloadLen() == 0 ) // Teredo bubbles having data after IPv6 header isn't strictly a @@ -206,22 +206,21 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack } } - if ( rslt == 0 || rslt > 0 ) - { - if ( packet->is_orig ) - or_it->second.valid_orig = true; - else - or_it->second.valid_resp = true; - - Confirm(or_it->second.valid_orig, or_it->second.valid_resp); - } - else + if ( result == packet_analysis::IP::ParseResult::CaplenTooSmall || + result == packet_analysis::IP::ParseResult::BadProtocol ) { AnalyzerViolation("Truncated Teredo or invalid inner IP version", conn, (const char*)data, len); return false; } + if ( packet->is_orig ) + or_it->second.valid_orig = true; + else + or_it->second.valid_resp = true; + + Confirm(or_it->second.valid_orig, or_it->second.valid_resp); + ValPtr teredo_hdr; if ( teredo_packet )