Skip to content

Pico-W Access Point: Fix DHCP server for certain clients (dhcpcd) #288

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

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

kripton
Copy link
Contributor

@kripton kripton commented Nov 2, 2022

See hathach/tinyusb#1712 and the initial trigger OpenLightingProject/rp2040-dmxsun#53:

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.

@kripton
Copy link
Contributor Author

kripton commented Nov 2, 2022

Oh, here are the wireshark dumps (zipped since GitHub doesn't allow them otherwise):
Pico-W-DHCP.zip

pico_w_AP_DHCP_Failed.pcapng shows the behaviour prior to the change. Transaction 0xa76ad45d is initiated by NetworkManager's internal DHCP client and is working as expected. Transaction 0xe4ab1f63 is initiated by dhcpcd as client and is not ACKed by the Pico W's DHCP server since the first option in the REQUEST package is the requested IP address. The second option is the DHCP Message Type. Transaction 0xd2704100 is again from NetworkManager and is ACKed to show that the DHCP server is able to serve multiple requests and the problem is not that only the first DHCP REQUEST is handled propery.

pico_w_AP_DHCP_Fixed.pcapng shows the behaviour WITH the changes from this PR. All DHCP REQUEST packages are handled properly.

@peterharperuk
Copy link
Contributor

Looks good. We "borrowed" this from Micropython. So I guess I should return the favour and fix it there?

@kripton
Copy link
Contributor Author

kripton commented Dec 5, 2022

Sure, spread the word :) The code from the pico-examples and the one from tinyusb look similar but not identical. Don't know the origin but the more places have the fix the better. TinyUsb merged it by now.

@peterharperuk peterharperuk self-assigned this Feb 2, 2023
@kripton
Copy link
Contributor Author

kripton commented Feb 2, 2023

Oh, wait a second. In the case where ptr is NULL, the pbuffer is not freed, leading to a possible DoS. I will fix this tomorrow

Copy link
Contributor

@peterharperuk peterharperuk left a comment

Choose a reason for hiding this comment

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

Can you please also push to develop? (before I accidentally push to master again)

@kripton
Copy link
Contributor Author

kripton commented Feb 4, 2023

Oh, wait a second. In the case where ptr is NULL, the pbuffer is not freed, leading to a possible DoS. I will fix this tomorrow

I seem to have gotten confused. The "original" (as I saw it) dhserver.c from tinyUSB didn't include the goto ignore_request-lines. As such, I introduced a buffer exhaustion there by not freeing the pbuf.

In this version (copied from micropython) WITH the goto ignore_request-solution, it is all fine, no buffer problems.

I rebased on top of develop as requested 👍

@kripton kripton force-pushed the PicoW_AP_FixDHCPServer branch from 2761901 to cba27e5 Compare February 4, 2023 21:31
@kripton kripton changed the base branch from master to develop February 4, 2023 21:31
@kripton kripton force-pushed the PicoW_AP_FixDHCPServer branch from cba27e5 to 57740f4 Compare February 4, 2023 21:32
@kripton kripton requested review from peterharperuk and removed request for kilograham February 4, 2023 21:33
@peterharperuk peterharperuk merged commit b59a522 into raspberrypi:develop Feb 9, 2023
@kripton kripton deleted the PicoW_AP_FixDHCPServer branch February 9, 2023 18:11
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