From 5fe7a2e44b8cd6714a0f1169fdfa6ee3ec5c5481 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 12 Dec 2024 14:21:13 +0100 Subject: [PATCH 1/5] cpu/avr8_common: fix C++ compatibility of unistd.h C++ does not know about `restrict`, but both g++ and clang++ support `__restrict`, as do `clang` and GCC [1]. Using `__restrict` instead of `restrict` is also what glibc does. [1]: https://en.wikipedia.org/wiki/Restrict#Support_by_C++_compilers --- cpu/avr8_common/avr_libc_extra/include/unistd.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpu/avr8_common/avr_libc_extra/include/unistd.h b/cpu/avr8_common/avr_libc_extra/include/unistd.h index 39049c605f99..dfe62e4ec07e 100644 --- a/cpu/avr8_common/avr_libc_extra/include/unistd.h +++ b/cpu/avr8_common/avr_libc_extra/include/unistd.h @@ -80,8 +80,8 @@ int pipe(int [2]); ssize_t pread(int, void *, size_t, off_t); ssize_t pwrite(int, const void *, size_t, off_t); ssize_t read(int, void *, size_t); -ssize_t readlink(const char *restrict, char *restrict, size_t); -ssize_t readlinkat(int, const char *restrict, char *restrict, size_t); +ssize_t readlink(const char *__restrict, char *__restrict, size_t); +ssize_t readlinkat(int, const char *__restrict, char *__restrict, size_t); int rmdir(const char *); int setegid(gid_t); int seteuid(uid_t); From 7a738d0e0bd9628d7a8a079fb61b463e5eb38917 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 11 Dec 2024 15:38:00 +0100 Subject: [PATCH 2/5] sys/net/nanocoap: fix buffer overflow in separate response handling When RFC 8974 support (module `nanocoap_token_ext`) is in use, the request token may be longer than the buffer in the separate response context is large. This adds a check to not overflow the buffer. Sadly, this is an API change: Preparing the separate response context can actually fail, so we need to report this with a return value. The example application has been adapted to only proceed if the separate reply context could have been prepared, and rather directly emit a reset message if the token exceeds the static buffer. Co-authored-by: benpicco --- examples/nanocoap_server/coap_handler.c | 8 ++++++-- sys/include/net/nanocoap_sock.h | 10 ++++++++-- sys/net/application_layer/nanocoap/sock.c | 21 +++++++++++++++++---- tests/net/nanocoap_cli/Makefile | 1 + tests/net/nanocoap_cli/nanocli_client.c | 1 + 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/examples/nanocoap_server/coap_handler.c b/examples/nanocoap_server/coap_handler.c index 7b6b480f5afd..f40a425362c2 100644 --- a/examples/nanocoap_server/coap_handler.c +++ b/examples/nanocoap_server/coap_handler.c @@ -209,9 +209,13 @@ static ssize_t _separate_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, coap return coap_build_reply(pkt, COAP_CODE_SERVICE_UNAVAILABLE, buf, len, 0); } - puts("_separate_handler(): send ACK, schedule response"); + if (nanocoap_server_prepare_separate(&_separate_ctx, pkt, context)) { + puts("_separate_handler(): failed to prepare context for separate response"); + /* send a reset message, as we don't support large tokens here */ + return coap_build_reply(pkt, 0, buf, len, 0); + } - nanocoap_server_prepare_separate(&_separate_ctx, pkt, context); + puts("_separate_handler(): send ACK, schedule response"); event_timeout_ztimer_init(&event_timeout, ZTIMER_MSEC, EVENT_PRIO_MEDIUM, &event_timed.super); diff --git a/sys/include/net/nanocoap_sock.h b/sys/include/net/nanocoap_sock.h index fc1b2d70c2d1..ba3e8664aa9f 100644 --- a/sys/include/net/nanocoap_sock.h +++ b/sys/include/net/nanocoap_sock.h @@ -249,9 +249,15 @@ typedef struct { * @param[out] ctx Context information for separate response * @param[in] pkt CoAP packet to which the response will be generated * @param[in] req Context of the CoAP request + * + * @retval 0 Success + * @retval -EOVERFLOW Storing context would have overflown buffers in @p ctx + * (e.g. RFC 8974 (module `nanocoap_token_ext`) is in + * use and token too long) + * @retval <0 Other error */ -void nanocoap_server_prepare_separate(nanocoap_server_response_ctx_t *ctx, - coap_pkt_t *pkt, const coap_request_ctx_t *req); +int nanocoap_server_prepare_separate(nanocoap_server_response_ctx_t *ctx, + coap_pkt_t *pkt, const coap_request_ctx_t *req); /** * @brief Send a separate response to a CoAP request diff --git a/sys/net/application_layer/nanocoap/sock.c b/sys/net/application_layer/nanocoap/sock.c index 02646001f7c1..c3e16ac24d36 100644 --- a/sys/net/application_layer/nanocoap/sock.c +++ b/sys/net/application_layer/nanocoap/sock.c @@ -1075,11 +1075,22 @@ void auto_init_nanocoap_server(void) nanocoap_server_start(&local); } -void nanocoap_server_prepare_separate(nanocoap_server_response_ctx_t *ctx, - coap_pkt_t *pkt, const coap_request_ctx_t *req) +int nanocoap_server_prepare_separate(nanocoap_server_response_ctx_t *ctx, + coap_pkt_t *pkt, const coap_request_ctx_t *req) { - ctx->tkl = coap_get_token_len(pkt); - memcpy(ctx->token, coap_get_token(pkt), ctx->tkl); + size_t tkl = coap_get_token_len(pkt); + if (tkl > sizeof(ctx->token)) { + DEBUG_PUTS("nanocoap: token too long for separate response ctx"); + /* Legacy code may not check the return value. To still have somewhat + * sane behavior, we ask for no response for any response class. + * Getting no reply is certainly not ideal, but better than one without + * a matching token. */ + memset(ctx, 0, sizeof(*ctx)); + ctx->no_response = 0xff; + return -EOVERFLOW; + } + ctx->tkl = tkl; + memcpy(ctx->token, coap_get_token(pkt), tkl); memcpy(&ctx->remote, req->remote, sizeof(ctx->remote)); #ifdef MODULE_SOCK_AUX_LOCAL assert(req->local); @@ -1088,6 +1099,8 @@ void nanocoap_server_prepare_separate(nanocoap_server_response_ctx_t *ctx, uint32_t no_response = 0; coap_opt_get_uint(pkt, COAP_OPT_NO_RESPONSE, &no_response); ctx->no_response = no_response; + + return 0; } int nanocoap_server_send_separate(const nanocoap_server_response_ctx_t *ctx, diff --git a/tests/net/nanocoap_cli/Makefile b/tests/net/nanocoap_cli/Makefile index cf7031ffee17..442ba186994c 100644 --- a/tests/net/nanocoap_cli/Makefile +++ b/tests/net/nanocoap_cli/Makefile @@ -15,6 +15,7 @@ USEMODULE += gnrc_ipv6_default USEMODULE += nanocoap_sock USEMODULE += nanocoap_resources +USEMODULE += fmt # scn_buf_hex() for shell cmd client_token # boards where basic nanocoap functionality fits, but no VFS LOW_MEMORY_BOARDS := \ diff --git a/tests/net/nanocoap_cli/nanocli_client.c b/tests/net/nanocoap_cli/nanocli_client.c index 3a67e1511334..c6f64e418d00 100644 --- a/tests/net/nanocoap_cli/nanocli_client.c +++ b/tests/net/nanocoap_cli/nanocli_client.c @@ -23,6 +23,7 @@ #include #include +#include "fmt.h" #include "net/coap.h" #include "net/gnrc/netif.h" #include "net/nanocoap.h" From 6b1279348a65ede842425b6840cc6007246e9025 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 11 Dec 2024 20:05:15 +0100 Subject: [PATCH 3/5] sys/fmt: add scn_buf_hex() This adds a function to convert a hex string to a byte array. --- sys/fmt/fmt.c | 59 ++++++++++++++++++++++++++- sys/include/fmt.h | 30 +++++++++++++- tests/unittests/tests-fmt/tests-fmt.c | 29 +++++++++++++ 3 files changed, 115 insertions(+), 3 deletions(-) diff --git a/sys/fmt/fmt.c b/sys/fmt/fmt.c index 9ed05f7fc32d..c02acdf9c64e 100644 --- a/sys/fmt/fmt.c +++ b/sys/fmt/fmt.c @@ -19,14 +19,16 @@ */ #include -#include +#include +#include #include #include #include #include -#include "kernel_defines.h" +#include "container.h" #include "fmt.h" +#include "modules.h" extern ssize_t stdio_write(const void* buffer, size_t len); @@ -512,6 +514,59 @@ uint32_t scn_u32_hex(const char *str, size_t n) return res; } +static bool _get_nibble(uint8_t *dest, char _c) +{ + uint8_t c = _c; + if (((uint8_t)'0' <= c) && (c <= (uint8_t)'9')) { + *dest = c - (uint8_t)'0'; + return true; + } + + if (((uint8_t)'a' <= c) && (c <= (uint8_t)'f')) { + *dest = c - (uint8_t)'a' + 10; + return true; + } + + if (((uint8_t)'A' <= c) && (c <= (uint8_t)'F')) { + *dest = c - (uint8_t)'A' + 10; + return true; + } + + return false; +} + +ssize_t scn_buf_hex(void *_dest, size_t dest_len, const char *hex, size_t hex_len) +{ + uint8_t *dest = _dest; + assert((dest != NULL) || (dest_len == 0)); + assert((hex != NULL) || (hex_len == 0)); + + if (hex_len & 1) { + /* we need to chars per every byte, so odd inputs don't work */ + return -EINVAL; + } + + size_t len = hex_len >> 1; + if (len > dest_len) { + return -EOVERFLOW; + } + + for (size_t pos = 0; pos < len; pos++) { + uint8_t high, low; + if (!_get_nibble(&high, hex[pos << 1])) { + return -EINVAL; + } + + if (!_get_nibble(&low, hex[(pos << 1) + 1])) { + return -EINVAL; + } + + dest[pos] = (high << 4) | low; + } + + return len; +} + /* native gets special treatment as native's stdio code is ... special. * And when not building for RIOT, there's no `stdio_write()`. * In those cases, just defer to `printf()`. diff --git a/sys/include/fmt.h b/sys/include/fmt.h index afc99bf3ef75..e0132510756a 100644 --- a/sys/include/fmt.h +++ b/sys/include/fmt.h @@ -41,8 +41,9 @@ #ifndef FMT_H #define FMT_H -#include #include +#include +#include #ifdef __cplusplus extern "C" { @@ -427,6 +428,33 @@ uint32_t scn_u32_dec(const char *str, size_t n); */ uint32_t scn_u32_hex(const char *str, size_t n); +/** + * @brief Convert a hex to binary + * + * @param[out] dest Destination buffer to write to + * @param[in] dest_len Size of @p dest in bytes + * @param[in] hex Hex string to convert + * @param[in] hex_len Length of @p hex in bytes + * + * @return Number of bytes written + * @retval -EINVAL @p hex_len is odd or @p hex contains non-hex chars + * @retval -EOVERFLOW Destination to small + * + * @pre If @p dest_len is > 0, @p dest is not a null pointer + * @pre If @p hex_len is > 0, @p hex is not a null pointer + * + * Examples + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~{.c} + * const char *hex = "deadbeef"; + * uint8_t binary[sizeof(hex) / 2]; + * ssize_t len = scn_buf_hex(binary, sizeof(binary), hex, strlen(hex)); + * if (len >= 0) { + * make_use_of(binary, len); + * } + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + */ +ssize_t scn_buf_hex(void *dest, size_t dest_len, const char *hex, size_t hex_len); + /** * @brief Print string to stdout * diff --git a/tests/unittests/tests-fmt/tests-fmt.c b/tests/unittests/tests-fmt/tests-fmt.c index 5606a2b0e23b..c9d4b45d9863 100644 --- a/tests/unittests/tests-fmt/tests-fmt.c +++ b/tests/unittests/tests-fmt/tests-fmt.c @@ -869,6 +869,34 @@ static void test_scn_u32_hex(void) TEST_ASSERT_EQUAL_INT(0xab, scn_u32_hex("aB", 9)); } +static void test_scn_buf_hex(void) +{ + uint8_t buf[8]; + const char *input_invalid = "hallo"; + const char *input_valid = "deadbeef"; + const uint8_t expected[] = { 0xde, 0xad, 0xbe, 0xef }; + const size_t len_valid = strlen(input_valid); + const size_t len_invalid = strlen(input_invalid); + const size_t len_expected = sizeof(expected); + + /* invalid due to odd length */ + TEST_ASSERT_EQUAL_INT(-EINVAL, scn_buf_hex(buf, sizeof(buf), + input_valid, len_valid - 1)); + /* invalid due to non-hex chars */ + TEST_ASSERT_EQUAL_INT(-EINVAL, scn_buf_hex(buf, sizeof(buf), + input_invalid, len_invalid)); + /* overflow */ + TEST_ASSERT_EQUAL_INT(-EOVERFLOW, scn_buf_hex(buf, 2, + input_valid, len_valid)); + + memset(buf, 0x55, sizeof(buf)); + TEST_ASSERT_EQUAL_INT(len_expected, scn_buf_hex(buf, sizeof(buf), + input_valid, len_valid)); + TEST_ASSERT(0 == memcmp(expected, buf, len_expected)); + /* did not overwrite */ + TEST_ASSERT_EQUAL_INT(0x55, buf[len_expected]); +} + static void test_fmt_lpad(void) { const char base[] = "abcd"; @@ -935,6 +963,7 @@ Test *tests_fmt_tests(void) new_TestFixture(test_fmt_to_lower), new_TestFixture(test_scn_u32_dec), new_TestFixture(test_scn_u32_hex), + new_TestFixture(test_scn_buf_hex), new_TestFixture(test_fmt_lpad), }; From fa011a8a1a5d6492a2377490065acb9d0743868d Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 11 Dec 2024 18:30:50 +0100 Subject: [PATCH 4/5] tests/net/nanocoap_cli: allow specifying the token This adds the client_token shell command that allows to specify the CoAP Token. This is particularly useful to test extended length Tokens, as enabled with module `nanocoap_token_ext`. Co-authored-by: benpicco --- tests/net/nanocoap_cli/nanocli_client.c | 38 +++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/tests/net/nanocoap_cli/nanocli_client.c b/tests/net/nanocoap_cli/nanocli_client.c index c6f64e418d00..a9df7ab54765 100644 --- a/tests/net/nanocoap_cli/nanocli_client.c +++ b/tests/net/nanocoap_cli/nanocli_client.c @@ -81,6 +81,14 @@ static ssize_t _send(coap_pkt_t *pkt, size_t len, return nanocoap_request(pkt, NULL, &remote, len); } +#if MODULE_NANOCOAP_TOKEN_EXT +# define CLIENT_TOKEN_LENGTH_MAX 16 +#else +# define CLIENT_TOKEN_LENGTH_MAX COAP_TOKEN_LENGTH_MAX +#endif +static uint8_t _client_token[CLIENT_TOKEN_LENGTH_MAX] = {0xDA, 0xEC}; +static uint8_t _client_token_len = 2; + static int _cmd_client(int argc, char **argv) { /* Ordered like the RFC method code numbers, but off by 1. GET is code 0. */ @@ -89,7 +97,6 @@ static int _cmd_client(int argc, char **argv) uint8_t buf[buflen]; coap_pkt_t pkt; size_t len; - uint8_t token[2] = {0xDA, 0xEC}; if (argc == 1) { /* show help for commands */ @@ -110,7 +117,8 @@ static int _cmd_client(int argc, char **argv) /* parse options */ if (argc == 5 || argc == 6) { - ssize_t hdrlen = coap_build_hdr(pkt.hdr, COAP_TYPE_CON, &token[0], 2, + ssize_t hdrlen = coap_build_hdr(pkt.hdr, COAP_TYPE_CON, + _client_token, _client_token_len, code_pos+1, 1); coap_pkt_init(&pkt, &buf[0], buflen, hdrlen); coap_opt_add_string(&pkt, COAP_OPT_URI_PATH, argv[4], '/'); @@ -165,9 +173,33 @@ static int _cmd_client(int argc, char **argv) argv[0]); return 1; } - SHELL_COMMAND(client, "CoAP client", _cmd_client); +static int _cmd_client_token(int argc, char **argv){ + if (argc != 2) { + printf("Usage: %s \n", argv[0]); + return 1; + } + + ssize_t tkl = scn_buf_hex(_client_token, sizeof(_client_token), + argv[1], strlen(argv[1])); + + if (tkl == -EOVERFLOW) { + puts("Token too long"); + return 1; + } + + if (tkl < 0) { + puts("Failed to parse token"); + return 1; + } + + _client_token_len = tkl; + + return 0; +} +SHELL_COMMAND(client_token, "Set Token for CoAP client", _cmd_client_token); + static int _blockwise_cb(void *arg, size_t offset, uint8_t *buf, size_t len, int more) { From c7af4b25a6974ee12ba8cd0282b6997e51633ff7 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 11 Dec 2024 20:20:11 +0100 Subject: [PATCH 5/5] sys/net/nanocoap: fix invalid RST messages An RST message has no token, so don't reply with a token when sending RST. This also adds unit tests to ensure this this exact bug does not sneak back in. --- sys/net/application_layer/nanocoap/nanocoap.c | 22 +++++----- .../unittests/tests-nanocoap/tests-nanocoap.c | 43 +++++++++++++++++++ 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index 262a01c1589d..5f6a6e8f0d29 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -656,23 +656,23 @@ ssize_t coap_build_reply(coap_pkt_t *pkt, unsigned code, uint8_t *rbuf, unsigned rlen, unsigned payload_len) { unsigned tkl = coap_get_token_len(pkt); + unsigned type = COAP_TYPE_NON; + + if (!code) { + /* if code is COAP_CODE_EMPTY (zero), assume Reset (RST) type. + * RST message have no token */ + type = COAP_TYPE_RST; + tkl = 0; + } + else if (coap_get_type(pkt) == COAP_TYPE_CON) { + type = COAP_TYPE_ACK; + } unsigned len = sizeof(coap_hdr_t) + tkl; if ((len + payload_len) > rlen) { return -ENOSPC; } - /* if code is COAP_CODE_EMPTY (zero), assume Reset (RST) type */ - unsigned type = COAP_TYPE_RST; - if (code) { - if (coap_get_type(pkt) == COAP_TYPE_CON) { - type = COAP_TYPE_ACK; - } - else { - type = COAP_TYPE_NON; - } - } - uint32_t no_response; if (coap_opt_get_uint(pkt, COAP_OPT_NO_RESPONSE, &no_response) == 0) { diff --git a/tests/unittests/tests-nanocoap/tests-nanocoap.c b/tests/unittests/tests-nanocoap/tests-nanocoap.c index 098e24325165..b6493b7b762c 100644 --- a/tests/unittests/tests-nanocoap/tests-nanocoap.c +++ b/tests/unittests/tests-nanocoap/tests-nanocoap.c @@ -1185,6 +1185,48 @@ static void test_nanocoap__token_length_ext_269(void) TEST_ASSERT_EQUAL_INT(14, hdr->ver_t_tkl & 0xf); } +/* + * Test that a RST message can be generated and parsed + */ +static void test_nanocoap___rst_message(void) +{ + static const uint8_t rst_expected[4] = { + 0x70, /* Version = 0b01, Type = 0b11 (RST), Token Length = 0b0000 */ + 0x00, /* Code = 0x00 */ + 0x13, 0x37 /* Message ID = 0x1337 */ + }; + + uint8_t buf[16]; + /* trivial case: build a reset message */ + memset(buf, 0x55, sizeof(buf)); + TEST_ASSERT_EQUAL_INT(sizeof(rst_expected), + coap_build_hdr((void *)buf, COAP_TYPE_RST, NULL, 0, + 0, 0x1337)); + TEST_ASSERT(0 == memcmp(rst_expected, buf, sizeof(rst_expected))); + /* did it write past the expected bytes? */ + TEST_ASSERT_EQUAL_INT(0x55, buf[sizeof(rst_expected)]); + + /* now check that parsing it back works */ + coap_pkt_t pkt; + TEST_ASSERT_EQUAL_INT(0, coap_parse(&pkt, buf, sizeof(rst_expected))); + TEST_ASSERT_EQUAL_INT(COAP_TYPE_RST, coap_get_type(&pkt)); + TEST_ASSERT_EQUAL_INT(0, coap_get_code_raw(&pkt)); + TEST_ASSERT_EQUAL_INT(0, coap_get_token_len(&pkt)); + + /* now check that generating a RST reply works */ + static uint8_t con_request[8] = { + 0x44, /* Version = 0b01, Type = 0b00 (CON), Token Length = 0b0100 */ + 0x01, /* Code = 0.01 (GET) */ + 0x13, 0x37, /* Message ID = 0x1337 */ + 0xde, 0xed, 0xbe, 0xef, /* Token = 0xdeadbeef */ + }; + memset(buf, 0x55, sizeof(buf)); + TEST_ASSERT_EQUAL_INT(0, coap_parse(&pkt, con_request, sizeof(con_request))); + TEST_ASSERT_EQUAL_INT(sizeof(rst_expected), coap_build_reply(&pkt, 0, buf, sizeof(buf), 0)); + TEST_ASSERT(0 == memcmp(rst_expected, buf, sizeof(rst_expected))); + TEST_ASSERT_EQUAL_INT(0x55, buf[sizeof(rst_expected)]); +} + Test *tests_nanocoap_tests(void) { EMB_UNIT_TESTFIXTURES(fixtures) { @@ -1223,6 +1265,7 @@ Test *tests_nanocoap_tests(void) new_TestFixture(test_nanocoap__token_length_over_limit), new_TestFixture(test_nanocoap__token_length_ext_16), new_TestFixture(test_nanocoap__token_length_ext_269), + new_TestFixture(test_nanocoap___rst_message), }; EMB_UNIT_TESTCALLER(nanocoap_tests, NULL, NULL, fixtures);