fix(identity): reject trailing-byte and non-canonical DER in identity decode paths - #2102
Open
AkramBitar wants to merge 1 commit into
Open
fix(identity): reject trailing-byte and non-canonical DER in identity decode paths#2102AkramBitar wants to merge 1 commit into
AkramBitar wants to merge 1 commit into
Conversation
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
AkramBitar
force-pushed
the
fix-2071-identity-canonical-der
branch
2 times, most recently
from
August 11, 2026 10:41
456a9ec to
73b770a
Compare
AkramBitar
force-pushed
the
fix-2071-identity-canonical-der
branch
from
August 13, 2026 10:21
73b770a to
9ae3544
Compare
Identity.UniqueID() hashes the raw identity bytes rather than a canonicalised
form of the decoded value, and it is the cache key throughout the identity and
wallet layers (role/registry.go's fast path, provider.go's signer cache). Any
two byte strings that decode to the same logical identity but hash differently
give that one identity two cache slots: a token paid to the second spelling
still verifies, because verification works on the decoded value, but never
resolves to its owner's wallet, because the lookup works on UniqueID().
Every identity decode path allowed exactly that. The four encoding/asn1 sites
(MultiIdentity.Deserialize, PolicyIdentity.Deserialize, MultiSignature.FromBytes,
PolicySignature.FromBytes) discarded asn1.Unmarshal's "rest" return, and
DecodeIdentity read the outer SEQUENCE length only to throw it away:
canonical : 300e 300c 0405 616c696365 0403 626f62
variant : 3011 300c 0405 616c696365 0403 626f62 02012a
Both decode to MultiIdentity{alice, bob}; both were accepted; their UniqueID()s
differ.
Four spellings of one identity were reachable:
- garbage appended after the value
- garbage smuggled inside the SEQUENCE with the outer length grown to cover
it. This is the one a "rest" check cannot see: rest reports only bytes after
the top-level TLV, and encoding/asn1 silently discards SEQUENCE elements the
destination struct has no field for, so the outer TLV consumes the whole
input and the parse looks clean
- T61String/IA5String/GeneralString where UTF8String was declared, which
encoding/asn1 accepts
- non-minimal lengths (0x81 0x06) and non-minimal INTEGER contents
(02 02 00 05)
marshal.UnmarshalStrict, which the four asn1 sites now route through, checks
rest and then re-encodes the decoded value and requires it to reproduce the
input byte-for-byte (ErrNonCanonical). One check covers the first three and any
further encoding/asn1 leniency, for one extra marshal of a small struct per
decode. DecodeIdentity — the TypedIdentity envelope and the hot path, which
hand-walks the TLVs and does not re-marshal — pins the outer SEQUENCE length to
len(b), requires the OCTET STRING to end exactly at len(b), and rejects
non-minimal lengths (ErrNonMinimalLen) and non-minimal integers
(ErrNonMinimalInt).
Nothing previously written can be rejected. The envelope is single-sourced: all
seven construction sites funnel through TypedIdentity.Bytes() ->
marshal.EncodeIdentity, and the four inner envelopes only through their own
asn1.Marshal. Every one of those encoders is canonical — appendTLV emits minimal
lengths, encodeInt32 strips exactly the bytes parseInt32 now rejects, and
asn1.Marshal is canonical by construction, including the legacy string-typed
spellings written by older versions of this SDK. There is no non-Go producer of
these bytes, so no identity on a ledger or in storage becomes undecodable and
old and new nodes cannot disagree during a rolling upgrade. Key material is
untouched: x509 certs and idemix credentials live inside the OCTET STRING and
never reach these decoders.
Scope is the DER envelopes. Signature parsing (x509/crypto/ecdsa.go,
idemixnym/nym/signer.go) stays lenient for external signers and HSMs; the
MultiSignature/PolicySignature envelopes are strict because we always produce
them, the signatures they carry are not. Two things remain malleable and are now
documented rather than implied away: the protobuf payload inside the OCTET
STRING, which is not DER at all, and the legacy type fold, where INTEGER 2,
UTF8String "x509" and PrintableString "x509" still give one x509 identity three
UniqueID()s. This change narrows that set from unbounded to exactly three;
closing it to one needs a rule at the validator boundary, since the older
spellings may exist in persisted data, and is tracked separately.
UnmarshalStrict's round-trip means "b is what asn1.Marshal would emit", which is
narrower than "b is valid DER": a field tagged optional or omitempty, or a
time.Time, would false-reject legal encodings. None of the four types has one,
and TestUnmarshalStrict_FourCallSitesHaveNoOptionalFields reflects over them so
a future field trips there rather than in production.
Tests cover every vector at each affected site, asserting the bypass really did
decode to the same value with an empty rest and a different UniqueID() rather
than only that it now errors, alongside guards that our own encoders' output is
never rejected. The five fuzz targets also assert canonicality; at the four asn1
sites that is a contract-level regression guard rather than an independent check
(UnmarshalStrict enforces it the way Bytes() computes it), and it is a real
check only in FuzzDecodeIdentityNoPanic, where DecodeIdentity and EncodeIdentity
are separate implementations. boolpolicy's two targets are new and wired into
the nightly-fuzz matrix. Two TestDecodeErrors fixtures had an outer length
disagreeing with their own buffer, so the new check fired before the inner
failure they were named for; their lengths are corrected rather than their
expectations.
Signed-off-by: AkramBitar <akram@il.ibm.com>
AkramBitar
force-pushed
the
fix-2071-identity-canonical-der
branch
from
August 13, 2026 13:34
9ae3544 to
85f2db8
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.
Identity.UniqueID()hashes the raw identity bytes, not the decoded value, and it is the cache key across the identity and wallet layers. So two byte strings that decode to the same identity but differ as bytes give that one identity two cache slots. Every identity decode path allowed exactly that.The problem
Alice and Bob's 2-of-2 multisig identity:
Both decode to
MultiIdentity{alice, bob}; both were accepted; theirUniqueID()s differ.Transfer a token to
variantand it verifies — verification works on the decoded identities — but never resolves to Alice's or Bob's wallet, because the lookup works onUniqueID(). A valid, signed, spendable token neither owner can see.Four spellings of one identity did this:
rest != 0T61String/IA5String/GeneralStringwhereUTF8Stringwas declared0x81 0x06) and integers (02 02 00 05)readLen/parseInt32The second is the subtle one:
restonly reports bytes after the top-level TLV, andencoding/asn1silently discards SEQUENCE elements the destination struct has no field for. The outer TLV consumed the whole input, sorestis empty and a check onrestalone sees a clean parse.The fix
marshal.UnmarshalStrict(MultiIdentity,PolicyIdentity,MultiSignature,PolicySignature) checksrest, then re-encodes the decoded value and requires it to reproduce the input byte-for-byte (ErrNonCanonical). One check for the first three rows, and for any furtherencoding/asn1leniency. Cost: one extra marshal of a small struct per decode.marshal.DecodeIdentity(theTypedIdentityenvelope, and the hot path — hand-walks TLVs, no re-marshal) pins the outer SEQUENCE length tolen(b), requires the OCTET STRING to end exactly atlen(b), and rejects non-minimal lengths (ErrNonMinimalLen) and non-minimal INTEGER contents (ErrNonMinimalInt).No existing identity can be rejected
The envelope is single-sourced: all seven construction sites funnel through
TypedIdentity.Bytes()→marshal.EncodeIdentity, and the four inner envelopes only through their ownasn1.Marshal. Every one of those encoders is canonical —appendTLVemits minimal lengths,encodeInt32strips exactly the bytesparseInt32now rejects, andasn1.Marshalis canonical by construction, including the legacy string-typed spellings written by older versions of this SDK. There is no non-Go producer of these bytes.So nothing previously written — on a ledger or in storage — becomes undecodable, and old and new nodes cannot disagree during a rolling upgrade. Pinned by
TestUnmarshalStrict_AcceptsWhatStdlibMarshalEmits,TestDecodeIdentity_AcceptsEveryLengthFormOurEncoderEmits,TestDecodeIdentity_AcceptsEveryIntegerOurEncoderEmitsandTestDecodeIdentity_StdlibEncodingsRemainDecodable. If you know of a producer outside this repo, say so — the two non-minimal checks are the only ones that could reject a legitimately-encoded foreign identity, and can be dropped or gated separately.Key material is unaffected: x509 certs and idemix credentials live inside the
OCTET STRINGand never reach these decoders.Scope
Identity envelopes only, per the issue. Deliberately not covered, and now stated in
docs/services/identity.md:x509/crypto/ecdsa.go,idemixnym/nym/signer.go) stays lenient — those bytes come from external signers and HSMs. TheMultiSignature/PolicySignatureenvelopes are strict since we always produce them; the signatures they carry are not.UniqueID()split, and it is the same class of problem.INTEGER 2,UTF8String "x509"andPrintableString "x509"all decode to one x509 identity under three differentUniqueID()s, so a token paid to the second or third spelling of a victim's identity verifies but does not reach their wallet. This PR narrows that set from unbounded to exactly three; closing it to one cannot be done in the decoder, because older versions of this SDK wrote those spellings and they may exist in persisted data. It needs a rule at the validator boundary — require theINTEGERspelling for new transactions, keep decoding the others for reads — tracked separately.OCTET STRINGis protobuf for x509/idemix identities, not DER, and remains malleable.UnmarshalStrict's round-trip means "bis whatasn1.Marshalwould emit", which is narrower than "valid DER": a field taggedoptional/omitempty, or atime.Time, would false-reject legal encodings. None of the four types has one, andTestUnmarshalStrict_FourCallSitesHaveNoOptionalFieldsreflects over them so a future field trips there rather than in production.Tests
Every vector is tested at each affected site, asserting the bypass really did decode to the same value with an empty
restand a differentUniqueID()— not just that it now errors. The five fuzz targets also assert canonicality; note this is a contract-level regression guard at the fourasn1sites (UnmarshalStrictenforces it the same wayBytes()computes it), and a genuinely independent check only inFuzzDecodeIdentityNoPanic, whereDecodeIdentityandEncodeIdentityare separate implementations. boolpolicy's two targets are new and wired into the nightly-fuzz matrix.Two
TestDecodeErrorsfixtures had an outer length disagreeing with their own buffer, so the new check fired before the inner failure they were named for; their lengths are corrected rather than their expectations.Squashed to a single commit, rebased onto current
main.Fixes #2071