sys/net/dhcpv6: Fixes for MUD URL option#15939
Conversation
JKRhb
requested review from
PeterKietzmann,
cgundogan,
haukepetersen,
jia200x,
leandrolanzieri and
miri64
as code owners
benpicco
added
Area: network
|
Please squash, let's see what Murdock thinks of this. |
benpicco
added
the
CI: ready for build
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
Some last minute style comments ;-)
|
You may squash those immediately. |
| If any problems are encountered (i.e. if the test prints the string `FAILED`), | ||
| set the echo parameter in the `run()` function at the bottom of the test script | ||
| (tests/01-run.py) to `True`. The test script will then offer a more detailed | ||
| (tests-as-root/01-run.py) to `True`. The test script will then offer a more detailed |
There was a problem hiding this comment.
@miri64 Thanks :) While I was at it I corrected the path to the test files in this line and also squashed it already. I hope that this was okay
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
When running the test on samr21-xpro, I get a timeout. When checking out e0f457 (the merge base of this PR) I do not get the timeout :-/
Details |
Hmm, that is strange :/ I just ran the test on native again and it succeeded – do you see indications on why it might fail on |
The address seems not to be configured correctly. Maybe the buffer is too small? |
Hmm, do you mean the receive buffer? Or is there another buffer that could be affected? I just ran the tests on an ESP32 where they also passed – could you maybe try it without once more without the MUD option for a comparison? (Sorry that this small change causes so much trouble :/) |
Yeah, but sadly, increasing the receive buffer did not help :-/. |
With everything regarding MUD options commented out, the test succeeds :-/. |
Hmm, very strange :/ Could you maybe run the rest with |
Did this already, but doesn't really give helpful output. :( |
|
Ah, gave it another try: |
|
As far as I can tell, there is a buffer overflow happening somewhere that overwrites the first few bytes of |
|
Ahhh, I believe diff --git a/sys/net/application_layer/dhcpv6/client.c b/sys/net/application_layer/dhcpv6/client.c
index ce20966dcb..052935847f 100644
--- a/sys/net/application_layer/dhcpv6/client.c
+++ b/sys/net/application_layer/dhcpv6/client.c
@@ -262,13 +262,13 @@ static inline size_t _compose_mud_url_opt(dhcpv6_opt_mud_url_t *mud_url_opt,
uint16_t len = strlen(mud_url);
if (len > len_max) {
- assert(0);
+ assert(len <= len_max);
return 0;
}
mud_url_opt->type = byteorder_htons(DHCPV6_OPT_MUD_URL);
mud_url_opt->len = byteorder_htons(len);
- strncpy(mud_url_opt->mudString, mud_url, len_max);
+ strncpy(mud_url_opt->mudString, mud_url, len);
return len + sizeof(dhcpv6_opt_mud_url_t);
}
fixes it for me! (note that the |
|
This bug existed previously, but probably adding it to the other message types revealed the bug more clearly. |
|
also just noticed, that |
|
Thank you for identifying these issues! :) I pushed a new fixup commit with your suggestions
You are right, back then we used the casing from the RFC which was not the right choice. This should now also be fixed :) |
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
ACK, tested in on samr21-xpro and native. Both succeed now. To be on the save side I also ran the test with make all-asan on native which yielded nothing.
|
Please squash! :-) |
|
Done :) Thank you once more for your patience and expertise :) |
But even there it is |
Oh, ahem, then I made two mistakes at once – I'll try my best to bring it down to zero when I open the next PR ;) |
|
Hmm, some tests on Murdock failed (see, e. g., this one). I guess this can be easily fixed by replacing Edit: If so, I already provided a fixup commit below. |
probably, yes. Though I'm wondering what GCC is actually trying to say with that error message ^^" |
|
You can squash that already :-) |
maribu
added
State: waiting for CI update
miri64
added
CI: ready for build
5 participants
Contribution description
This PR fixes a couple of mistakes I made in PR #15508 which added the Manufacturer Usage Description option (MUD, RFC 8520) to the DHCPv6 client.
So far, the option was only included in Solicit Messages. According to RFC 8415, however, this is only permitted if it is "specifically allowed in the definition of individual options". I therefore removed the call of
_compose_mud_url_optfrom the_solicit_serversfunction and added it to_request_renew_rebindinstead, including the option now in Request, Renew, and Rebind Messages. I updated the tests intests/gnrc_dhcpv6_client_6lbraccordingly.I also noticed that the naming of the Kconfig option I added in #15508 was incorrect. Setting the option
DHCPV6_CLIENT_MUD_URLshould now work as intended.Furthermore, I rearranged some of the code, hopefully making the module a bit more efficient and robust. For example, the assertion of the MUD URL now happens in the
dhcpv6_client_initfunction causing the application to crash immediately if something is wrong with the URL (rather than when a Request Message is sent).Lastly I have noticed that the instructions for using the
tests-as-rootwas outdated both in the readme files oftests/gnrc_dhcpv6_client_6lbrandtests/gnrc_dhcpv6_client. I edited the two files in two separate commits which could also be merged into one (especially if this information should be missing in more readmes).Testing procedure
The changes to the client can be tested manually by using the updated tests provided in
tests/gnrc_dhcpv6_client_6lbr.The Kconfig option can be tested, for instance, by first enabling Kconfig in the Makefile of
tests/gnrc_dhcpv6_client_6lbrand then changing the MUD URL by usingmake menuconfig. If the URL is nothttps://example.organymore, performing the tests withsudo make test-as-rootshould fail.Issues/PRs references
See also #15508.