nanocoap: fix coap_block2_finish() when option size is shrinking

[benpicco]

Contribution description

The CoAP block option gets written twice: First a 'dummy' value is written by coap_opt_add_block2(), later this gets overwritten by the real option value by coap_block2_finish().

The problem arises when the size of the option changes. If the option ends up smaller than the dummy, we have garbage bytes after the real option value, corrupting the packet.

To fix this, move the packet contents in case the option size shrank.

Testing procedure

Issues/PRs references

fixes #20686

[riot-ci]

Murdock results

:heavy_check_mark: PASSED

1c9b17fc15f0e289bdfd2568c5e190b85eef94b5 nanocoap_fileserver: adjust pkt length if CoAP header shrunk

Success	Failures	Total	Runtime
10104	0	10105	14m:48s

Artifacts

• Documentation preview

[benpicco]

This now changes the API of coap_block_finish() so the caller can account for the removed bytes. I couldn't find any case where the old return value would be used, so maybe this is fine?

[fabian18]

I think when there is an option after the BLOCK2 option the memmove is still not correct. I added a SIZE2 option afterwards and tried to fix the issue. The part to be moved depends on everything after the re-encoded option, including payload, so I think a memove inside coap_block_finish() cannot solve the issue.

diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h
index 1c4cadbbe1..8c0f984bd2 100644
--- a/sys/include/net/nanocoap.h
+++ b/sys/include/net/nanocoap.h
@@ -988,8 +988,7 @@ void coap_block_object_init(coap_block1_t *block, size_t blknum, size_t blksize,
  * This function finalizes the block response header
  *
  * Checks whether the `more` bit should be set in the block option and
- * sets/clears it if required.  Doesn't return the number of bytes, as this
- * function overwrites bytes in the packet rather than adding new.
+ * sets/clears it if required.
  *
  * @param[in]     slicer      Preallocated slicer struct to use
  * @param[in]     option      option number (block1 or block2)
diff --git a/sys/net/application_layer/nanocoap/fileserver.c b/sys/net/application_layer/nanocoap/fileserver.c
index 414a82dc63..09e83d7c50 100644
--- a/sys/net/application_layer/nanocoap/fileserver.c
+++ b/sys/net/application_layer/nanocoap/fileserver.c
@@ -206,12 +206,14 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
     int err;
     uint32_t etag;
     coap_block1_t block2 = { .szx = CONFIG_NANOCOAP_BLOCK_SIZE_EXP_MAX };
+    size_t resp_len;
     {
         struct stat stat;
         if ((err = vfs_stat(request->namebuf, &stat)) < 0) {
             return _error_handler(pdu, buf, len, err);
         }
         stat_etag(&stat, &etag);
+        resp_len = stat.st_size;
     }
     if (request->options.exists.block2 && !coap_get_block2(pdu, &block2)) {
         return _error_handler(pdu, buf, len, COAP_CODE_BAD_OPTION);
@@ -240,7 +242,8 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
                &block2);
     coap_block_slicer_init(&slicer, block2.blknum, coap_szx2size(block2.szx));
     coap_opt_add_block2(pdu, &slicer, true);
-    size_t resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);
+    coap_opt_add_uint(pdu, COAP_OPT_SIZE2, resp_len);
+    resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);
 
     err = vfs_lseek(fd, slicer.start, SEEK_SET);
     if (err < 0) {
@@ -268,8 +271,19 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
 
     vfs_close(fd);
 
+    /* Solution for problem of re-encoded options with changing size:
+       Move everything behind a re-encoded option for which the size changed backwards */
+    int shrunk;
+
     slicer.cur = slicer.end + more;
-    resp_len -= coap_block2_finish(&slicer);
+    if ((shrunk = coap_block2_finish(&slicer))) {
+        resp_len -= shrunk;
+        memmove(slicer.opt + slicer.obytes,
+                slicer.opt + slicer.obytes + shrunk,
+                (pdu->payload + read) - (slicer.opt + slicer.obytes + shrunk));
+        pdu->payload -= shrunk;
+    }
 
     if (read == 0) {
         /* Rewind to clear payload marker */
diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c
index 3d9f2121b9..ef845c2b3e 100644
--- a/sys/net/application_layer/nanocoap/nanocoap.c
+++ b/sys/net/application_layer/nanocoap/nanocoap.c
@@ -1342,13 +1342,12 @@ int coap_block_finish(coap_block_slicer_t *slicer, uint16_t option)
 
     uint8_t obytes = coap_put_option(slicer->opt, option - delta, option,
                                      (uint8_t *)&blkopt, olen);
-    if (obytes != slicer->obytes) {
-        /* if the option grew larger, we already corrupted content */
-        assert(obytes < slicer->obytes);
-        memmove(slicer->opt + obytes, slicer->opt + slicer->obytes,
-                1 + slicer->cur - slicer->obytes);
-    }
-    return slicer->obytes - obytes;
+
+    /* if the option grew larger, we already corrupted content */
+    assert(obytes <= slicer->obytes);
+    int odiff = slicer->obytes - obytes;
+    slicer->obytes = obytes;
+    return odiff;
 }
 
 ssize_t coap_block2_build_reply(coap_pkt_t *pkt, unsigned code,

This is the patch I tried with. Maybe you still find another issue. The other places of coap_block<x>_finish() may also need to be updated

[benpicco]

What I don't like about that patch it that is basically is a per-application workaround :confused:

#20855 provides a much simpler fix.