asymcute: fix one byte out-of-bounds access in _len_get#18433
Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom Aug 13, 2022
Merged
asymcute: fix one byte out-of-bounds access in _len_get#18433miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64 merged 1 commit intoRIOT-OS:masterfrom
Conversation
As per Section 5.2.1 of the MQTT-SN specification, the MQTT-SN length header is either 1- or 3-octet long. If it is 3-octet long then the first octet is 0x01. The asymcute implementation currently only checks that the incoming packet is at least 2-octet long before attempting to parse it (MIN_PKT_LEN). However, if the first octet is 0x01 the packet must be more than 3 octet long in order to be valid. Since asymcute does not check this it reads one octet beyond the packet data for a 2-octet packet where the first octet has the value 0x01. This commit fixes this issue by adding an additional sanity check to _len_get.
nmeum
requested review from
PeterKietzmann,
cgundogan,
haukepetersen and
miri64
as code owners
nmeum
force-pushed
the
pr/asymcute-length-check
branch
from
621121d to
7e6cb89
Compare
miri64
reviewed
Aug 10, 2022
benpicco
added
the
Type: bug
nmeum
force-pushed
the
pr/asymcute-length-check
branch
2 times, most recently
from
6b939d3 to
ccc8e92
Compare
miri64
approved these changes
Aug 10, 2022
Member
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
Looks good and change makes sense.
miri64
added
CI: ready for build
Member
|
❗ This may cause a semantic merge conflict with #18434, once that is merged: |
Member
|
Build error is unrelated. |
miri64
added
CI: skip compile test
Contributor
|
This will now need a rebase anyway |
nmeum
force-pushed
the
pr/asymcute-length-check
branch
from
ccc8e92 to
8d771b9
Compare
Member
Author
Rebased and made the necessary adjustments for the |
benpicco
removed
the
CI: skip compile test
nmeum
commented
Aug 12, 2022
auto-merge was automatically disabled
August 12, 2022 12:48
Head branch was pushed to by a user without write access
nmeum
force-pushed
the
pr/asymcute-length-check
branch
3 times, most recently
from
4d616c5 to
06d572c
Compare
Member
|
This needs a backport to the 2022.07 branch as well. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
As per Section 5.2.1 of the MQTT-SN specification, the MQTT-SN length header is either 1- or 3-octet long. If it is 3-octet long then the first octet is
0x01. The asymcute implementation currently only checks that the incoming packet is at least 2-octet long before attempting to parse it (MIN_PKT_LEN):RIOT/sys/net/application_layer/asymcute/asymcute.c
Line 578 in 4dcb8ed
However, if the first octet is
0x01the packet must be more than 3 octet long in order to be valid. Since asymcute does not check this it reads one octet beyond the packet data (viabyteorder_bebuftohswhich reads auint16_tin_len_get) for a 2-octet packet where the first octet has the value0x01. This commit fixes this issue by adding an additional sanity check to_len_get.Testing procedure
I think this should (hopefully) be obvious from reading the code and comparing consulting the aforementioned section in the MQTT-SN specification.
Also note that emcute has an explicit check for this:
RIOT/sys/net/application_layer/emcute/emcute.c
Lines 531 to 534 in b0f4781
Issues/PRs references
None.