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

nanocoap: always write at least 1 byte in coap_block2_finish() #20855

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Sep 7, 2024

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 mitigate this, always write at least one option byte (which will be a 0 byte) to ensure the dummy data is overwritten.

Testing procedure

  • 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 -
    
2024-09-07 16:07:09,198 # ncget coap://[fe80::d07c:7cff:fe6d:9441]/vfs/me -
2024-09-07 16:07:09,199 # Hello World!

Issues/PRs references

fixes #20686
alternative to #20688

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
@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Sep 7, 2024
@benpicco benpicco requested review from chrysn and fabian18 September 7, 2024 14:13
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Sep 7, 2024
@riot-ci
Copy link

riot-ci commented Sep 7, 2024

Murdock results

✔️ PASSED

66fe083 nanocoap: always write at least 1 byte in coap_block2_finish()

Success Failures Total Runtime
10197 0 10197 20m:23s

Artifacts

@benpicco benpicco requested a review from maribu September 11, 2024 09:28
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for spotting and fixing this!

@maribu maribu enabled auto-merge September 12, 2024 15:01
@maribu maribu added this pull request to the merge queue Sep 12, 2024
Merged via the queue into RIOT-OS:master with commit 1fa7db2 Sep 12, 2024
28 checks passed
@benpicco benpicco deleted the coap_block_finish-fix branch September 12, 2024 19:44
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcoap_fileserver: can't deal with 16 byte block size
3 participants