Merge #19927

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]>
RIOT-OS · Sep 19, 2023 · 8900510 · 8900510
2 parents 9609a49 + 1145a41
commit 8900510
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 1 deletion.
diff --git a/sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c b/sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c
@@ -150,6 +150,12 @@ int gnrc_icmpv6_echo_send(const gnrc_netif_t *netif, const ipv6_addr_t *addr,
     ipv6_hdr_t *ipv6;
     uint8_t *databuf;
 
+    /* max IPv6 payload 65535 minus 8 bytes of icmp header = 65527 */
+    if (len > (UINT16_MAX - sizeof(icmpv6_hdr_t))) {
+        DEBUG("error: wrong icmpv6 packet length\n");
+        return -EINVAL;
+    }
+
     pkt = gnrc_icmpv6_echo_build(ICMPV6_ECHO_REQ, id, seq, NULL, len);
     if (pkt == NULL) {
         DEBUG("error: packet buffer full\n");

diff --git a/sys/shell/cmds/gnrc_icmpv6_echo.c b/sys/shell/cmds/gnrc_icmpv6_echo.c
@@ -172,6 +172,7 @@ static int _configure(int argc, char **argv, _ping_data_t *data)
 {
     char *cmdname = argv[0];
     int res = 1;
+    int value;
 
     /* parse command line arguments */
     for (int i = 1; i < argc; i++) {
@@ -207,7 +208,13 @@ static int _configure(int argc, char **argv, _ping_data_t *data)
                     /* intentionally falls through */
                 case 's':
                     if ((++i) < argc) {
-                        data->datalen = atoi(argv[i]);
+                        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");
+                            return -1;
+                        }
+                        data->datalen = value;
                         continue;
                     }
                     /* intentionally falls through */