gnrc_dhcpv6_client: Fix out-of-bounds access during option parsing #18307

nmeum · 2022-07-11T13:54:25Z

Contribution description

The _parse_reply function provided by gnrc_dhcpv6_client has two parsing loops for the DHCPv6 message options:

RIOT/sys/net/application_layer/dhcpv6/client.c

Lines 991 to 1023 in 37a4fff

    
           for (dhcpv6_opt_t *opt = (dhcpv6_opt_t *)(&rep[sizeof(dhcpv6_msg_t)]); 
        
                len > 0; len -= _opt_len(opt), opt = _opt_next(opt)) { 
        
               if (len > orig_len) { 
        
                   DEBUG("DHCPv6 client: ADVERTISE options overflow packet boundaries\n"); 
        
                   return false; 
        
               } 
        
               switch (byteorder_ntohs(opt->type)) { 
        
                   case DHCPV6_OPT_CID: 
        
                       cid = (dhcpv6_opt_duid_t *)opt; 
        
                       break; 
        
                   case DHCPV6_OPT_SID: 
        
                       sid = (dhcpv6_opt_duid_t *)opt; 
        
                       break; 
        
                   case DHCPV6_OPT_STATUS: 
        
                       status = (dhcpv6_opt_status_t *)opt; 
        
                       break; 
        
                   case DHCPV6_OPT_SMR: 
        
                       smr = (dhcpv6_opt_smr_t *)opt; 
        
                       break; 
        
                   case DHCPV6_OPT_IMR: 
        
                       imr = (dhcpv6_opt_imr_t *)opt; 
        
                       break; 
        
                   case DHCPV6_OPT_IRT: 
        
                       irt = (dhcpv6_opt_irt_t *)opt; 
        
                       break; 
        
                   default: 
        
                       break; 
        
               } 
        
               /* 0 option is used as an end marker, len can include bogus bytes */ 
        
               if (!byteorder_ntohs(opt->type)) { 
        
                   break; 
        
               } 
        
           }

and:

RIOT/sys/net/application_layer/dhcpv6/client.c

Lines 1053 to 1082 in 37a4fff

    
               len = orig_len - sizeof(dhcpv6_msg_t); 
        
               for (dhcpv6_opt_t *opt = (dhcpv6_opt_t *)(&rep[sizeof(dhcpv6_msg_t)]); 
        
                    len > 0; len -= _opt_len(opt), opt = _opt_next(opt)) { 
        
                   switch (byteorder_ntohs(opt->type)) { 
        
           #if IS_USED(MODULE_DHCPV6_CLIENT_DNS) 
        
                       case DHCPV6_OPT_DNS_RNS: 
        
                           dhcpv6_client_dns_rns_conf((dhcpv6_opt_dns_rns_t *)opt, 
        
                                                      remote.netif); 
        
                           break; 
        
           #endif  /* IS_USED(MODULE_DHCPV6_CLIENT_DNS) */ 
        
                       case DHCPV6_OPT_IA_PD: 
        
                           if (_parse_ia_pd_option((dhcpv6_opt_ia_pd_t *)opt)) { 
        
                                /* No error occurred */ 
        
                                break; 
        
                            } else { 
        
                                /* Something went wrong */ 
        
                                return false; 
        
                            } 
        
                       case DHCPV6_OPT_IA_NA: 
        
                            if (_parse_ia_na_option((dhcpv6_opt_ia_na_t *)opt)) { 
        
                                /* No error occurred */ 
        
                                break; 
        
                            } else { 
        
                                /* Something went wrong */ 
        
                                return false; 
        
                            } 
        
                       default: 
        
                           break; 
        
                   } 
        
               }

Only the first parsing loop performs a sanity check on the length field of the DHCPv6 option:

RIOT/sys/net/application_layer/dhcpv6/client.c

Lines 993 to 996 in 37a4fff

    
           if (len > orig_len) { 
        
               DEBUG("DHCPv6 client: ADVERTISE options overflow packet boundaries\n"); 
        
               return false; 
        
           }

In the second parsing loop it is assumed that all options have a valid length field and no further sanity checks are performed. However, there are two problems with this assumption:

