const-oid: reject over-u32 arcs in BER decoder instead of truncating - #2412
Open
jaideeppyne wants to merge 2 commits into
Open
const-oid: reject over-u32 arcs in BER decoder instead of truncating#2412jaideeppyne wants to merge 2 commits into
jaideeppyne wants to merge 2 commits into
Conversation
The BER byte decoder (`Arcs::try_next`) accumulated base-128 arc digits with an unchecked `result << 7` shift, guarded only by `arc_bytes > ARC_MAX_BYTES`. Because a `u32` arc can occupy up to five base-128 bytes, that guard never fired for a five-byte arc, so the final shift could push the value past `u32::MAX` and silently truncate it. As a result `ObjectIdentifier::from_bytes` accepted encodings denoting arcs greater than `u32::MAX` and returned a wrong value rather than `ArcTooBig`. For example `2A 90 80 80 80 00` (arc 2^32) decoded to `1.2.0` and `2A 90 80 80 80 05` (arc 2^32+5) decoded to `1.2.5`. Accumulate the digits with `checked_mul`/`checked_add` and return `ArcTooBig` on overflow. The largest in-range arc (`u32::MAX`, `2A 8F FF FF FF 7F`) still decodes correctly. Add a regression test.
tarcieri
reviewed
Aug 28, 2026
Comment on lines
+76
to
+84
| // Accumulate the base 128 digits with overflow checking. | ||
| // | ||
| // An arc whose value does not fit in `Arc` (`u32`) is | ||
| // rejected as `ArcTooBig` rather than being silently | ||
| // truncated by the `<< 7` shift. Note that a `u32` arc can | ||
| // be up to five base 128 bytes long, so checking the number | ||
| // of consumed bytes alone is not sufficient: the final byte | ||
| // of a five-byte arc can still push the value past | ||
| // `u32::MAX`. |
Member
There was a problem hiding this comment.
This comment seems overly verbose and also references the previous bug which is somewhat irrelevant after it's fixed.
Can you whittle it down to something shorter? And no need to mention the truncation bug in a fixed implementation.
It can stay on the comment on the tests, since those are talking about desirable or undesirable behavior.
Author
There was a problem hiding this comment.
Cut down to three lines and the truncation reference is gone:
// A five byte arc can still exceed `Arc`, so the digits are
// accumulated with overflow checking rather than by counting
// bytes.Left the test comments as they are, since those are describing the behaviour being asserted.
Member
|
Nice catch, thanks! |
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.
Arcs::try_next(used byObjectIdentifier::from_bytes) accumulated base-128 arc digits with an uncheckedresult << 7, guarded only byarc_bytes > ARC_MAX_BYTES. Since au32arc can span up to five base-128 bytes, that guard never fires for a five-byte arc, so the final shift can push the value pastu32::MAXand silently truncate it.As a result
from_bytesaccepted encodings denoting arcs greater thanu32::MAXand returned a wrong value instead ofArcTooBig:2A 90 80 80 80 00(arc 2³²) decoded to1.2.02A 90 80 80 80 05(arc 2³²+5) decoded to1.2.5This is the byte-decode counterpart of the base-10 parse overflow fixed for #1585/#1592, which only touched
parser.rs.Fix: accumulate with
checked_mul/checked_addand returnArcTooBigon overflow. The largest in-range arc (u32::MAX,2A 8F FF FF FF 7F) still decodes correctly. Adds a regression test.