Fix out-of-bounds write in decode_type1033 (RTCM3 antenna/receiver descriptors) — closes #799 - #801
Open
tiarno wants to merge 1 commit into
Open
Fix out-of-bounds write in decode_type1033 (RTCM3 antenna/receiver descriptors) — closes #799#801tiarno wants to merge 1 commit into
tiarno wants to merge 1 commit into
Conversation
Fixes an out-of-bounds write in decode_type1033 (issue tomojitakasu#799): the five descriptor length fields (n, m, n1, n2, n3) come off the wire unclamped and are used as strncpy lengths against 64-byte sta_t fields. A declared length above 63 writes past the 64-byte destination field; everything from byte 31 onward is zero-padding rather than attacker content, since the 32-byte staging buffers' read loops never capture more than 31 real bytes. At 209 and up (n1, via rectype) the write reaches sta.pos, the base station coordinates, zeroing low-order mantissa bytes and silently corrupting the adopted base position. Clamp each length to the staging buffers' capacity immediately before the strncpy block. Benign streams decode byte-identically; malicious streams (declared lengths 211, 255, and an out-of-bounds variant) land at the true coordinate instead of a corrupted one; UBSan's array-bounds finding is gone.
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.
Fixes the out-of-bounds write reported in #799: the five RTCM3 type 1033 descriptor lengths (
n,m,n1,n2,n3) come straight off the wire and are used unclamped asstrncpylengths against 64-byte fields insta_t. A declared length above 63 writes past the 64-byte destination field — everything from byte 31 onward is zero-padding rather than attacker content, since the 32-byte staging buffers' read loops never capture more than 31 real bytes. At 209 and up (n1, viarectype) the write reachessta.pos— the base station's own coordinates — zeroing low-order mantissa bytes and silently corrupting a receiver's adopted base position while it continues to report a fixed solution.postpos.c's post-processing path guards the equivalent assignment withnorm(sta[i].pos,3)>0.0; the real-time server path (rtksvr.c) does not.The fix clamps each declared length to the staging buffers' actual capacity, immediately before the
strncpyblock:Verified against benign and malicious streams (declared lengths 211, 255, and an out-of-bounds variant targeting
recsno): benign streams decode byte-identically before and after, all three attack variants land at the true coordinate instead of a corrupted one, and UBSan'sarray-boundsfinding (index 255 out of bounds for type 'char [64]', matching the original report) is gone.The same missing clamp exists unchanged on
rtklib_2.4.3(2.4.3 b34) — the branch this defect was actually measured against in detail, and what's shipped in Debian'srtklibpackage and in at least one commercial RTK receiver's firmware. The fix there is the same six lines, offset by one unrelated line present only on that branch; verified separately. Happy to open a second PR against that branch if useful.Scope note:
decode_type1007/1008have the same unclamped-length pattern but the fields they write (antdes/antsno) sit too early insta_tto reachposor leave the struct — memory-safety-only, no positioning impact, and out of scope for this PR.