From 1fed0ed58d4cb196d71f2220e395be3c3e2bb88f Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Wed, 2 Jul 2025 16:21:19 +0100 Subject: [PATCH] PPPoE: don't forward more bytes than header indicates 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. Two tests needed to be updated - with this change, the traffic in pppoe-over-qinq.pcap is now valid TLS. A new trace was introduced for the confirmation-violation-info test. Addresses GH-4602 --- src/packet_analysis/protocol/pppoe/PPPoE.cc | 15 +++++++++++++-- .../.stdout | 4 ++-- .../.stdout | 4 ++-- .../btest/Baseline/core.pppoe-over-qinq/conn.log | 2 +- .../btest/Traces/tls/tls-1.2-protocol-error.pcap | Bin 0 -> 1131 bytes .../analyzer-confirmation-violation-info.zeek | 4 ++-- 6 files changed, 20 insertions(+), 9 deletions(-) create mode 100644 testing/btest/Traces/tls/tls-1.2-protocol-error.pcap diff --git a/src/packet_analysis/protocol/pppoe/PPPoE.cc b/src/packet_analysis/protocol/pppoe/PPPoE.cc index cb416f1bac..ac99dbf005 100644 --- a/src/packet_analysis/protocol/pppoe/PPPoE.cc +++ b/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]; + + // 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(len - 8, data + 8, packet, protocol); + return ForwardPacket(std::min(payload_length, len - 8), data + 8, packet, protocol); } diff --git a/testing/btest/Baseline.zam/core.analyzer-confirmation-violation-info/.stdout b/testing/btest/Baseline.zam/core.analyzer-confirmation-violation-info/.stdout index 329c1b0aa1..0402878fc5 100644 --- a/testing/btest/Baseline.zam/core.analyzer-confirmation-violation-info/.stdout +++ b/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_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_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=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3 diff --git a/testing/btest/Baseline/core.analyzer-confirmation-violation-info/.stdout b/testing/btest/Baseline/core.analyzer-confirmation-violation-info/.stdout index b7d9aa18eb..e2eab4cd37 100644 --- a/testing/btest/Baseline/core.analyzer-confirmation-violation-info/.stdout +++ b/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_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_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=192.168.4.149, orig_p=53525/tcp, resp_h=74.125.239.37, resp_p=443/tcp, proto=6, ctx=[]], 3 diff --git a/testing/btest/Baseline/core.pppoe-over-qinq/conn.log b/testing/btest/Baseline/core.pppoe-over-qinq/conn.log index 4225bae72f..344027d6bb 100644 --- a/testing/btest/Baseline/core.pppoe-over-qinq/conn.log +++ b/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 diff --git a/testing/btest/Traces/tls/tls-1.2-protocol-error.pcap b/testing/btest/Traces/tls/tls-1.2-protocol-error.pcap new file mode 100644 index 0000000000000000000000000000000000000000..b50ebd3492aa8558739d9bdfb353acdf63e62dfa GIT binary patch literal 1131 zcmaKqO=uHQ6ot=wZ!*S7tNAf$Ou+`FnqovuM-(G8nh0%$fGCKf3l)@B#Fe;FiQqy} z!HSKDViZBEV3DG3st{WXMG&miE(90;U_r$!3<`qAdndFtnb3g?^WMz+?s@0j@!^4+ z5*ntSQV9^WcpVCC?SJb+n%=pv^z`KUlYQa+?Ju@Cu?a|w??@n_A2^Zk(|S9P7n;ZX zVlWlzy1+j_kfqY8Bp@}%L!p!=gwu#0{WZzoG+Cz0KQ46Anybt$_ps>ejc34W{a7QD zxq}!S^S{qcET;}VH@vr_XE$5)tR|htn~{kQrc)=Kb?+teMxt`rberwlS@(}~Rl0kK z9?iJJv9_<|Kv{2r$xJ-_*M%a%xNOAFO;4HFc%&vV7>-?fR7ngdf%|0gU8U~bdi=3D zcWux2mUo{b;X7-tTwZbTXK3il+MO3izwPc{33&7Fyq@>xU3q~xB!yFwboLNj&=iD( z0TVIILp>JKULAs%OM3w{zzH35umFC_*8wklbXY=RKHTtN30P}|dGP~vB#l%njnt~PmgxZ~vRo$md^2|1iR)f6Q8d&bnier)Cw`cfxFN&D-}`D3 kJ*07&o!Awb(P#_EG)62Ro2q;?$;Xrx`IBiMw^8K9e-cDWA^-pY literal 0 HcmV?d00001 diff --git a/testing/btest/core/analyzer-confirmation-violation-info.zeek b/testing/btest/core/analyzer-confirmation-violation-info.zeek index 2dbbfa0056..7dc764a203 100644 --- a/testing/btest/core/analyzer-confirmation-violation-info.zeek +++ b/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)