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

dhcpv6: don't treat zero option as an end-of-payload marker #18625

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Sep 22, 2022

Contribution description

As far as I can tell, no DHCPv6 RFC specifies this option.

The handling for the zero option was added in #17736 by @benpicco to fix issues encountered while trying to retrieve a DHCHPv6 lease. However, I strongly suspect that the zero option was encountered in this case due to an out-of-bounds read performed in RIOT's DHCPv6 client implementation (i.e. the option parsing loop read beyond the packet bounds). This issue was fixed in #18307 and I strongly suspect that it should also fix the issue @benpicco originally encountered in #17736. As such, I propose that we remove the if statement which treats the zero option as an end-of-payload marker.

Testing procedure

@benpicco could you test this with the DHCPv6 server implementation with which you originally encountered the error described in #17736 and report back if you still encounter this error with this PR applied? 🙏

Issues/PRs references

Fixes #18309

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I can confirm that this is no longer needed.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 22, 2022
@benpicco benpicco enabled auto-merge September 22, 2022 16:28
As far as I can tell, no DHCPv6 RFC specifies this option. The handling
for the zero option was added in RIOT-OS#17736 by @benpicco to fix issues
encountered while trying to retrieve a DHCHPv6 lease. However, I
strongly suspect that the zero option was encountered in this case due
to an out-of-bounds read performed in RIOT's DHCPv6 client
implementation (i.e. the option parsing loop read beyond the packet
bounds). This issue was fixed in RIOT-OS#18307 and I strongly suspect that it
should also fix the issue @benpicco originally encountered in RIOT-OS#17736. As
such, I propose that we remove the if statement which treats the zero
option as an end-of-payload marker.

Fixes RIOT-OS#18309
@benpicco benpicco merged commit 04b7ea5 into RIOT-OS:master Sep 23, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gnrc_dhcpv6_client: checking for 0 option in REPLY might be superfluous.
3 participants