Merge pull request #21075 from maribu/sys/net/nanocoap/buffer-overflo…

…w-separate-response

sys/net/nanocoap: fix buffer overflow in separate response handling

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);

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);

sys/fmt/fmt.c

@@ -19,14 +19,16 @@
  */
 
 #include <assert.h>
-#include <stdarg.h>
+#include <errno.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
 
-#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()`.

sys/include/fmt.h

@@ -41,8 +41,9 @@
 #ifndef FMT_H
 #define FMT_H
 
-#include <stdint.h>
 #include <stddef.h>
+#include <stdint.h>
+#include <unistd.h>
 
 #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
  *

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

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) {
 

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,

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 := \

tests/net/nanocoap_cli/nanocli_client.c

@@ -23,6 +23,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "fmt.h"
 #include "net/coap.h"
 #include "net/gnrc/netif.h"
 #include "net/nanocoap.h"
@@ -80,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. */
@@ -88,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 */
@@ -109,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], '/');
@@ -164,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 <TOKEN_HEX>\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)
 {

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),
     };