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

gnrc_rpl: Lack of bounds check for packed structs #16018

Closed
nmeum opened this issue Feb 16, 2021 · 0 comments · Fixed by #16048
Closed

gnrc_rpl: Lack of bounds check for packed structs #16018

nmeum opened this issue Feb 16, 2021 · 0 comments · Fixed by #16048
Assignees
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@nmeum
Copy link
Member

nmeum commented Feb 16, 2021

Description

RIOT's RPL implementation as provided by the gnrc_rpl module lacks proper bounds checks. RPL messages are encapsulated in ICMPv6 datagrams. The message body of the ICMPv6 datagram is extracted as follows:

icmpv6_hdr = (icmpv6_hdr_t *)icmpv6->data;
switch (icmpv6_hdr->code) {
case GNRC_RPL_ICMPV6_CODE_DIS:
DEBUG("RPL: DIS received\n");
gnrc_rpl_recv_DIS((gnrc_rpl_dis_t *)(icmpv6_hdr + 1), iface, &ipv6_hdr->src,
&ipv6_hdr->dst, byteorder_ntohs(ipv6_hdr->len));
break;
case GNRC_RPL_ICMPV6_CODE_DIO:
DEBUG("RPL: DIO received\n");
gnrc_rpl_recv_DIO((gnrc_rpl_dio_t *)(icmpv6_hdr + 1), iface, &ipv6_hdr->src,
&ipv6_hdr->dst, byteorder_ntohs(ipv6_hdr->len));
break;
case GNRC_RPL_ICMPV6_CODE_DAO:
DEBUG("RPL: DAO received\n");
gnrc_rpl_recv_DAO((gnrc_rpl_dao_t *)(icmpv6_hdr + 1), iface, &ipv6_hdr->src,
&ipv6_hdr->dst, byteorder_ntohs(ipv6_hdr->len));
break;
case GNRC_RPL_ICMPV6_CODE_DAO_ACK:
DEBUG("RPL: DAO-ACK received\n");
gnrc_rpl_recv_DAO_ACK((gnrc_rpl_dao_ack_t *)(icmpv6_hdr + 1), iface, &ipv6_hdr->src,
&ipv6_hdr->dst, byteorder_ntohs(ipv6_hdr->len));
break;

The code above casts icmpv6_hdr + 1 (i.e. the ICMPv6 message body) to the appropriate RPL packed struct (e.g. gnrc_rpl_dio_t). However, it does not check whether the message is large enough to even contain a gnrc_rpl_dio_t (or any other packed RPL struct). As such, the handlers in gnrc_rpl_control_messages.c for specific RPL messages must check the len parameter before accessing any fields of these structs. The handler for gnrc_rpl_dao_t messages, for example, directly pass the required information to the gnrc_rpl_validation_DAO function, however, this function itself access fields of the struct before performing a length check to ensure that these fields are actually present. For example:

if ((dao->k_d_flags & GNRC_RPL_DAO_D_BIT)) {
expected_len += sizeof(ipv6_addr_t);
}

If the ICMP packet is too short this will result in an out-of-bounds read.

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.

Compile and run the application using:

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

Afterwards run socat as:

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

Expected results

The application shouldn't crash.

Actual results

RIOT network stack example application
All up, running the shell now
> ==12092==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0xffddc000; bottom 0x5665b000; size: 0xa9781000 (-1451749376)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
RPL: GNRC_NETAPI_MSG_TYPE_RCV received
RPL: DAO received
=================================================================
==12092==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf4e00635 at pc 0x565fa023 bp 0x5666bc38 sp 0x5666bc2c
READ of size 1 at 0xf4e00635 thread T0
    #0 0x565fa022 in gnrc_rpl_validation_DAO /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:116
    #1 0x56602fdf in gnrc_rpl_recv_DAO /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:1153
    #2 0x565f6698 in _receive /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl.c:190
    #3 0x565f725a in _event_loop /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl.c:312
    #4 0xf772353a in makecontext (/lib/i386-linux-gnu/libc.so.6+0x4153a)

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

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:116 in gnrc_rpl_validation_DAO
Shadow bytes around the buggy address:
  0x3e9c0070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9c0080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9c0090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9c00a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9c00b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3e9c00c0: fa fa fa fa fa fa[05]fa fa fa fd fd fa fa fd fd
  0x3e9c00d0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fd
  0x3e9c00e0: fa fa fd fd fa fa fd fa fa fa fd fa fa fa fd fa
  0x3e9c00f0: fa fa fd fa fa fa fd fa fa fa fa fa fa fa fa fa
  0x3e9c0100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3e9c0110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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
==12092==ABORTING
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants