Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcoap_fileserver: can't deal with 16 byte block size #20686

Closed
benpicco opened this issue May 22, 2024 · 2 comments · Fixed by #20855
Closed

gcoap_fileserver: can't deal with 16 byte block size #20686

benpicco opened this issue May 22, 2024 · 2 comments · Fixed by #20855
Assignees
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@benpicco
Copy link
Contributor

Description

When requesting 16 byte block size, the server will respond with an invalid packet.

Steps to reproduce the issue

  • run examples/gcoap_fileserver:

    make -C examples/gcoap_fileserver PORT=tap1 all term
    
  • create a dummy file

    echo "Hello World!" > examples/gcoap_fileserver/native/me
    
  • Try to access the file via e.g. ncget with a 16 byte block size

     CFLAGS=-DCONFIG_NANOCOAP_BLOCKSIZE_DEFAULT=COAP_BLOCKSIZE_16 make -C tests/net/nanocoap_cli all term
     > ncget coap://[fe80::d07c:7cff:fe6d:9441]/vfs/me -
    

Expected results

We get the response in a single block

Actual results

We get the response in a single block, but there is some garbage after the block2 option:

Wireshark Output

gcoap_fileserver.pcapng.gz

Versions

RIOT master

@benpicco benpicco changed the title gcoap_fleserver: can't deal with 16 byte block size gcoap_fileserver: can't deal with 16 byte block size May 22, 2024
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels May 22, 2024
@fabian18
Copy link
Contributor

Screenshot from 2024-05-22 21-54-49

It happens because first we assume more=true --> [block num 0, more 1, szx 0]. It is encoded as 3 bytes 0xd1, 0x06, 0x08. Meaning delta 13 + 6 , length 1. Now when we correct the more flag in coap_block_finish() the option value becomes 0. The encoded option value unsigned integer 0 has length 0. That means 0xd1 in the previous encoding becomes 0xd0. The byte sequence left in the packed buffer becomes 0xd0, 0x06, 0x08. This decodes as the correct block option and the 0x08which is a leftover of the first encoding.

The issue is that an option written to a buffer is rewritten with a smaller option length, because the option value bacame actually 0.

@benpicco
Copy link
Contributor Author

Thank you, that explanation makes perfect sense!
So the (long) option gets first written by coap_opt_add_block2(), then it is overwritten later by coap_block2_finish() with a shorter option, leaving two garbage bytes before the next option starts.

benpicco added a commit to benpicco/RIOT that referenced this issue May 22, 2024
benpicco added a commit to benpicco/RIOT that referenced this issue Sep 7, 2024
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 mitigate this, always write at least one option byte (which will be a 0 byte)
to ensure the dummy data is overwritten.

fixes RIOT-OS#20686
benpicco added a commit to benpicco/RIOT that referenced this issue Sep 7, 2024
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 mitigate this, always write at least one option byte (which will be a 0 byte)
to ensure the dummy data is overwritten.

fixes RIOT-OS#20686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
3 participants