Skip to content

asymcute: fix one byte out-of-bounds access in _len_get #18433

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 13, 2022
Merged

asymcute: fix one byte out-of-bounds access in _len_get #18433

merged 1 commit into from
Aug 13, 2022

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Aug 10, 2022

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

if (pkt_len >= 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 (via byteorder_bebuftohs which reads a uint16_t in _len_get) 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.

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

/* catch invalid length field */
if ((len == 2) && (rbuf[0] == 0x01)) {
continue;
}

Issues/PRs references

None.

@nmeum
asymcute: fix one byte out-of-bounds access in _len_get
06d572c 
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.
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Aug 10, 2022
@nmeum nmeum force-pushed the pr/asymcute-length-check branch from 621121d to 7e6cb89 Compare August 10, 2022 14:34
miri64
miri64 reviewed Aug 10, 2022
View reviewed changes
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Aug 10, 2022
@benpicco benpicco requested a review from chrysn August 10, 2022 14:59
@nmeum nmeum force-pushed the pr/asymcute-length-check branch 2 times, most recently from 6b939d3 to ccc8e92 Compare August 10, 2022 15:13
miri64
miri64 approved these changes Aug 10, 2022
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Looks good and change makes sense.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Aug 10, 2022
@miri64 miri64 enabled auto-merge August 10, 2022 16:44
@miri64 miri64 mentioned this pull request Aug 10, 2022
Merged
@miri64
Copy link
Member

miri64 commented Aug 10, 2022

❗ This may cause a semantic merge conflict with #18434, once that is merged: _get_len() gains another parameter, so only trust Murdock if it has successfully build both with either of one in master (and check if the master Murdock used includes the other PR).

@miri64
Copy link
Member

miri64 commented Aug 11, 2022

Build error is unrelated.

@miri64 miri64 added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 11, 2022
@benpicco
Copy link
Contributor

This will now need a rebase anyway

@miri64 miri64 mentioned this pull request Aug 11, 2022
Merged
@miri64 miri64 disabled auto-merge August 11, 2022 12:16
@nmeum nmeum force-pushed the pr/asymcute-length-check branch from ccc8e92 to 8d771b9 Compare August 12, 2022 08:00
@nmeum
Copy link
Member Author

nmeum commented Aug 12, 2022

This will now need a rebase anyway

Rebased and made the necessary adjustments for the _len_get function prototype.

@benpicco benpicco removed the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Aug 12, 2022
@miri64 miri64 enabled auto-merge August 12, 2022 09:42
nmeum
nmeum commented Aug 12, 2022
View reviewed changes
@MrKevinWeiss MrKevinWeiss mentioned this pull request Aug 12, 2022
Closed
auto-merge was automatically disabled August 12, 2022 12:48

Head branch was pushed to by a user without write access

@nmeum nmeum force-pushed the pr/asymcute-length-check branch 3 times, most recently from 4d616c5 to 06d572c Compare August 12, 2022 13:47
@miri64 miri64 merged commit 25a5269 into RIOT-OS:master Aug 13, 2022
@miri64
Copy link
Member

miri64 commented Aug 13, 2022

This needs a backport to the 2022.07 branch as well.

@nmeum nmeum mentioned this pull request Aug 15, 2022
Merged
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@chrysn chrysn Awaiting requested review from chrysn

Assignees
No one assigned
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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 2022.10
Development

Successfully merging this pull request may close these issues.
4 participants
@nmeum @miri64 @benpicco @maribu