From aa79356963c10140a9e120162b43b884c0d46d69 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 18 Aug 2022 10:07:55 -0700 Subject: [PATCH] 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 )