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

Jump to bottom

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

Merged
merged 5 commits into from
Dec 13, 2024
Merged

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

merged 5 commits into from
Dec 13, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 11, 2024

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

A second commit has been sneaked in to add the client_token shell command to tests/net/nanocoap_cli that allow specifying the Token as hex. The most recently set token is used in client.

Preparation

$  sudo ./dist/tools/tapsetup/tapsetup

Running the Server

This should be done with master 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 this PR only (or cherry-picking the second commit onto 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

Output

On master, this will result in

RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-261-g75828)
RIOT nanocoap example application
Waiting for address autoconfiguration...
{"IPv6 addresses": ["fe80::24de:d0ff:fe85:bdb2"]}
_separate_handler(): send ACK, schedule response
_separate_handler(): send delayed response
*** stack smashing detected ***: terminated
[1]    367561 IOT instruction (core dumped)  examples/nanocoap_server/bin/native64/nanocoap_server.elf -w tap1

With this PR, a RST message is replied and no buffer overflow / crash should appear.

Issues/PRs references

None

@maribu maribu 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 Dec 11, 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 11, 2024
@riot-ci
Copy link

riot-ci commented Dec 11, 2024 •
edited
Loading

Murdock results

✔️ PASSED

c7af4b2 sys/net/nanocoap: fix invalid RST messages

Success Failures Total Runtime
10249 0 10249 19m:26s

Artifacts

benpicco
benpicco reviewed Dec 11, 2024
View reviewed changes
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 41d8af4 to 6221061 Compare December 11, 2024 19:20
@maribu maribu requested a review from kaspar030 as a code owner December 11, 2024 19:20
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 6221061 to 7e0c535 Compare December 11, 2024 19:50
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 7e0c535 to 4c46407 Compare December 11, 2024 19:55
@maribu maribu added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Dec 11, 2024
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 4c46407 to c391765 Compare December 11, 2024 20:43
benpicco
benpicco approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Even makes then code nicer :)

@maribu maribu added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
maribu and others added 5 commits December 12, 2024 14:28
@maribu
cpu/avr8_common: fix C++ compatibility of unistd.h
5fe7a2e 
C++ does not know about `restrict`, but both g++ and clang++ support
`__restrict`, as do `clang` and GCC [1].

Using `__restrict` instead of `restrict` is also what glibc does.

[1]: https://en.wikipedia.org/wiki/Restrict#Support_by_C++_compilers
@maribu @benpicco
sys/net/nanocoap: fix buffer overflow in separate response handling
7a738d0 
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>
@maribu
sys/fmt: add scn_buf_hex()
6b12793 
This adds a function to convert a hex string to a byte array.
@maribu @benpicco
tests/net/nanocoap_cli: allow specifying the token
fa011a8 
This adds the client_token shell command that allows to specify the
CoAP Token. This is particularly useful to test extended length Tokens,
as enabled with module `nanocoap_token_ext`.

Co-authored-by: benpicco <benpicco@googlemail.com>
@maribu
sys/net/nanocoap: fix invalid RST messages
c7af4b2 
An RST message has no token, so don't reply with a token when sending
RST.

This also adds unit tests to ensure this this exact bug does not sneak
back in.
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from c391765 to c7af4b2 Compare December 12, 2024 13:28
@github-actions github-actions bot added Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports labels Dec 12, 2024
@maribu maribu enabled auto-merge December 12, 2024 13:28
@maribu maribu added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@maribu maribu added this pull request to the merge queue Dec 12, 2024
Merged via the queue into RIOT-OS:master with commit 28753e3 Dec 13, 2024
26 checks passed
@maribu
Copy link
Member Author

maribu commented Dec 13, 2024

Thx a lot!

@maribu maribu deleted the sys/net/nanocoap/buffer-overflow-separate-response branch December 13, 2024 07:21
@maribu maribu mentioned this pull request Dec 13, 2024
Merged
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@nandojve nandojve nandojve approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@kYc0o kYc0o Awaiting requested review from kYc0o kYc0o is a code owner

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports 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 Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2025.01
Development

Successfully merging this pull request may close these issues.
5 participants
@maribu @riot-ci @benpicco @nandojve @MrKevinWeiss