The second parsing loop correctly decrements the orig_len by sizeof(dhcpv6_msg_t). The first parsing loop doesn't do this which is wrong since it also (correctly) skips the message type and transaction ID headers when parsing the options. As such, both loops operate on different len values. I believe this to be an oversight which was missed in sys/net/dhcpv6: fixes option length handling in client implementation #13622.
Since sys/net/dhcpv6: miscellaneous tweaks #17736, the first parsing loop stops parsing when encountering a zero option. However, the second parsing loop continues parsing even after the zero option. As such, the length field of all options after a zero option are not checked and can cause an out-of-bounds memory read in the second option parsing loop.

Testing procedure

This is somewhat difficult to test since transaction ID etc. need to match.

However, I can demonstrate this issue through "unit testing" of _parse_reply.

Compile the following application:

#include <stdbool.h>
#include <stdint.h>

uint8_t input[] = {
0x07, 0x02, 0x20, 0x8E, 0x00, 0x01, 0x00, 0x04, 0x00, 0x03, 0x00, 0x14, 0x00, 0x02, 0x00, 0x04,
0xAA, 0xBB, 0x0C, 0x1D, 0x58, 0x5E, 0x00, 0x02, 0xDE, 0x9C, 0x00, 0x00, 0xB8, 0x10, 0x00, 0x06,
0x9C, 0x1C, 0x1C, 0x1C
};

extern bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type);

int main(void)
{
    _parse_reply(input, sizeof(input), 0);
    return 0;
}

With the following Makefile:

APPLICATION = parse-reply
BOARD = native

RIOTBASE ?= $(CURDIR)/../..

DEVELHELP ?= 1

USEMODULE += gnrc_ipv6_default
USEMODULE += gnrc_dhcpv6_client

include $(RIOTBASE)/Makefile.include

and make the following modifications to _parse_reply to disable some sanity checks on the transaction ID etc:

diff --git a/sys/net/application_layer/dhcpv6/client.c b/sys/net/application_layer/dhcpv6/client.c
index 237a86a44a..885d9963df 100644
--- a/sys/net/application_layer/dhcpv6/client.c
+++ b/sys/net/application_layer/dhcpv6/client.c
@@ -33,7 +33,7 @@
 #include "xtimer/implementation.h"
 #endif

-#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1
 #include "debug.h"

 #include "_dhcpv6.h"
@@ -974,7 +974,7 @@ static bool _parse_ia_na_option(dhcpv6_opt_ia_na_t *ia_na)
     return true;
 }

-static bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type)
+bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type)
 {
     dhcpv6_opt_duid_t *cid = NULL, *sid = NULL;
     dhcpv6_opt_status_t *status = NULL;
@@ -984,7 +984,7 @@ static bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type)
     size_t orig_len = len;

     DEBUG("DHCPv6 client: received REPLY\n");
-    if ((len < sizeof(dhcpv6_msg_t)) || !_is_tid((dhcpv6_msg_t *)rep)) {
+    if ((len < sizeof(dhcpv6_msg_t))) {
         DEBUG("DHCPv6 client: packet too small or transaction ID wrong\n");
         return false;
     }
@@ -1027,7 +1027,9 @@ static bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type)
         return false;
     }
     if (!_check_cid_opt(cid) || !_check_sid_opt(sid)) {
+#if 0
         return false;
+#endif
     }
     if (smr != NULL) {
         sol_max_rt = byteorder_ntohl(smr->value);
@@ -1047,9 +1049,7 @@ static bool _parse_reply(uint8_t *rep, size_t len, uint8_t request_type)
         }
         _set_event_timeout_sec(&information_refresh_timeout, &refresh_information, refresh_time);
     }
