fix: reject 5-byte LEB128 encodings that overflow uint32 in decodeUInt32 - #1690
Merged
gjtorikian merged 2 commits intoSep 17, 2026
Merged
Conversation
decodeUInt32 guards the maximum byte count, but not the range of the final byte. On the 5th byte only 4 of its 7 data bits fit in a uint32, so a byte above 0x0F encodes a value greater than MAX_UINT32. The 32-bit `<<` then silently drops the overflowing bits, so e.g. [0x80,0x80,0x80,0x80,0x10] (2^32) decodes to 0 and [0xFF,0xFF,0xFF,0xFF,0x7F] decodes to 4294967295 instead of being rejected. encodeUInt32 already validates the uint32 range, and the suite's existing 'exceeds uint32 range' test only covered the too-many-bytes case. Reject an over-range final byte before the shift so decode fails loudly on such input, matching the encoder. Valid encodings (final byte <= 0x0F, including MAX_UINT32) are unaffected.
Contributor
 |
eeshsaxena
changed the title
…ix path - Compute FINAL_BYTE_MAX_DATA from FINAL_BYTE_SHIFT instead of hardcoding 0x0F, so the two constants cannot drift if MAX_BYTES_FOR_UINT32 changes. - Rename the 6-byte test to "throws for encoding longer than 5 bytes" so it no longer shares wording with the new "exceeds uint32 range" error. - Add a vault.decrypt test that a payload whose key-length prefix is an over-range 5-byte LEB128 sequence rejects, exercising the guard through its only real caller.
gjtorikian
approved these changes
Sep 17, 2026
Contributor
|
thank you! |
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
2 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.
Problem
decodeUInt32insrc/common/utils/leb128.tsvalidates the byte count of a LEB128 sequence but not the range of its last byte.A uint32 needs at most 5 bytes. The 5th byte is read at
shift = 28, and only32 - 28 = 4of its 7 data bits fit in a uint32. When the 5th byte carries data above0x0F, the value is greater thanMAX_UINT32, but the JavaScript 32-bit<<silently drops the overflowing bits instead of erroring:So over-range 5-byte encodings decode to a wrong value rather than being rejected:
[0x80,0x80,0x80,0x80,0x10](2³²)0[0xFF,0xFF,0xFF,0xFF,0x1F]4294967295[0xFF,0xFF,0xFF,0xFF,0x7F]4294967295encodeUInt32already validates the uint32 range, and the suite's existingthrows for encoding that exceeds uint32 rangetest only covers the too-many-bytes case ([0x80,0x80,0x80,0x80,0x80,0x01]), so this class of over-range input slipped through — a decoder that's meant to fail on out-of-range input silently returns garbage instead.Fix
Reject an over-range final byte before the shift, so the decoder fails loudly to match the encoder:
FINAL_BYTE_SHIFTis(MAX_BYTES_FOR_UINT32 - 1) * DATA_BITS_PER_BYTE = 28andFINAL_BYTE_MAX_DATA = 0x0F. Every valid encoding (final byte ≤0x0F, includingMAX_UINT32→[0xFF,0xFF,0xFF,0xFF,0x0F]) is unaffected; only genuinely out-of-range input now throws.Tests
Added three cases to the
invalid inputsblock: two over-range 5-byte sequences that must throwLEB128 sequence exceeds uint32 range, and one assertingMAX_UINT32(final byte exactly0x0F) still decodes to4294967295. Fullleb128.spec.tspasses (42/42); reverting only the source change fails the two new "throws" cases, confirming they catch the bug.