diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index 672d31b101..455ebfe7e2 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -106,6 +106,11 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len) DEBUG("option count=%u nr=%u len=%i\n", option_count, option_nr, option_len); if (option_delta) { + if (option_count >= NANOCOAP_NOPTS_MAX) { + DEBUG("nanocoap: max nr of options exceeded\n"); + return -ENOMEM; + } + 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); diff --git a/tests/unittests/tests-nanocoap/tests-nanocoap.c b/tests/unittests/tests-nanocoap/tests-nanocoap.c index 2d0bb1a5c0..38185b2bbd 100644 --- a/tests/unittests/tests-nanocoap/tests-nanocoap.c +++ b/tests/unittests/tests-nanocoap/tests-nanocoap.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "embUnit.h" @@ -433,6 +434,90 @@ static void test_nanocoap__server_reply_simple_con(void) TEST_ASSERT_EQUAL_INT(COAP_TYPE_ACK, coap_get_type(&pkt)); } +static void test_nanocoap__server_option_count_overflow_check(void) +{ + /* this test passes a forged CoAP packet containing 42 options (provided by + * @nmeum in #10753, 42 is a random number which just needs to be higher + * than NANOCOAP_NOPTS_MAX) to coap_parse(). The used coap_pkt_t is part + * of a struct, followed by an array of 42 coap_option_t. The array is + * cleared before the call to coap_parse(). If the overflow protection is + * working, the array must still be clear after parsing the packet, and the + * proper error code (-ENOMEM) is returned. Otherwise, the parsing wrote + * past scratch.pkt, thus the array is not zeroed anymore. + */ + + static uint8_t pkt_data[] = { + 0x40, 0x01, 0x09, 0x26, 0x01, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, 0x11, 0x17, + 0x11, 0x17, 0x11, 0x17 }; + + /* ensure NANOCOAP_NOPTS_MAX is actually lower than 42 */ + TEST_ASSERT(NANOCOAP_NOPTS_MAX < 42); + + struct { + coap_pkt_t pkt; + uint8_t guard_data[42 * sizeof(coap_optpos_t)]; + } scratch; + + memset(&scratch, 0, sizeof(scratch)); + + int res = coap_parse(&scratch.pkt, pkt_data, sizeof(pkt_data)); + + /* check if any byte of the guard_data array is non-zero */ + int dirty = 0; + volatile uint8_t *pos = scratch.guard_data; + for (size_t i = 0; i < sizeof(scratch.guard_data); i++) { + if (*pos) { + dirty = 1; + break; + } + } + + TEST_ASSERT_EQUAL_INT(0, dirty); + TEST_ASSERT_EQUAL_INT(-ENOMEM, res); +} + +/* + * Verifies that coap_parse() recognizes inclusion of too many options. + */ +static void test_nanocoap__server_option_count_overflow(void) +{ + /* base pkt is a GET for /riot/value, which results in two options for the + * path, but only 1 entry in the options array. + * Size buf to accept an extra 2-byte option */ + unsigned base_len = 17; + uint8_t buf[17 + (2 * NANOCOAP_NOPTS_MAX)] = { + 0x42, 0x01, 0xbe, 0x16, 0x35, 0x61, 0xb4, 0x72, + 0x69, 0x6f, 0x74, 0x05, 0x76, 0x61, 0x6c, 0x75, + 0x65 + }; + coap_pkt_t pkt; + + /* nonsense filler option that contains a single byte of data */ + uint8_t fill_opt[] = { 0x11, 0x01 }; + + /* fill pkt with maximum options; should succeed */ + int i = 0; + for (; i < (2 * (NANOCOAP_NOPTS_MAX - 1)); i+=2) { + memcpy(&buf[base_len+i], fill_opt, 2); + } + + /* don't read final two bytes, where overflow option will be added later */ + int res = coap_parse(&pkt, buf, sizeof(buf) - 2); + TEST_ASSERT_EQUAL_INT(0, res); + + /* add option to overflow */ + memcpy(&buf[base_len+i], fill_opt, 2); + + res = coap_parse(&pkt, buf, sizeof(buf)); + TEST_ASSERT(res < 0); +} + Test *tests_nanocoap_tests(void) { EMB_UNIT_TESTFIXTURES(fixtures) { @@ -450,6 +535,8 @@ Test *tests_nanocoap_tests(void) new_TestFixture(test_nanocoap__server_reply_simple), new_TestFixture(test_nanocoap__server_get_req_con), new_TestFixture(test_nanocoap__server_reply_simple_con), + new_TestFixture(test_nanocoap__server_option_count_overflow_check), + new_TestFixture(test_nanocoap__server_option_count_overflow), }; EMB_UNIT_TESTCALLER(nanocoap_tests, NULL, NULL, fixtures);