Merge remote-tracking branch 'origin/topic/johanna/gh-4602'

* origin/topic/johanna/gh-4602:
  PPPoE: don't forward more bytes than header indicates

CHANGES

@@ -1,3 +1,15 @@
+8.0.0-dev.609 | 2025-07-08 11:42:05 +0100
+
+  * PPPoE: don't forward more bytes than header indicates (Johanna Amann, Corelight)
+
+    This changes the PPPoE parser so that it doesn't forward extra bytes
+    that might be appended after the payload. Instead, it raises a weird if
+    the payload size doesn't match the size indicated by the header.
+
+    This is in line with what other protocol parsers (like UDP) are doing.
+
+    Addresses GH-4602
+
 8.0.0-dev.605 | 2025-07-08 10:40:45 +0200
 
   * Bump ZeekJS to work with new Location constructor (Arne Welzel, Corelight)

NEWS

@@ -272,6 +272,9 @@ Changed Functionality
   Similarly, the related events and the Zeek data structures all interpreted
   times in X509 certificates as local times.
 
+- The PPPoE parser now respects the size value given in the PPPoE header. Data
+  beyon the size given in the header will be truncated.
+
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-8.0.0-dev.605
+8.0.0-dev.609

src/packet_analysis/protocol/pppoe/PPPoE.cc

@@ -12,8 +12,19 @@ bool PPPoEAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packe
         return false;
     }
 
-    // Extract protocol identifier
+    size_t payload_length = (data[4] << 8u) + data[5];
     uint32_t protocol = (data[6] << 8u) + data[7];
-    // Skip the PPPoE session and PPP header
+
-    return ForwardPacket(len - 8, data + 8, packet, protocol);
+    // PPPoE header is six bytes. The protocol identifier is not part of the header
+    if ( payload_length != len - 6 ) {
+        if ( payload_length < len - 6 )
+            Weird("pppoe_extra_data_after_payload", packet);
+        else
+            Weird("pppoe_truncated_payload");
+    }
+
+    payload_length = payload_length >= 2 ? payload_length - 2 : 0;
+
+    // Skip the PPPoE session and PPP header
+    return ForwardPacket(std::min(payload_length, len - 8), data + 8, packet, protocol);
 }

testing/btest/Baseline.zam/core.analyzer-confirmation-violation-info/.stdout

@@ -1,3 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-analyzer_confirmation_info, AllAnalyzers::ANALYZER_ANALYZER_SSL, [orig_h=1.1.1.1, orig_p=20394/tcp, resp_h=2.2.2.2, resp_p=443/tcp, proto=6, ctx=[]], 3
+analyzer_confirmation_info, AllAnalyzers::ANALYZER_ANALYZER_SSL, [orig_h=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3
-analyzer_violation_info, AllAnalyzers::ANALYZER_ANALYZER_SSL, Invalid version late in TLS connection. Packet reported version: 0, [orig_h=1.1.1.1, orig_p=20394/tcp, resp_h=2.2.2.2, resp_p=443/tcp, proto=6, ctx=[]], 3
+analyzer_violation_info, AllAnalyzers::ANALYZER_ANALYZER_SSL, Invalid version late in TLS connection. Packet reported version: 0, [orig_h=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3

testing/btest/Baseline/core.analyzer-confirmation-violation-info/.stdout

@@ -1,3 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-analyzer_confirmation_info, Analyzer::ANALYZER_SSL, [orig_h=1.1.1.1, orig_p=20394/tcp, resp_h=2.2.2.2, resp_p=443/tcp, proto=6, ctx=[]], 3
+analyzer_confirmation_info, Analyzer::ANALYZER_SSL, [orig_h=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3
-analyzer_violation_info, Analyzer::ANALYZER_SSL, Invalid version late in TLS connection. Packet reported version: 0, [orig_h=1.1.1.1, orig_p=20394/tcp, resp_h=2.2.2.2, resp_p=443/tcp, proto=6, ctx=[]], 3
+analyzer_violation_info, Analyzer::ANALYZER_SSL, Invalid version late in TLS connection. Packet reported version: 0, [orig_h=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3

testing/btest/Baseline/core.pppoe-over-qinq/conn.log

@@ -7,5 +7,5 @@
 #open XXXX-XX-XX-XX-XX-XX
 #fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents	ip_proto
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	1.1.1.1	20394	2.2.2.2	443	tcp	-	273.626833	11352	4984	SF	F	F	0	ShADdtaTTtFf	44	25283	42	13001	-	6
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	1.1.1.1	20394	2.2.2.2	443	tcp	ssl	273.626833	11352	4984	SF	F	F	0	ShADdtaTTtFf	44	25283	42	13001	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/core/analyzer-confirmation-violation-info.zeek

@@ -1,6 +1,6 @@
-# @TEST-DOC: The SSL analyzer picks up on the traffic in pppoe-over-qing, but then raises analyzer_violation_info
+# @TEST-DOC: The SSL analyzer picks up on the traffic, but then raises analyzer_violation_info
 # @TEST-REQUIRES: ! have-spicy-ssl
-# @TEST-EXEC: zeek -r $TRACES/pppoe-over-qinq.pcap %INPUT
+# @TEST-EXEC: zeek -r $TRACES/tls/tls-1.2-protocol-error.pcap %INPUT
 # @TEST-EXEC: btest-diff .stdout
 
 event analyzer_confirmation_info(tag: AllAnalyzers::Tag, info: AnalyzerConfirmationInfo)