Conversation
The Authenticode PKCS#7 blob (a WIN_CERTIFICATE payload padded to an 8-byte boundary) and the pkix `splice` fuzz fixture both carry trailing bytes after their DER value. Decode them with `decode_with_remainder` so they keep working once `der::decode` rejects trailing data (see librasn#552).
`der::decode` decoded the root value and returned without checking that the whole input was consumed, so e.g. `der::decode::<()>(&[0x05, 0x00, 0xDE, 0xAD, 0xBE, 0xEF])` silently accepted four trailing bytes. X.690 §8.1.1.1 requires a DER message to be exactly one complete value with no trailing data. Return `UnexpectedExtraData` when any bytes remain after the root value. `decode_with_remainder` is unchanged for callers that decode a value from the front of a larger buffer. Reported in librasn#552.
Author
|
The matrix failures are pre-existing: current stable clippy's new |
Collaborator
|
should be fixed by rebasing now :) |
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
Fixes #552.
der::decodedecoded the root value and returned without checking that the whole inputwas consumed, so
der::decode::<()>(&[0x05, 0x00, 0xDE, 0xAD, 0xBE, 0xEF])(aNULLfollowed by four garbage bytes) decoded to
()and silently dropped the trailing bytes.X.690 §8.1.1.1 requires a DER message to be exactly one complete value with no trailing
data. The same leniency is what produced the round-trip divergence @tynus2 flagged, where
encode(decode(bytes))can be shorter thanbytes. I've kept out of thesecurity-classification discussion and scoped this purely to the exact-consumption / API
contract of
decode<T>.What the fix does
src/der.rs: after decoding the root value,der::decodenow checks the decoder'sremaining input and returns
DecodeErrorKind::UnexpectedExtraData { length }when anybytes are left. It reuses the extra-data error rasn already emits for trailing bytes inside
a constructed value (
src/ber/de.rs), so the behaviour and error type stay consistent withthe rest of the codec.
decode_with_remainderis unchanged, for callers that intentionallydecode a value from the front of a larger buffer, and the
decodedoc comment now statesthe strictness.
Scope: DER only, and the codec-wide question
I scoped this to
der::decode, matching the issue. @Nicceboy's point that the same patternapplies to several
decodeentry points is right: each codec has its own thin top-leveldecodewrapper (ber,cer,oer,uper, ...), and the strictness lives in thatwrapper rather than in shared machinery, so extending it is mechanical. I deliberately did
not touch the others here:
streaming heritage), so making it strict is a semantics decision I didn't want to take
unilaterally.
same shape of question.
If you'd like the same check applied uniformly across the strict
decodeentry points(with the
decodedocs updated to state the strictness, per your suggestion), I'm happy todo that as a follow-up — just say which codecs you want it on.
Real-world impact surfaced by the change
Two fixtures already in the tree carried trailing bytes that the lenient decoder silently
accepted, and both are legitimate:
standards/cms/tests/data/pesig.p7(Authenticode): the PKCS#7 blob lives inside aWIN_CERTIFICATE structure padded to an 8-byte boundary, so it has two
00padding bytesafter the DER value.
test_authenticodenow parses it withdecode_with_remainder.standards/pkix/tests/data/splice.bin(a fuzz-regression fixture): a validAlgorithmIdentifierfollowed by three trailing bytes. Thesplicetest's DER arm nowuses
decode_with_remainder, matching its existing BER/CER arms.Both are small, mechanical migrations to the "decode one value, ignore the rest" API, and
are included here so CI stays green. They're also a useful signal for the codec-wide
decision: trailing bytes do turn up in real inputs (container padding), so the strict
default and the
decode_with_remainderescape hatch matter together.Tests
tests/issue552.rs:der_decode_rejects_trailing_bytes— the issue's exactNULL+ 4 trailing bytes nowreturns
UnexpectedExtraData { length: 4 }.der_decode_rejects_trailing_bytes_on_integer—INTEGER+ 1 trailing byte.der_decode_accepts_exact_input— exact encodings still decode.der_decode_round_trip_holds_for_exact_input—encode(decode(bytes)) == bytesforexact input.
I confirmed the two rejection tests fail on the unpatched tree and pass with the fix. The
full workspace suite is green (
cargo test --workspace), as arecargo fmt --all -- --check,cargo clippy --workspace --all-targets --features=f32,f64,bytes,std,backtraces -- -D warnings, and the doc build with-D warnings.Evidence note (independent reference)
As an independent cross-check on the property, not as a dependency: I maintain
der-verified(0.1.0, MIT/Apache-2.0, proofs re-runnable from a fresh clone), a DER/X.690core in which exact whole-input consumption is one of the verified properties. Its
top-level strict decoders return a distinct
TrailingDataerror on any trailing bytes, anda Kani harness proves that whenever the TLV reader accepts an input it consumes exactly
header + declared_lengthbytes and never over-reads. That is bounded model checking (a16-byte symbolic buffer, loop unwinding to depth 16) rather than an all-length statement,
but it covers the trailing-data property on that domain; the crate also carries round-trip
harnesses of the same shape. This PR simply brings
der::decodein line with thatexact-consumption behaviour.