Skip to content
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.

Sign up for GitHub

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

Jump to bottom

panic decoding DHCP domain name server option #305

Closed
cjbe opened this issue Sep 10, 2019 · 3 comments
Closed

panic decoding DHCP domain name server option #305

cjbe opened this issue Sep 10, 2019 · 3 comments
Labels
kind/bug

Comments

@cjbe
Copy link
Contributor

cjbe commented Sep 10, 2019

Using the current smoltcp master, I get a panic during parsing a DHCP response:

panicked at 'index out of bounds: the len is 3 but the index is 3', C:\scratch\smoltcp\src\wire\dhcpv4.rs:749:25

This panic is from parsing the DHCP option field OPT_DOMAIN_NAME_SERVER here

The panic occurs as my DHCP server is broadcasting 4 DNS server addresses, and the code here iterates through the complete list of 4 DNS servers but tries to store the parsed addresses into an array of fixed length 3.

In my case the data in the options field is:
[163, 1, 74, 6, 163, 1, 74, 7, 163, 1, 74, 3, 163, 1, 74, 4]
This is valid, as per the RFC the only requirements are a minimum length of 4 octets, and length a multiple of 4 octets (i.e. there is no maximum length).
@whitequark
Copy link
Contributor

Sounds like we should add fuzzing for DHCP packets too.

@cjbe
Copy link
Contributor Author

cjbe commented Sep 11, 2019

@whitequark would you accept a PR to only use the first 3 name servers supplied by the DHCP server? This is not a long term solution, but seems useful in the short term. (This seems to be what the current code is trying to implement at the moment with its fixed array of 3 DNS servers)

@whitequark
Copy link
Contributor

Yes, with a test for the panic.

cjbe added a commit to cjbe/smoltcp that referenced this issue Sep 12, 2019
@cjbe
Test parsing DHCP ACK with >3 DNS servers (cf smoltcp-rs#305)
c34ca21 
The OPT_DOMAIN_NAME_SERVER DHCP option field can advertise an unbounded
number of DNS servers. This adds a test that we correctly parse packets
with more DNS servers advertised than we can fit in our fixed length buffer.
cjbe added a commit to cjbe/smoltcp that referenced this issue Sep 12, 2019
@cjbe
Only use first 3 DHCP advertised DNS servers (cf smoltcp-rs#305)
3ed877f 
The OPT_DOMAIN_NAME_SERVER option can advertise an arbitrary number of
DNS servers, but we currently use a fixed length array to store these
servers. This commit ensures only the first 3 (highest priority) servers
are used.
@alexandrasandulescu alexandrasandulescu mentioned this issue Oct 28, 2021
Merged
bors bot added a commit that referenced this issue Nov 3, 2021
@bors
Merge #562
72f2073 
562: fuzz: DHCP header parser r=Dirbaio a=alexandrasandulescu

Hi. This fuzz target tests the [DHCPRepr](https://docs.rs/smoltcp/0.7.5/smoltcp/wire/struct.DhcpRepr.html) `parse` and `emit` functions ([context](#305 (comment))).
The fuzz target requires a small change in the `DHCPOption` parser because the option type `OPT_DOMAIN_NAME_SERVER ` requires the payload representing the DNS server list to have a length multiple of 4. Otherwise the `chunks` call places the remainder in the last chunk (length < 4). This panics when the last chunk is parsed into an `IPv4Address`. Do you have a better suggestion for handling this situation?

I ran the fuzzer for a couple of days but it didn't find any crash yet. To increase the chances, I used the oss-fuzz seed corpora of `dnsmasq` DHCP fuzzer and `systemd` DHCP server fuzzer.
Please share if you have suggestions on how to improve coverage.

Co-authored-by: Alexandra Sandulescu <aesa@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug
Milestone
No milestone
Development

No branches or pull requests
2 participants
@whitequark @cjbe