diff --git a/CHANGES b/CHANGES index eea83abaff..f2f1786452 100644 --- a/CHANGES +++ b/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) diff --git a/NEWS b/NEWS index adad0e09e9..d80fb54bf9 100644 --- a/NEWS +++ b/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 --------------------- diff --git a/VERSION b/VERSION index ddda1e637c..3adee24a00 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -8.0.0-dev.605 +8.0.0-dev.609 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 0000000000..b50ebd3492 Binary files /dev/null and b/testing/btest/Traces/tls/tls-1.2-protocol-error.pcap differ 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)