Mirror/zeek
1
0
Fork 0
mirror of https://github.com/zeek/zeek.git synced 2025-10-02 06:38:20 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

PPPoE: don't forward more bytes than header indicates

Browse source 
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
This commit is contained in:
Johanna Amann 2025-07-02 16:21:19 +01:00
parent d42d467965
commit 1fed0ed58d
6 changed files with 20 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

15
src/packet_analysis/protocol/pppoe/PPPoE.cc
View file

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

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

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

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

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

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

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

BIN
testing/btest/Traces/tls/tls-1.2-protocol-error.pcap Normal file
View file

Binary file not shown.

4
testing/btest/core/analyzer-confirmation-violation-info.zeek
View file

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