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

ipv6/nib: bugfix of 6CO length checking #17850

Merged

Conversation

fabian18
Copy link
Contributor

Contribution description

The condition to check the length of an 6LoWPAN-ND Context Option was wrong.
If the prefix length > 64 it must be 3 (MAX), if it is less or equal 64 it must be 2 (MIN).
RFC6775 section 4.2

Testing procedure

See #17803 (comment).
At least nib: received 6CO of invalid length (3), must be 3 or wasn't delivered by RA. does not show up anymore.

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Mar 23, 2022
@fabian18 fabian18 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 23, 2022
DEBUG("nib: received 6CO of invalid length (%u), must be %u "
"or wasn't delivered by RA."
"\n",
assert(icmpv6->type == ICMPV6_RTR_ADV);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's from a received package, right?
Since there is always the possibility to receive garbage, we can't trigger an assert on a malformed packet, otherwise a bad node can bring other, perfectly fine behaving nodes down by sending packets that trigger asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Then I would drop the check for Router Advertisement completely, because we decide when we handle the Context Option (call the function) and we only handle it in an RA as it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That´s why I found the check unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if the assert ever fires, we would have a misuse of the function rather than an evil node sending us garbage :)

@fabian18 fabian18 force-pushed the bugfix_nib_6LN_context_option_length branch from 12830ab to 77cbe15 Compare March 24, 2022 09:08
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.

Makes sense and the code more readable 👍

@fabian18 fabian18 merged commit 09b98ea into RIOT-OS:master Mar 24, 2022
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 8, 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.

3 participants