Repository navigation
const-oid: reject non-minimal arc encodings in BER decoder - #2441
Open
yuxi-liu-wired wants to merge 2 commits into
Open
yuxi-liu-wired wants to merge 2 commits into
yuxi-liu-wired wants to merge 2 commits into
Conversation
X.690 8.19.2 requires each subidentifier to be encoded in the fewest possible octets, so the leading octet of an arc is never 0x80. `Arcs::try_next` (used by `ObjectIdentifier::from_bytes` and so by the `der` decoder) skipped such padding, so `2A 80 86 48` decoded to `1.2.840` alongside the canonical `2A 86 48`. Because `ObjectIdentifier` stores and compares the original bytes, the two decode to OIDs that print the same but compare unequal: a padded `rsaEncryption` displays as `1.2.840.113549.1.1.1` yet is `!=` the `rsaEncryption` constant, and DER re-encoding keeps the padding. pyca/cryptography and OpenSSL reject the padded form. Reject a leading 0x80 octet with `Error::Base128`. Adds a regression test. Found by differential fuzzing of x509-cert against pyca/cryptography's rust-asn1.
dishmaker
reviewed
Oct 2, 2026
Comment on lines
+156
to
+165
|
|
||
| // Padding in front of a single-octet arc: `1.2.0` and `1.2.5`. | ||
| assert_eq!( | ||
| ObjectIdentifier::from_bytes(&[0x2A, 0x80, 0x00]), | ||
| Err(Error::Base128), | ||
| ); | ||
| assert_eq!( | ||
| ObjectIdentifier::from_bytes(&[0x2A, 0x80, 0x80, 0x05]), | ||
| Err(Error::Base128), | ||
| ); |
Contributor
There was a problem hiding this comment.
I think one case is enough. Is it testing/covering some more lines than the 840 test?
Suggested change
| // Padding in front of a single-octet arc: `1.2.0` and `1.2.5`. | |
| assert_eq!( | |
| ObjectIdentifier::from_bytes(&[0x2A, 0x80, 0x00]), | |
| Err(Error::Base128), | |
| ); | |
| assert_eq!( | |
| ObjectIdentifier::from_bytes(&[0x2A, 0x80, 0x80, 0x05]), | |
| Err(Error::Base128), | |
| ); |
dishmaker
reviewed
Oct 2, 2026
Comment on lines
+180
to
+183
| assert_eq!( | ||
| ObjectIdentifier::from_bytes(&[0x2A, 0x00]).unwrap().arc(2), | ||
| Some(0), | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggested change
| assert_eq!( | |
| ObjectIdentifier::from_bytes(&[0x2A, 0x00]).unwrap().arc(2), | |
| Some(0), | |
| ); |
dishmaker
reviewed
Oct 2, 2026
Comment on lines
+167
to
+171
| // Padding on a later arc. | ||
| assert_eq!( | ||
| ObjectIdentifier::from_bytes(&[0x2A, 0x86, 0x48, 0x80, 0x01]), | ||
| Err(Error::Base128), | ||
| ); |
Contributor
There was a problem hiding this comment.
Maybe remove this or merge with the first 840 test.
Contributor
|
Thanks! |
Keep one padded multi-octet arc, one padded single-octet arc and the non-leading 0x80 case.
This branch has not been deployed
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.
X.690 8.19.2 requires each subidentifier to be encoded in the fewest possible octets, so the leading octet of an arc is never
0x80.Arcs::try_next, whichObjectIdentifier::from_bytesand therefore thederdecoder use, accepted that padding:2A 80 86 48decoded to1.2.840, the same as the canonical2A 86 48.ObjectIdentifierstores and compares the original bytes, so the two encodings give OIDs that print the same but compare unequal:DER re-encoding keeps the padding too, so a certificate with a padded OID decodes and re-encodes without error. pyca/cryptography and OpenSSL both reject the padded form.
Fix: return
Error::Base128when the first octet of an arc is0x80, the same error that already covers a truncated arc. Arcs that only contain0x80after their first octet (e.g.81 80 00) are unaffected.from_bytes_reject_non_minimal_arcintests/oid.rscovers padding on the first and on later arcs, on single- and multi-octet arcs, and checks that valid0x80octets still decode. It fails on master.cargo hack test --feature-powersetpasses on stable and on 1.85 (MSRV), thethumbv7em-none-eabipowerset build succeeds, andcargo fmt --checkandcargo +1.90.0 clippy --all-features --testsare clean.Found by differential fuzzing of x509-cert against pyca/cryptography's rust-asn1. This PR was produced by AI agents (Claude), from the fuzzing and the fix through to this description.