fix(advert): bounds-check optional fields before memcpy#2531
Open
swaits wants to merge 1 commit into
Open
Conversation
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
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.
Summary
AdvertDataParser::AdvertDataParser(src/helpers/AdvertDataHelpers.cpp) optimisticallymemcpy'd_lat/_lon/_extra1/_extra2from the supplied buffer before validating that the buffer was long enough. A crafted advert with all optional-field flag bits set andapp_data_len = 1over-read up to 12 bytes past the supplied slice. This change validates the length before eachmemcpy.Background
The buggy pattern (verbatim, before this PR):
The
app_dataargument in the live path is&pkt->payload[i]wherepkt->payloadis the 184-byte packet buffer (src/Packet.h), so OOB reads land inside the same allocation. That's the only thing keeping this from being a remote info-disclosure today. The fix prevents the OOB regardless of where the buffer happens to live, which is the right hygiene for a parser that takes a length parameter.Change
src/helpers/AdvertDataHelpers.cpp— before eachmemcpyof an optional field, check that the requested byte count is available. If not, return early;_validstaysfalse, which is the same outcome callers already get for a too-short payload.Why this is the minimal fix
The pattern matches the existing build-side
AdvertDataBuilder::encodeToreasoning. Rewriting the parser around a separate validator pass would add code without removing the underlying bug.Risk / compatibility
_lat/_lon/ etc. and then being rejected by the trailing length check.References