-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[inet6] recognize unknown router advertisement options #4233
Conversation
The CI seems to be failing because cryptography switched to Rust in pyca/cryptography@ba9131e and the |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4233 +/- ##
==========================================
- Coverage 81.77% 81.76% -0.01%
==========================================
Files 331 331
Lines 76716 76722 +6
==========================================
Hits 62731 62731
- Misses 13985 13991 +6
|
395884a
to
6a68c74
Compare
This comment was marked as outdated.
This comment was marked as outdated.
eb6be56
to
af571ba
Compare
ea33e99
to
7f503a0
Compare
According to https://www.rfc-editor.org/rfc/rfc4861.html#section-4.6 All options are of the form: 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | Type | Length | ... | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ where Length 8-bit unsigned integer. The length of the option (including the type and length fields) in units of 8 octets. This patch makes it possible to recognize/generate unknown options by introducing a new field where lengths are calculated according to the RFC and options are padded with zeroes if necessary. By default trailing zeroes aren't stripped to make it easier to process options where trailing zeroes can be mixed up with actual data (like the encrypted DNS option for example). The captive portal option was switched to the new field too but there trailing zeroes are still stripped. fuzz(ICMPv6NDOptUnknown(type=...)) is much more effective now because it generates options valid enough to get past basic checks but nonsensical enough to trigger interesting issues like systemd/systemd#30952 (comment) systemd/systemd#30952 (comment) ICMPv6NDOptUnknown is also used instead of Raw to guess payloads so router advertisements can be parsed even when unknown options pop up in the middle. The patch was also cross-checked with Wireshark: ``` >>> tdecode(Ether()/IPv6()/ICMPv6ND_RA()/ICMPv6NDOptSrcLLAddr()/ICMPv6NDOptUnknown(data='watwatwat')/ICMPv6NDOptCaptivePortal()) ... ICMPv6 Option (Source link-layer address : 00:00:00:00:00:00) Type: Source link-layer address (1) Length: 1 (8 bytes) Link-layer address: 00:00:00_00:00:00 (00:00:00:00:00:00) ICMPv6 Option (Unknown 0) Type: Unknown (0) Length: 2 (16 bytes) [Expert Info (Note/Undecoded): Dissector for ICMPv6 Option (0) code not implemented, Contact Wireshark developers if you want this supported] [Dissector for ICMPv6 Option (0) code not implemented, Contact Wireshark developers if you want this supported] [Severity level: Note] [Group: Undecoded] Data: 7761747761747761740000000000 ICMPv6 Option (DHCP Captive-Portal) Type: DHCP Captive-Portal (37) Length: 1 (8 bytes) Captive Portal: ```
Really cool PR! Thanks. |
7f503a0
to
14f928f
Compare
Without this patch the type of Neighbor Discovery options generated by fuzz(ICMPv6NDOptUnknown()) is always 0. With this patch applied option types are random. It's a follow-up to secdev#4233
Without this patch the type of Neighbor Discovery options generated by fuzz(ICMPv6NDOptUnknown()) is always 0. With this patch applied option types are random. It's a follow-up to #4233
@@ -1737,18 +1737,41 @@ class _ICMPv6NDGuessPayload: | |||
|
|||
def guess_payload_class(self, p): | |||
if len(p) > 1: | |||
return icmp6ndoptscls.get(orb(p[0]), Raw) # s/Raw/ICMPv6NDOptUnknown/g ? # noqa: E501 | |||
return icmp6ndoptscls.get(orb(p[0]), ICMPv6NDOptUnknown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would maybe be useful if the known options that can't be parsed properly turned into ICMPv6NDOptUnknown
or something like that by analogy with tshark
showing options and their bytes. One recent example where it could have helped would be https://lists.debian.org/debian-security-announce/2024/msg00123.html (which was (partly) powered by scapy
FWIW). I'll try to figure out if it makes sense.
According to https://www.rfc-editor.org/rfc/rfc4861.html#section-4.6
where
This patch makes it possible to recognize/generate unknown options by introducing a new field where lengths are calculated according to the RFC and options are padded with zeroes if necessary. By default trailing zeroes aren't stripped to make it easier to process options where trailing zeroes can be mixed up with actual data (like the encrypted DNS option for example). The captive portal option was switched to the new field too but there trailing zeroes are still stripped.
fuzz(ICMPv6NDOptUnknown(type=...)) is much more effective now because it generates options valid enough to get past basic checks but nonsensical enough to trigger interesting issues like
systemd/systemd#30952 (comment)
systemd/systemd#30952 (comment)
ICMPv6NDOptUnknown is also used instead of Raw to guess payloads so router advertisements can be parsed even when unknown options pop up in the middle.
The patch was also cross-checked with Wireshark: