Merge branch 'topic/timw/parse-packet-results'

* topic/timw/parse-packet-results:
  Make result of IP::ParsePacket easier to understand
  Remove reporter warning for bad IP protocols

CHANGES

@@ -1,3 +1,20 @@
+5.1.0-dev.424 | 2022-08-22 10:56:47 -0700
+
+  * Merge branch 'topic/timw/parse-packet-results' (Tim Wojtulewicz, Corelight)
+
+    * topic/timw/parse-packet-results:
+      Make result of IP::ParsePacket easier to understand
+      Remove reporter warning for bad IP protocols
+
+  * Make result of IP::ParsePacket easier to understand (Tim Wojtulewicz, Corelight)
+
+  * Remove reporter warning for bad IP protocols (Tim Wojtulewicz, Corelight)
+
+    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.
+
 5.1.0-dev.421 | 2022-08-19 15:23:22 -0700
 
   * Remove unified2 file analyzer (Arne Welzel, Corelight)

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
 -----------------
 

VERSION

@@ -1 +1 @@
-5.1.0-dev.421
+5.1.0-dev.424

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<IP_Hdr> 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;

src/packet_analysis/protocol/ip/IP.cc

@@ -283,39 +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,
+ParseResult zeek::packet_analysis::IP::ParsePacket(int caplen, const u_char* const pkt, int proto,
                                                    std::shared_ptr<zeek::IP_Hdr>& 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<zeek::IP_Hdr>(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<zeek::IP_Hdr>(ip4, false);
 		if ( ip4->ip_v != 4 )
-			return -2;
+			return ParseResult::BadProtocol;
 		}
-
 	else
 		{
-		zeek::reporter->InternalWarning("Bad IP protocol version in IP::ParsePacket");
-		return -1;
+		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;
 	}

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<IP_Hdr>& inner);
+ParseResult ParsePacket(int caplen, const u_char* const pkt, int proto,
+                        std::shared_ptr<IP_Hdr>& inner);
+
 	}

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;
 		}
 

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<IP_Hdr> 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,21 +206,20 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack
 			}
 		}
 
-	if ( rslt == 0 || rslt > 0 )
+	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);
-		}
-	else
-		{
-		AnalyzerViolation("Truncated Teredo or invalid inner IP version", conn, (const char*)data,
-		                  len);
-		return false;
-		}
 
 	ValPtr teredo_hdr;