-    if (!_check_status_opt(status)) {
-        return false;
-    }
+    (void)status;
     len = orig_len - sizeof(dhcpv6_msg_t);
     for (dhcpv6_opt_t *opt = (dhcpv6_opt_t *)(&rep[sizeof(dhcpv6_msg_t)]);
          len > 0; len -= _opt_len(opt), opt = _opt_next(opt)) {

Afterwards, run:

$ make -C examples/parse_reply/ all-valgrind
$ make -C examples/parse_reply/ term-valgrind
main(): This is RIOT! (Version: 2022.07-devel-1001-g1ba2ef)
DHCPv6 client: received REPLY
DHCPv6 client: message is not for me
==21654== Invalid read of size 2
==21654==    at 0x804E712: byteorder_swaps (sys/include/byteorder.h:405)
==21654==    by 0x804E712: byteorder_ntohs (sys/include/byteorder.h:548)
==21654==    by 0x804E712: _parse_reply (sys/net/application_layer/dhcpv6/client.c:1056)
==21654==    by 0x804964C: main (examples/parse_reply/main.c:15)
==21654==  Address 0x8079000 is on thread 1's stack
==21654==
==21654== Invalid read of size 2
==21654==    at 0x804E6FC: byteorder_swaps (sys/include/byteorder.h:405)
==21654==    by 0x804E6FC: byteorder_ntohs (sys/include/byteorder.h:548)
==21654==    by 0x804E6FC: _opt_len (sys/net/application_layer/dhcpv6/client.c:547)
==21654==    by 0x804E6FC: _parse_reply (sys/net/application_layer/dhcpv6/client.c:1055)
==21654==    by 0x804964C: main (examples/parse_reply/main.c:15)
==21654==  Address 0x8079002 is on thread 1's stack
==21654==
==21654==
==21654== Process terminating with default action of signal 11 (SIGSEGV)
==21654==  Access not within mapped region at address 0x807A000
==21654==    at 0x804E712: byteorder_swaps (sys/include/byteorder.h:405)
==21654==    by 0x804E712: byteorder_ntohs (sys/include/byteorder.h:548)
==21654==    by 0x804E712: _parse_reply (sys/net/application_layer/dhcpv6/client.c:1056)

Issues/PRs references

miri64

Testing and reasoning provided seem sensible, however, when testing with

USE_DHCPV6=1 make -C examples/gnrc_border_router -j flash term

I don't get a prefix from the Kea server, even when waiting for a while or after a reboot. On current master I basically get a prefix immediately.

miri64 · 2022-07-11T15:05:24Z

(I ran the application on native)

The _parse_reply function iterates over the DHCPv6 message options twice but only performs sanity checks on the option length in the first iteration. As such, both loop iterations need to be identical. Unfortunately, there aren't without this commit as (1) they use different maximum length values and (2) the first iteration stops parsing as soon as it encounters a zero option while the second doesn't. As such, it is possible for out-of-bounds read to be performed by the second loop iteration. This commit fixes this.

nmeum · 2022-07-12T08:02:44Z

I don't get a prefix from the Kea server, even when waiting for a while or after a reboot. On current master I basically get a prefix immediately.

Thanks testing this and pointing me to this test case. The problem was: I didn't update the len variable before the second parsing loop. Since the first parsing loop decrements len until it is zero, the body of the second parsing loop was never entered. Should be fixed now.

I am still not sure why this special handling for options with option type zero is needed though. I couldn't find a mention of this option type in RFC 8415. However, with the changes I just pushed I do seem to be able to retrieve a lease from Kea again.

miri64 · 2022-07-12T08:47:00Z

I am still not sure why this special handling for options with option type zero is needed though. I couldn't find a mention of this option type in RFC 8415.

It was introduced in #17736, so you have to ask @benpicco and @JKRhb where they got this from. My hunch: a try to hotfix the issue you are trying to fix proper here.

miri64 · 2022-07-12T08:51:54Z

Thanks testing this and pointing me to this test case. The problem was: I didn't update the len variable before the second parsing loop. Since the first parsing loop decrements len until it is zero, the body of the second parsing loop was never entered. Should be fixed now.

[…], with the changes I just pushed I do seem to be able to retrieve a lease from Kea again.

Confirmed.

Tested and problem was addressed.

benpicco · 2022-07-12T08:58:28Z

The added len = orig_len - sizeof(dhcpv6_msg_t); might indeed be the proper fix. I'm not in the office for the next couple days, but then I'll see if that makes our TP-Link router happy without the 0 terminator 'option'.

miri64 · 2022-07-12T09:00:57Z

It was introduced in #17736, so you have to ask @benpicco and @JKRhb where they got this from. My hunch: a try to hotfix the issue you are trying to fix proper here.

Documenting my offline discussion with @benpicco: The full explanation can be found here #17736 (review)

miri64 · 2022-07-12T09:03:11Z

At least in Wireshark, I do not see the 0 type option you claim Kea emits in #17736 (comment).

benpicco · 2022-07-12T09:04:59Z

Then this might indeed have been an artifact of the bogus len

miri64 · 2022-07-12T09:05:19Z

I open an issue, so we can get ahead with this PR.

nmeum · 2022-07-12T09:08:38Z

I open an issue, so we can get ahead with this PR.

Thanks, we can still remove the zero option check later on. This PR should fix the undefined behavior in the option parser and not removing the zero check for now might also make it easier to backport this to the last release.

miri64 · 2022-07-12T09:09:30Z

I open an issue, so we can get ahead with this PR.

See #18309.

miri64

Let's merge it as is for now and investigate #18309 later.

miri64 · 2022-07-12T09:20:19Z

For good measure, I also ran the tests on native while waiting for Murdock:

make -C tests/gnrc_dhcpv6_client flash test-with-config
make -C tests/gnrc_dhcpv6_client_6lbr flash test-as-root
make -C tests/gnrc_dhcpv6_client_stateless flash test-as-root
make -C tests/gnrc_dhcpv6_relay flash test-as-root

All still succeed.

miri64 · 2022-07-12T21:10:07Z

Thanks for the fix @nmeum!

@benpicco

As far as I can tell, no DHCPv6 RFC specifies this option. The handling for the zero option was added in RIOT-OS#17736 as @benpicco while trying to retrieve a DHCHPv6 lease with KEA. However, I strongly suspect that the zero option was encountered due to an out-of-bounds read performed in the dhcpv6 client implementation (i.e. the option parsing loop read beyond the packet bounds). This issue was fixed in RIOT-OS#18307 and I strongly suspect that it should also fix the issue @benpicco originally encountered in RIOT-OS#17736. As such, I propose that we remove the if statement which treats the zero option as an end-of-payload marker. Fixes RIOT-OS#18309

@benpicco

As far as I can tell, no DHCPv6 RFC specifies this option. The handling for the zero option was added in RIOT-OS#17736 by @benpicco to fix issues encountered while trying to retrieve a DHCHPv6 lease. However, I strongly suspect that the zero option was encountered in this case due to an out-of-bounds read performed in RIOT's DHCPv6 client implementation (i.e. the option parsing loop read beyond the packet bounds). This issue was fixed in RIOT-OS#18307 and I strongly suspect that it should also fix the issue @benpicco originally encountered in RIOT-OS#17736. As such, I propose that we remove the if statement which treats the zero option as an end-of-payload marker. Fixes RIOT-OS#18309

nmeum requested review from miri64, haukepetersen, PeterKietzmann and cgundogan as code owners July 11, 2022 13:54

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 11, 2022

miri64 added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 11, 2022

miri64 previously requested changes Jul 11, 2022

View reviewed changes

nmeum force-pushed the pr/gnrc_dhcpv6_client_parse_reply branch from d59b4ab to f073dcd Compare July 12, 2022 07:59

miri64 mentioned this pull request Jul 12, 2022

gnrc_dhcpv6_client: checking for 0 option in REPLY might be superfluous. #18309

Closed

miri64 approved these changes Jul 12, 2022

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 12, 2022

miri64 enabled auto-merge July 12, 2022 09:21

miri64 merged commit 11dc836 into RIOT-OS:master Jul 12, 2022

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

nmeum mentioned this pull request Sep 22, 2022

dhcpv6: don't treat zero option as an end-of-payload marker #18625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc_dhcpv6_client: Fix out-of-bounds access during option parsing #18307

gnrc_dhcpv6_client: Fix out-of-bounds access during option parsing #18307

nmeum commented Jul 11, 2022

miri64 left a comment

miri64 commented Jul 11, 2022 •

edited

Loading

nmeum commented Jul 12, 2022

miri64 commented Jul 12, 2022

miri64 commented Jul 12, 2022

benpicco commented Jul 12, 2022 •

edited

Loading

miri64 commented Jul 12, 2022 •

edited

Loading

miri64 commented Jul 12, 2022

benpicco commented Jul 12, 2022

miri64 commented Jul 12, 2022

nmeum commented Jul 12, 2022 •

edited

Loading

miri64 commented Jul 12, 2022

miri64 left a comment

miri64 commented Jul 12, 2022

miri64 commented Jul 12, 2022

	for (dhcpv6_opt_t opt = (dhcpv6_opt_t )(&rep[sizeof(dhcpv6_msg_t)]);
	len > 0; len -= _opt_len(opt), opt = _opt_next(opt)) {
	if (len > orig_len) {
	DEBUG("DHCPv6 client: ADVERTISE options overflow packet boundaries\n");
	return false;
	}
	switch (byteorder_ntohs(opt->type)) {
	case DHCPV6_OPT_CID:
	cid = (dhcpv6_opt_duid_t *)opt;
	break;
	case DHCPV6_OPT_SID:
	sid = (dhcpv6_opt_duid_t *)opt;
	break;
	case DHCPV6_OPT_STATUS:
	status = (dhcpv6_opt_status_t *)opt;
	break;
	case DHCPV6_OPT_SMR:
	smr = (dhcpv6_opt_smr_t *)opt;
	break;
	case DHCPV6_OPT_IMR:
	imr = (dhcpv6_opt_imr_t *)opt;
	break;
	case DHCPV6_OPT_IRT:
	irt = (dhcpv6_opt_irt_t *)opt;
	break;
	default:
	break;
	}
	/* 0 option is used as an end marker, len can include bogus bytes */
	if (!byteorder_ntohs(opt->type)) {
	break;
	}
	}

	len = orig_len - sizeof(dhcpv6_msg_t);
	for (dhcpv6_opt_t opt = (dhcpv6_opt_t )(&rep[sizeof(dhcpv6_msg_t)]);
	len > 0; len -= _opt_len(opt), opt = _opt_next(opt)) {
	switch (byteorder_ntohs(opt->type)) {
	#if IS_USED(MODULE_DHCPV6_CLIENT_DNS)
	case DHCPV6_OPT_DNS_RNS:
	dhcpv6_client_dns_rns_conf((dhcpv6_opt_dns_rns_t *)opt,
	remote.netif);
	break;
	#endif /* IS_USED(MODULE_DHCPV6_CLIENT_DNS) */
	case DHCPV6_OPT_IA_PD:
	if (_parse_ia_pd_option((dhcpv6_opt_ia_pd_t *)opt)) {
	/* No error occurred */
	break;
	} else {
	/* Something went wrong */
	return false;
	}
	case DHCPV6_OPT_IA_NA:
	if (_parse_ia_na_option((dhcpv6_opt_ia_na_t *)opt)) {
	/* No error occurred */
	break;
	} else {
	/* Something went wrong */
	return false;
	}
	default:
	break;
	}
	}

gnrc_dhcpv6_client: Fix out-of-bounds access during option parsing #18307

gnrc_dhcpv6_client: Fix out-of-bounds access during option parsing #18307

Conversation

nmeum commented Jul 11, 2022

Contribution description

Testing procedure

Issues/PRs references

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Jul 11, 2022 • edited Loading

nmeum commented Jul 12, 2022

miri64 commented Jul 12, 2022

miri64 commented Jul 12, 2022

benpicco commented Jul 12, 2022 • edited Loading

miri64 commented Jul 12, 2022 • edited Loading

miri64 commented Jul 12, 2022

benpicco commented Jul 12, 2022

miri64 commented Jul 12, 2022

nmeum commented Jul 12, 2022 • edited Loading

miri64 commented Jul 12, 2022

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Jul 12, 2022

miri64 commented Jul 12, 2022

miri64 commented Jul 11, 2022 •

edited

Loading

benpicco commented Jul 12, 2022 •

edited

Loading

miri64 commented Jul 12, 2022 •

edited

Loading

nmeum commented Jul 12, 2022 •

edited

Loading