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.

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

dhserver: Support DHCP clients that don't send the MESSAGETYPE as first option #1712

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

kripton
Copy link
Contributor

@kripton kripton commented Nov 1, 2022

The included DHCP server fails to process DHCP DISCOVER or REQUEST packages that don't contain the "DHCP MESSAGE TYPE" option as the first option in the package.
The initial report that triggered me was in OpenLightingProject/rp2040-dmxsun#53 and after some investigation, I found that the "dhcpcd" client (maybe others, too) don't send the MESSAGE TYPE as the first option (see Wireshark dump attached in the linked issue). This seems to be valid since the order of the options contained in the package shouldn't matter. However, tinyUSB's DHCP server expects the MESSAGE TYPE to be the first option.

This change uses the already existing function to extract the MESSAGE TYPE option from the options properly. It will also fail now, if the MESSAGE TYPE option doesn't exist at all, making the code safer than the current approach.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superb, thank you very much for the fix.

@hathach hathach merged commit 95fb11f into hathach:master Nov 18, 2022
@kripton kripton deleted the fixDhserver branch November 18, 2022 21:18
hathach added a commit that referenced this pull request Feb 13, 2023
dhserver: Fix a potential DoS vulnerability accidentially introduced by #1712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants