Fix coap_parse_message() #640
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
As reported by Chongqing Lei, coap_parse_message() does not check whether the number of bytes provided by `data` `data_len`) matches or exceeds the minimal CoAP message size before starting to parse the header using the bytes pointed at by the `data` parameter. This can lead to OOB reads, causing all kinds of (remote exploitable) issues. Example of a DoS attack: 1) Start lwm2mserver: `$ examples/server/lwm2mserver` 2) Connect to it `$ echo -e -n '\x6e\x8d' | nc -6 -u localhost 5683` 3) lwm2mserver output: ``` > 2 bytes received from [::1]:44955 6E 8D n. Segmentation fault ```
As reported by Chongqing Lei, coap_parse_message() overruns the options field in coap_packet_t when the option number is exactly 40.
Testing is not exhaustive, yet surfaces some issues within the function. This commit has been inspired by and includes a test for an issue found by Chongqing Lei: coap_parse_message() does not check data_len if data_len is less than COAP_HEADER_LEN. This shortcoming is (trivially) remote exploitable. An example of a DoS attack is included in the report and has been added to the unit tests. The exposed issues will be fixed by upcoming patches.
This fixes some buffer over-read issues uncovered by: - reading the code - unit tests introduced for coap_parse_message() - fuzzing Chances are that there are even more issues.
rettichschnidi
force-pushed
the
gardena/rs/fix-coap_parse_message
branch
from
cc2aa8d
to
b30e097
Compare
RFC 7252 states: > The presence of a marker followed by a zero-length payload MUST be > processed as a message format error.
RFC 7252 regarding Empty Messages: > A message with a Code of 0.00; neither a request nor a response. > An Empty message only contains the 4-byte header. Note: Erroring on requests or responses is not not implemented.
RFC 7252 states: > Lengths 9-15 are reserved, MUST NOT be sent, and MUST be processed as > a message format error.
RFC 7252 on Option Delta of 15: > Reserved for the Payload Marker. If the field is set to this value but > the entire byte is not the payload marker, this MUST be processed as a > message format error. RFC 7252 on Option Length of 15: > Reserved for future use. If the field is set to this value, it MUST be > processed as a message format error.
rettichschnidi
force-pushed
the
gardena/rs/fix-coap_parse_message
branch
from
b30e097
to
5a7f7af
Compare
mlasch
approved these changes
Jan 13, 2022
There was a problem hiding this comment.
Nice work @rettichschnidi! I did some smoke testing and it looks good to me. Also nice to have unittest coverage for the message parser.
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.
@VoodooChild99 has reported buffer overflows and over-reads in coap_parse_message() [1]. As this method is parsing data received over the network (internet), attackers can exploit this weakness.
This PR fixes [1], adds some unit tests for coap_parse_message() and fixes issues found while working this method. More details can be found in the respective commit messages.
Thanks @VoodooChild99 for letting us know and helping on getting this fixed!
[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=577968