sys/net/nanocoap: fix UB when building hdr #20917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maribu
added
Type: bug
github-actions
bot
added
Area: network
mguetschow
approved these changes
Oct 17, 2024
Some calls to `coap_build_hdr()` were done with the target buffer for the header and the source buffer for the token overlapping: They reuse the buffer that held the request to assemble the response in. We cannot use `memcpy()` in this case to copy the token into the target buffer, as source and destination would (fully) overlap. This commit makes reusing the request buffer for the response a special case: `memcpy()` is only used to copy the token if source and destination address of the token differ. An alternative fix would have been to use `memmove()` unconditionally. But `memmove()` does not make any assumption about the layout of target and source buffer, while we know that the token either will already be at the right position (when reusing the request buffer for the response) or be in a non-overlapping buffer (when generating a fresh token). This approach is more efficient than `memmove()`.
maribu
force-pushed
the
sys/net/nanocoap/fix-ub
branch
from
4f4ef64 to
835571c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
Some calls to
coap_build_hdr()were done with the target buffer for the header and the source buffer for the token overlapping: They reuse the buffer that held the request to assemble the response in. We cannot usememcpy()in this case to copy the token into the target buffer, as source and destination would (fully) overlap.This commit makes reusing the request buffer for the response a special case:
memcpy()is only used to copy the token if source and destination address of the token differ.An alternative fix would have been to use
memmove()unconditionally. Butmemmove()does not make any assumption about the layout of target and source buffer, while we know that the token either will already be at the right position (when reusing the request buffer for the response) or be in a non-overlapping buffer (when generating a fresh token). This approach is more efficient thanmemmove().Testing procedure
This did not cause any issues yet, so just testing for regressions should be sufficient. The CI should provide decent test coverage.
Issues/PRs references
Issue uncovered while debugging #20900