sys/shell/ping: fix ping packet size overflow by krzysztof-cabaj · Pull Request #19927 · RIOT-OS/RIOT

krzysztof-cabaj · 2023-09-16T17:59:38Z

Contribution description

In #19829 @mchesser point out integer overflow in the ping command and API. This PR fix this issue in two ways:

Add protection in the API.
Add protection in the user command.

Testing procedure

Without this PR passing negative number to the ping -s option cause segmentation fault, for example in the
example/gnrc_networking:

> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault

With this PR user shows appropriate warning test:

> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
>

Issues/PRs references

Issue #19829

miri64

Soft ACK, but a small formatting issue.

sys/shell/cmds/gnrc_icmpv6_echo.c

miri64 · 2023-09-17T09:09:50Z

sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c

    uint8_t *databuf;

+    /* max IPv6 payload 65535 minus 8 bytes of icmp header = 65527 */
+    if (len > 65527) {


Ah and can you please use e.g. UINT16_MAX - sizeof(icmpv6_hdr_t) instead of the magic number, then you don't need the comment ;-).

Ok. Fixed. If this is not a problem I would like to leave this comment.

miri64 · 2023-09-17T09:09:57Z

sys/shell/cmds/gnrc_icmpv6_echo.c

-                        data->datalen = atoi(argv[i]);
+                        value = atoi(argv[i]);
+
+                        if(value < 0 || value > 65527) {


krzysztof-cabaj · 2023-09-18T09:29:20Z

Fixed and squashed.

sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c

sys/shell/cmds/gnrc_icmpv6_echo.c

miri64

bors merge

bors · 2023-09-18T13:12:36Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

riot-ci · 2023-09-18T15:56:57Z

Murdock results

✔️ PASSED

a958fa2 sys/shell/ping: reduce text to save ROM

Success	Failures	Total	Runtime
7937	0	7937	15m:39s

Artifacts

Documentation preview

miri64 · 2023-09-19T11:39:08Z

bors merge

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj ### Contribution description In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways: 1) Add protection in the API. 2) Add protection in the user command. ### Testing procedure Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the ```example/gnrc_networking```: ``` > ping -s -7 ::1 ping -s -7 ::1 Segmentation fault ``` With this PR user shows appropriate warning test: ``` > ping -s -7 ::1 ping -s -7 ::1 ICMPv6 datagram size should be in range <0, 65527>. > ``` ### Issues/PRs references Issue #19829 Co-authored-by: krzysztof-cabaj <[email protected]>

bors · 2023-09-19T12:55:15Z

Build failed:

Murdock

miri64 · 2023-09-19T18:09:28Z

bors merge

miri64 · 2023-09-19T18:10:04Z

bors cancel

bors · 2023-09-19T18:10:08Z

Canceled.

miri64 · 2023-09-19T18:56:52Z

bors merge

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj ### Contribution description In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways: 1) Add protection in the API. 2) Add protection in the user command. ### Testing procedure Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the ```example/gnrc_networking```: ``` > ping -s -7 ::1 ping -s -7 ::1 Segmentation fault ``` With this PR user shows appropriate warning test: ``` > ping -s -7 ::1 ping -s -7 ::1 ICMPv6 datagram size should be in range <0, 65527>. > ``` ### Issues/PRs references Issue #19829 Co-authored-by: krzysztof-cabaj <[email protected]>

miri64 · 2023-09-19T18:57:46Z

bors cancel

bors · 2023-09-19T18:57:48Z

Canceled.

benpicco · 2023-09-20T13:30:49Z

I'm not sure what you did there, but your PR now adds +389,749 new lines of code.

Please do a git rebase -i master, that should clean things up. (given your master branch is on a state that matches upstream)

benpicco · 2023-09-20T13:33:03Z

sys/shell/cmds/gnrc_icmpv6_echo.c

+                        value = atoi(argv[i]);
+
+                        if ((value < 0) || ((unsigned)value > (UINT16_MAX - sizeof(icmpv6_hdr_t)))) {
+                            printf("ICMPv6 datagram size should be in range 0-65527.\n");


tbh I don't think you need to check that twice. You already have that check in gnrc_icmpv6_echo_send(), no need to pre-check - you can just let gnrc_icmpv6_echo_send() return error.

Sending ICMP messages larger than 65527 bytes is not such a common use-case that we need special handling for it in the shell command.

There is nothing speaking against pre-checking if you want to have user-friendly error messages, IMHO. At least the value < 0 should stay in any case, since value is int, but len is size_t (so unsigned). Casting could introduce some unexpected values here (especially if size_t is set to uint16_t by the architecture).

I agree that now I have to checks. The one in the API - protects users which would like send icmp pings in the own code. The second check as @miri64 write gives more user friendly warning. Instead, user when gives wrong size see only cryptic text:

All up, running the shell now > ping -s -7 ::1 ping -s -7 ::1 error: -22 error: -22 error: -22 --- ::1 PING statistics --- 3 packets transmitted, 0 packets received, 100% packet loss >

krzysztof-cabaj · 2023-09-20T13:57:45Z

A while ago I think that I understand rebase ;)
Sorry for the mess - give me a (longer) while to fix everything.

miri64 · 2023-09-20T18:07:14Z

bors merge

bors · 2023-09-20T18:07:18Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

krzysztof-cabaj · 2023-09-20T18:54:48Z

And now everything is corrected (I hope) - I missed one, crucial commit, which save ROM.

miri64 · 2023-09-20T20:13:11Z

bors merge

bors · 2023-09-21T00:00:51Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

krzysztof-cabaj · 2023-09-21T07:27:09Z

Thanks all for support ... and patience ;).

miri64 · 2023-09-21T20:32:06Z

Thank you for your contribution and for fixing this ☺️

krzysztof-cabaj requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners September 16, 2023 17:59

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Sep 16, 2023

miri64 reviewed Sep 17, 2023

View reviewed changes

sys/shell/cmds/gnrc_icmpv6_echo.c Outdated Show resolved Hide resolved

sys/shell/cmds/gnrc_icmpv6_echo.c Outdated Show resolved Hide resolved

miri64 reviewed Sep 17, 2023

View reviewed changes

krzysztof-cabaj force-pushed the gnrc_icmp6_overflow branch from 0dcf20b to 8c7116e Compare September 18, 2023 09:26

miri64 requested changes Sep 18, 2023

View reviewed changes

sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c Outdated Show resolved Hide resolved

sys/shell/cmds/gnrc_icmpv6_echo.c Outdated Show resolved Hide resolved

krzysztof-cabaj force-pushed the gnrc_icmp6_overflow branch from a2a7560 to 1145a41 Compare September 18, 2023 10:28

krzysztof-cabaj requested a review from miri64 September 18, 2023 10:37

miri64 approved these changes Sep 18, 2023

View reviewed changes

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 18, 2023

krzysztof-cabaj mentioned this pull request Sep 19, 2023

examples/gcoap: add saml11-xpro to CI boards with insufficient memory #19933

Merged

miri64 mentioned this pull request Sep 19, 2023

boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit #19923

Merged

github-actions bot added Area: drivers Area: Device drivers Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Sep 20, 2023

benpicco reviewed Sep 20, 2023

View reviewed changes

krzysztof-cabaj added 3 commits September 20, 2023 11:24

gnrc/icmpv6: add check for too big icmpv6 packets

d700047

sys/shell/ping: add protection from icmp packet overflow

be7ae57

examples/gcoap: revert PR RIOT-OS#19933

838e62c

krzysztof-cabaj force-pushed the gnrc_icmp6_overflow branch from de36035 to 838e62c Compare September 20, 2023 16:32

sys/shell/ping: reduce text to save ROM

a958fa2

bors bot merged commit 8d1a93a into RIOT-OS:master Sep 21, 2023

krzysztof-cabaj mentioned this pull request Sep 27, 2023

Integer overflow in icmpv6 echo packet creation #19829

Closed

MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023

Conversation

krzysztof-cabaj commented Sep 16, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miri64 Sep 17, 2023

Choose a reason for hiding this comment

Uh oh!

krzysztof-cabaj Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

miri64 Sep 17, 2023

Choose a reason for hiding this comment

Uh oh!

krzysztof-cabaj commented Sep 18, 2023

Uh oh!

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Sep 18, 2023

Uh oh!

riot-ci commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

miri64 commented Sep 19, 2023

Uh oh!

bors bot commented Sep 19, 2023

Uh oh!

miri64 commented Sep 19, 2023

Uh oh!

miri64 commented Sep 19, 2023

Uh oh!

bors bot commented Sep 19, 2023

Uh oh!

miri64 commented Sep 19, 2023

Uh oh!

miri64 commented Sep 19, 2023

Uh oh!

bors bot commented Sep 19, 2023

Uh oh!

benpicco commented Sep 20, 2023

Uh oh!

benpicco Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

miri64 Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

krzysztof-cabaj Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

krzysztof-cabaj commented Sep 20, 2023

Uh oh!

miri64 commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

krzysztof-cabaj commented Sep 20, 2023

Uh oh!

miri64 commented Sep 20, 2023

Uh oh!

bors bot commented Sep 21, 2023

Uh oh!

krzysztof-cabaj commented Sep 21, 2023

Uh oh!

miri64 commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

riot-ci commented Sep 18, 2023 •

edited

Loading