Fix crash: bounds-check Localization binary decode against out-of-bounds reads#2218
Open
ankuper wants to merge 1 commit into
Open
Fix crash: bounds-check Localization binary decode against out-of-bounds reads#2218ankuper wants to merge 1 commit into
ankuper wants to merge 1 commit into
Conversation
Localization.init(from:) parses a length-prefixed binary blob with zero validation: ReadBuffer.read() does a raw memcpy with no bounds check, and `for _ in 0 ..< count` traps on a negative count. This buffer is persisted (and can arrive via a server-pushed localization update), so a truncated or corrupt record crashes on decode — and crash-loops on every relaunch since the bad record stays in Postbox (confirmed: SIGTRAP, build 33193, procLaunch showed an app that had been running over an hour, then an instant relaunch that crashed again 40s later). Add a length-prefixed-read helper that validates offsets against the buffer length before every read and aborts the parse (keeping entries already decoded) on the first out-of-bounds access. Invalid UTF-8 in an otherwise in-bounds string is untouched — it still drops just that one entry and continues, matching the original behavior.
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.
Why
Localization.init(from:)parses a length-prefixed binary blob with no validation:ReadBuffer.read()does a rawmemcpywith no bounds check against the buffer's actual length, andfor _ in 0 ..< counttraps with a negativecount. Every length-prefixed string field in this parser (key, plural forms, single value) is read the same unchecked way.This buffer is persisted (and can arrive via a server-pushed localization update), so a truncated or corrupted record crashes on decode — and crash-loops on every subsequent relaunch, since the bad record stays in Postbox. Confirmed on-device:
SIGTRAP, withprocLaunchshowing an app instance that had been running over an hour before the first crash, followed by an instant relaunch that crashed again ~40s later hitting the same record.Fix
Added a small
readLengthPrefixedData()helper used for every length-prefixed read in the parser. It validates the offset against the buffer length before reading the 4-byte length prefix, and validateslength >= 0and that the resulting slice stays in bounds before reading the payload — returningnil(which aborts the parse, keeping whatever entries were already decoded) on any violation.Behavior for already-valid-but-garbled data is unchanged: a length-prefixed read that is in bounds but decodes to invalid UTF-8 still just drops that one entry and the loop continues, exactly as before — only real out-of-bounds/negative-length reads abort the parse now.
Testing
Verified against the crash report that triggered this (
SIGTRAP, trap point consistent with either the0 ..< countRange precondition or an out-of-boundsData(bytes:count:)read). Built and ran on-device (iPhone, iOS 26.5); localization loading works normally post-fix, no regression in displayed strings.