Skip to content
New issue

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

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

Already on GitHub? # to your account

sys/net/nanocoap: fix buffer overflow in separate response handling [backport 2024.10] #21087

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 13, 2024

(Partial) Backport of #21075

Contribution description

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.

Testing procedure

This requires a client from master, as extending the test to allow specifying the token is not backported.

Preparation

$  sudo ./dist/tools/tapsetup/tapsetup 

Running the Server

This should be done with 2024.10-branch and this PR

$ USEMODULE=nanocoap_token_ext make BOARD=native64 -C examples/nanocoap_server && examples/nanocoap_server/bin/native64/nanocoap_server.elf -w tap1
[...]
main(): This is RIOT! (Version: 2025.01-devel-261-g75828)
RIOT nanocoap example application
Waiting for address autoconfiguration...
{"IPv6 addresses": ["<IPV6_ADDR>"]}

copy the <IPV6_ADDR> part of the output

Running the Client

This should be done with master.

$ USEMODULE=nanocoap_token_ext make BOARD=native64 -C tests/net/nanocoap_cli flash term
[...]
2024-12-11 18:10:58,750 # RIOT native interrupts/signals initialized.
2024-12-11 18:10:58,751 # RIOT native board initialized.
2024-12-11 18:10:58,751 # RIOT native hardware initialization complete.
2024-12-11 18:10:58,751 # 
2024-12-11 18:10:58,752 # main(): This is RIOT! (Version: 2025.01-devel-262-g562036-sys/net/nanocoap/buffer-overflow-separate-response)
2024-12-11 18:10:58,752 # nanocoap test app
2024-12-11 18:10:58,752 # All up, running the shell now
> client_token deadbeefdeadbeefdeadbeef
2024-12-11 18:12:11,060 # client_token deadbeefdeadbeefdeadbeef
> client get fe80::24de:d0ff:fe85:bdb2 5683 /separate
2024-12-11 18:12:27,736 # client get fe80::24de:d0ff:fe85:bdb2 5683 /separate
2024-12-11 18:12:27,736 # nanocli: sending msg ID 1, 25 bytes
2024-12-11 18:13:45,417 # nanocli: msg send failed: -110

Expected result: No segfault in the server

Issues/PRs references

Partial backport of #21075

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 <benpicco@googlemail.com>
(cherry picked from commit 7a738d0)
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master labels Dec 13, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Dec 13, 2024
@maribu
Copy link
Member Author

maribu commented Dec 13, 2024

I did test this with native64, but without cherry-pick 2b6f65a the XFA bug triggers and the contents of the XFA resources are misaligned.

I guess I should backport 2b6f65a as well, then 😅

With that fix cherry picked, the test did succeed, so the buffer overflow is fixed as promised.

@maribu maribu requested a review from benpicco December 13, 2024 20:50
@riot-ci
Copy link

riot-ci commented Dec 13, 2024

Murdock results

✔️ PASSED

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

Success Failures Total Runtime
10215 0 10215 19m:39s

Artifacts

@maribu maribu enabled auto-merge December 14, 2024 12:03
@maribu maribu added this pull request to the merge queue Dec 14, 2024
Merged via the queue into RIOT-OS:2024.10-branch with commit 90a04fc Dec 14, 2024
29 checks passed
@maribu
Copy link
Member Author

maribu commented Dec 14, 2024

Thx :-)

@maribu maribu deleted the backport/2024.10/sys/net/nanocoap/buffer-overflow-separate-response branch December 14, 2024 19:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master 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.

3 participants