fix: mask cert serial MSB to prevent DER Overlength panic in --generate#220
Draft
ryancee wants to merge 1 commit intostr4d:mainfrom
Draft
fix: mask cert serial MSB to prevent DER Overlength panic in --generate#220ryancee wants to merge 1 commit intostr4d:mainfrom
ryancee wants to merge 1 commit intostr4d:mainfrom
Conversation
97684df to
6a350c8
Compare
SerialNumber::new in x509-cert / der encodes the value as a DER INTEGER.
When the MSB of the first byte is set the encoder prepends a 0x00
sign-extension byte, producing a 21-byte encoding that exceeds the
RFC 5280 §4.1.2.2 limit of 20 bytes. The crate returns Overlength
and the .expect("valid") call panics.
Because the 20-byte array is random, the MSB is set ~50% of the time,
so --generate panics on roughly every other invocation.
Fix: mask serial[0] &= 0x7f before constructing the certificate to ensure
the value is always positive and the DER encoding never exceeds 20 bytes.
Tested on YubiKey 5C NFC (serial 37153875, firmware 5.7.4).
6a350c8 to
15b8111
Compare
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.
Summary
--generatepanics withOverlengthatbuilder.rs:127on approximately every other invocation.Root cause
builder.rsgenerates a 20-byte random certificate serial and passes it directly toSerialNumber::new:A DER
INTEGERrequires a0x00sign-extension byte when the MSB of the first content byte is set (to indicate a positive value). For a 20-byte random array this happens ~50% of the time, producing a 21-byte encoding that exceeds the RFC 5280 §4.1.2.2 limit of 20 bytes.x509-cert 0.2 / der 0.7returnsError { kind: Overlength, position: None }and the.expect("valid")call panics with:Fix
Mask the high bit of the first byte before constructing
SerialNumberto ensure the value is always non-negative and the DER encoding never exceeds 20 bytes:This is the standard approach used by most X.509 certificate generators (see e.g. OpenSSL, rcgen, rustls-cert-gen). The existing TODO comment notes that
SerialNumber::generate(RustCrypto/formats#1270) will eventually handle this correctly; until that lands, the manual mask is the correct fix.Testing
Verified on YubiKey 5C NFC (serial 37153875, firmware 5.7.4) with AES256 management key. Previously
--generatefailed consistently on this device (panicking atbuilder.rs:127). With this fix applied the command completes successfully and produces a valid recipient/identity stub.Also related to / builds on top of #219 (wrong OID in
util.rs).Note on branch base
This branch is based on the PR #190 branch (
str4d:yubikey-0.8) which has not yet been merged. The fix applies equally tomainonce #190 merges —serial[0] &= 0x7fis a one-line change independent of the rest of the #190 changes.