fix(identity): reject trailing-byte and non-canonical DER in identity decode paths

.github/workflows/nightly-fuzz.yml

@@ -57,6 +57,12 @@ jobs:
           - name: identity-multisig-signature-from-bytes
             pkg: ./token/services/identity/multisig
             func: FuzzMultiSignatureFromBytesNoPanic
+          - name: identity-boolpolicy-identity-deserializer
+            pkg: ./token/services/identity/boolpolicy
+            func: FuzzPolicyIdentityDeserializeNoPanic
+          - name: identity-boolpolicy-signature-from-bytes
+            pkg: ./token/services/identity/boolpolicy
+            func: FuzzPolicySignatureFromBytesNoPanic
           - name: identity-idemix-audit-info-deserializer
             pkg: ./token/services/identity/idemix/crypto
             func: FuzzDeserializeAuditInfoNoPanic

docs/services/identity.md

@@ -269,6 +269,30 @@ It wraps the raw identity bytes with a type label, enabling the system to verify
     - `Type` (string): The identifier of the identity scheme (e.g., `"x509"`, `"idemix"`).
     - `Identity` (bytes): The raw payload of the identity, specific to the key manager.
 
+#### Canonical encoding requirement
+
+The **DER envelopes** of an identity must be the canonical encoding of the value they decode to — exactly one byte string per logical envelope:
+
+*   `marshal.DecodeIdentity` (the `TypedIdentity` envelope decoder in `token/services/identity/marshal`) pins the outer `SEQUENCE`'s declared length to the end of the buffer, requires the read position to land exactly on the last byte after the final field (`ErrTrailingBytes`), rejects non-minimal DER length encodings — a length that fits the short form written in the long form, or a long form with leading zero bytes (`ErrNonMinimalLen`) — and rejects non-minimal `INTEGER` contents, i.e. a redundant leading `0x00`/`0xFF` in the type field (`ErrNonMinimalInt`).
+*   Envelopes decoded with `encoding/asn1` (`MultiIdentity`, `PolicyIdentity`, `MultiSignature`, `PolicySignature`) go through `marshal.UnmarshalStrict`, which rejects any bytes left over after the top-level value **and** re-encodes the decoded value to require it reproduces the input byte-for-byte (`ErrNonCanonical`). The second check is the load-bearing one: `asn1.Unmarshal`'s `rest` return only reports bytes *after* the top-level TLV, while `encoding/asn1` silently discards `SEQUENCE` elements the destination struct has no field for and accepts `T61String`/`IA5String`/`GeneralString` where a `UTF8String` was declared — neither of which leaves anything in `rest`.
+
+The reason is `Identity.UniqueID()`: it 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 cache, `provider.go`'s signer cache, and so on). 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()`. Both producers of these bytes — `appendTLV` in the `marshal` package and `encoding/asn1.Marshal` for legacy encodings — already emit minimal lengths, minimal integers and no undeclared elements, so the stricter decode rejects nothing this tree ever writes.
+
+**What this does not cover.** The guarantee is about the envelopes, not about everything reachable through them:
+
+*   **The legacy type spellings remain a `UniqueID()` split, and it is the same class of problem as the one above.** `DecodeIdentity` folds `INTEGER 2`, `UTF8String "x509"` and `PrintableString "x509"` onto the same type for compatibility with identities written by older versions of this SDK. For one x509 identity that is three accepted byte strings and therefore three `UniqueID()`s:
+
+    ```
+    3006 020102       040150   -> {Type: 2, "P"}   uid NQ5fFHOcay5c
+    3009 0c0478353039 040150   -> {Type: 2, "P"}   uid FK3RQ1kfFhZG
+    3009 1304783530 39 040150  -> {Type: 2, "P"}   uid f4VvsY1uwzTB
+    ```
+
+    A token paid to the second or third spelling of a victim's identity verifies — validation decodes type 2 and checks the payload's cert — but does not resolve to that owner's wallet. The checks above reduce this set from unbounded to exactly three; closing it to one cannot be done in the decoder, because the older spellings may exist in persisted data. It needs a rule at the validator boundary: require the `INTEGER` spelling for identities in *new* transactions while still decoding the others for reads. Out of scope here, tracked separately.
+*   The payload inside the `TypedIdentity` `OCTET STRING` is **protobuf** for x509 and idemix identities (`x509/crypto/config.go`, `idemix/crypto/deserializer.go`), not DER. Protobuf permits field reordering and redundant varints, so those payload bytes remain malleable and the checks above say nothing about them.
+
+This applies to identity decoding only. Signature parsing (`x509/crypto/ecdsa.go`, `idemixnym/nym/signer.go`) stays deliberately lenient: those bytes come from external signers and HSMs whose DER encoders are routinely non-minimal in ways that are still valid for signature purposes. The `MultiSignature` / `PolicySignature` *envelopes* are strict, because we always produce them ourselves; the individual signatures they carry are not.
+
 ### Default Key Managers
 
 The identity service includes two primary implementations for concrete identities:
@@ -528,8 +552,10 @@ type MyNewIdentity struct {
 
 func (m *MyNewIdentity) Serialize() ([]byte, error) { return asn1.Marshal(*m) }
 func (m *MyNewIdentity) Deserialize(raw []byte) error {
-    _, err := asn1.Unmarshal(raw, m)
-    return err
+    // Never `_, err := asn1.Unmarshal(raw, m)`: discarding the "rest" return
+    // accepts trailing garbage, which breaks the canonical encoding
+    // requirement described under TypedIdentity above.
+    return marshal.UnmarshalStrict(raw, m)
 }
 ```
 

token/services/identity/boolpolicy/boolpolicy_fuzz_test.go

@@ -0,0 +1,108 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package boolpolicy
+
+import (
+	"encoding/asn1"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+const maxFuzzBoolPolicyBytes = 64 << 10
+
+// FuzzPolicyIdentityDeserializeNoPanic hunts for malformed ASN.1 that panics
+// PolicyIdentity.Deserialize instead of returning an error. This is the
+// deserialization entry point for policy identities across DeserializeVerifier,
+// GetAuditInfoMatcher, Recipients and Unwrap, and it mirrors
+// FuzzMultiIdentityDeserializeNoPanic in the structurally identical multisig
+// package.
+func FuzzPolicyIdentityDeserializeNoPanic(f *testing.F) {
+	valid, err := (&PolicyIdentity{
+		Policy:     "$0 OR ($1 AND $2)",
+		Identities: [][]byte{[]byte("alice"), []byte("bob"), []byte("carol")},
+	}).Bytes()
+	require.NoError(f, err)
+	f.Add(valid)
+	// Trailing bytes after the canonical encoding — the laxness this decode
+	// path was hardened against; it must now be a clean error, never a panic.
+	f.Add(append(append([]byte{}, valid...), 0xDE, 0xAD))
+	// An element smuggled *inside* the SEQUENCE rather than appended after it.
+	// A rest check cannot see this one, so it is the shape most likely to hide
+	// a regression.
+	smuggled, err := asn1.Marshal(policyIdentityWithExtra{
+		Policy:     "$0 OR ($1 AND $2)",
+		Identities: [][]byte{[]byte("alice"), []byte("bob"), []byte("carol")},
+		Extra:      42,
+	})
+	require.NoError(f, err)
+	f.Add(smuggled)
+	f.Add([]byte{})
+	f.Add([]byte("not asn1"))
+	f.Add([]byte{0x30, 0x81})
+
+	f.Fuzz(func(t *testing.T, raw []byte) {
+		if len(raw) > maxFuzzBoolPolicyBytes {
+			t.Skip()
+		}
+		pi := &PolicyIdentity{}
+		var err error
+		require.NotPanics(t, func() {
+			err = pi.Deserialize(raw)
+		})
+		if err != nil {
+			return
+		}
+		// Canonicality contract: raw was accepted, so it must be the one
+		// encoding of what it decoded to — any other accepted spelling is a
+		// second Identity.UniqueID() cache slot for one identity. This is a
+		// regression guard stated in terms of the contract, not an independent
+		// check: UnmarshalStrict enforces it by re-marshalling and comparing,
+		// which is what Bytes() does, so it cannot fail against the current
+		// implementation. security_test.go carries the actual vectors.
+		reencoded, err := pi.Bytes()
+		require.NoError(t, err, "an accepted PolicyIdentity must be re-serializable")
+		require.Equal(t, raw, reencoded,
+			"accepted bytes must be the canonical encoding of the decoded identity")
+	})
+}
+
+// FuzzPolicySignatureFromBytesNoPanic hunts for malformed ASN.1 that panics
+// PolicySignature.FromBytes instead of returning an error. This is the
+// deserialization entry point invoked directly on peer-supplied signature bytes
+// in PolicyVerifier.Verify.
+func FuzzPolicySignatureFromBytesNoPanic(f *testing.F) {
+	sigs := [][]byte{[]byte("sig1"), nil, []byte("sig3")}
+	valid, err := (&PolicySignature{Signatures: sigs}).Bytes()
+	require.NoError(f, err)
+	f.Add(valid)
+	f.Add(append(append([]byte{}, valid...), 0x00))
+	smuggled, err := asn1.Marshal(policySignatureWithExtra{Signatures: sigs, Extra: 7})
+	require.NoError(f, err)
+	f.Add(smuggled)
+	f.Add([]byte{})
+	f.Add([]byte("not asn1"))
+
+	f.Fuzz(func(t *testing.T, raw []byte) {
+		if len(raw) > maxFuzzBoolPolicyBytes {
+			t.Skip()
+		}
+		sig := &PolicySignature{}
+		var err error
+		require.NotPanics(t, func() {
+			err = sig.FromBytes(raw)
+		})
+		if err != nil {
+			return
+		}
+		reencoded, err := sig.Bytes()
+		require.NoError(t, err, "an accepted PolicySignature must be re-serializable")
+		// Contract-level regression guard; see FuzzPolicyIdentityDeserializeNoPanic.
+		require.Equal(t, raw, reencoded,
+			"accepted bytes must be the canonical encoding of the decoded signature")
+	})
+}

token/services/identity/boolpolicy/identity.go

@@ -39,6 +39,7 @@ import (
 	"github.com/LFDT-Panurus/panurus/token/core/common/encoding/json"
 	tdriver "github.com/LFDT-Panurus/panurus/token/driver"
 	"github.com/LFDT-Panurus/panurus/token/services/identity"
+	"github.com/LFDT-Panurus/panurus/token/services/identity/marshal"
 	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 )
 
@@ -74,11 +75,13 @@ func (p *PolicyIdentity) Serialize() ([]byte, error) {
 	return asn1.Marshal(*p)
 }
 
-// Deserialize decodes raw DER bytes into the receiver.
+// Deserialize decodes raw DER bytes into the receiver. It rejects trailing
+// bytes after the encoded value: raw comes off the wire, and accepting a
+// non-canonical re-encoding of an identity would make two distinct byte
+// strings decode to the same PolicyIdentity while hashing to different
+// Identity.UniqueID()s. See marshal.UnmarshalStrict.
 func (p *PolicyIdentity) Deserialize(raw []byte) error {
-	_, err := asn1.Unmarshal(raw, p)
-
-	return err
+	return marshal.UnmarshalStrict(raw, p)
 }
 
 // Bytes is an alias for Serialize, provided for symmetry with MultiIdentity.

token/services/identity/boolpolicy/security_test.go

@@ -8,6 +8,7 @@ package boolpolicy
 
 import (
 	"context"
+	"encoding/asn1"
 	"testing"
 
 	"github.com/LFDT-Panurus/panurus/token"
@@ -99,3 +100,164 @@ func TestDuplicateIdentityRejectedAtDeserializeTime(t *testing.T) {
 	_, err = d.DeserializeVerifier(ctx, Policy, raw)
 	require.Error(t, err, "a policy identity with a duplicated component identity must be rejected at deserialize time")
 }
+
+// TestTrailingBytesRejectedOnIdentityDeserialize is the reproduction from the
+// issue: PolicyIdentity.Deserialize discarded encoding/asn1.Unmarshal's "rest"
+// return, so a canonical PolicyIdentity with garbage appended still
+// deserialized successfully — while hashing to a *different*
+// Identity.UniqueID(), because UniqueID() hashes the raw bytes rather than a
+// canonicalised form of the decoded value. Since UniqueID() is the cache key
+// throughout the identity/wallet layers (role/registry.go's fast path,
+// provider.go's signer cache, …), one logical identity could occupy two cache
+// slots.
+func TestTrailingBytesRejectedOnIdentityDeserialize(t *testing.T) {
+	canonical, err := (&PolicyIdentity{Policy: "$0 OR $1", Identities: [][]byte{id0, id1}}).Bytes()
+	require.NoError(t, err)
+
+	require.NoError(t, (&PolicyIdentity{}).Deserialize(canonical),
+		"the canonical encoding must still deserialize")
+
+	padded := append(append([]byte{}, canonical...), 0xDE, 0xAD, 0xBE, 0xEF)
+
+	require.Error(t, (&PolicyIdentity{}).Deserialize(padded),
+		"a PolicyIdentity with trailing bytes appended must be rejected")
+
+	// This inequality is why the lenient parse mattered: the two byte strings
+	// decoded to the same logical identity but keyed the caches differently.
+	require.NotEqual(t, token.Identity(canonical).UniqueID(), token.Identity(padded).UniqueID(),
+		"UniqueID() hashes the raw bytes, so the padded form was a distinct cache key")
+}
+
+// TestTrailingBytesRejectedOnSignatureFromBytes covers the same laxness in
+// PolicySignature.FromBytes, which is invoked directly on peer-supplied bytes
+// in PolicyVerifier.Verify. Only the policy *envelope* is tightened here; the
+// individual signatures it carries are still parsed by their own verifiers,
+// which must stay lenient for external signers and HSMs.
+func TestTrailingBytesRejectedOnSignatureFromBytes(t *testing.T) {
+	canonical, err := (&PolicySignature{Signatures: [][]byte{
+		[]byte("sig1"), []byte("sig2"),
+	}}).Bytes()
+	require.NoError(t, err)
+
+	require.NoError(t, (&PolicySignature{}).FromBytes(canonical),
+		"the canonical encoding must still deserialize")
+
+	padded := append(append([]byte{}, canonical...), 0x00)
+
+	require.Error(t, (&PolicySignature{}).FromBytes(padded),
+		"a PolicySignature with trailing bytes appended must be rejected")
+}
+
+// policyIdentityWithExtra is PolicyIdentity plus one undeclared trailing
+// element. Marshalling it is how an attacker writes the smuggled form:
+// encoding/asn1 consumes only as many SEQUENCE elements as the destination
+// struct has fields and silently drops the rest, so these bytes decode into a
+// plain PolicyIdentity with nothing left over.
+type policyIdentityWithExtra struct {
+	Policy     string `asn1:"utf8"`
+	Identities [][]byte
+	Extra      int32
+}
+
+// TestExtraElementInsideSequenceRejectedOnIdentityDeserialize covers the vector
+// the trailing-byte check alone cannot see. Appending garbage *after* the
+// top-level TLV is caught by asn1.Unmarshal's "rest"; moving that same garbage
+// *inside* the SEQUENCE, with the outer length grown to cover it, leaves rest
+// empty — so the identity decodes to the same policy over the same components
+// under raw bytes that hash to a different UniqueID(). Concretely: a token
+// transferred to the smuggled form verifies (the verifier works on the decoded
+// policy) but never resolves to a component owner's wallet (the lookup works on
+// UniqueID()), leaving a valid token none of the owners can see.
+func TestExtraElementInsideSequenceRejectedOnIdentityDeserialize(t *testing.T) {
+	const policy = "$0 OR $1"
+	ids := [][]byte{id0, id1}
+
+	canonical, err := (&PolicyIdentity{Policy: policy, Identities: ids}).Bytes()
+	require.NoError(t, err)
+	require.NoError(t, (&PolicyIdentity{}).Deserialize(canonical),
+		"the canonical encoding must still deserialize")
+
+	smuggled, err := asn1.Marshal(policyIdentityWithExtra{Policy: policy, Identities: ids, Extra: 42})
+	require.NoError(t, err)
+	require.NotEqual(t, canonical, smuggled, "the two encodings must really differ")
+
+	// Baseline: the lax decode accepts it, yields the same policy identity, and
+	// has nothing left over for a rest check to catch.
+	var lenient PolicyIdentity
+	rest, err := asn1.Unmarshal(smuggled, &lenient)
+	require.NoError(t, err)
+	require.Empty(t, rest, "the extra element is inside the SEQUENCE, so there is no tail to reject")
+	require.Equal(t, policy, lenient.Policy)
+	require.Equal(t, ids, lenient.Identities, "same logical identity, different bytes")
+
+	require.NotEqual(t, token.Identity(canonical).UniqueID(), token.Identity(smuggled).UniqueID(),
+		"UniqueID() hashes the raw bytes, so the smuggled form is a distinct cache key")
+
+	require.Error(t, (&PolicyIdentity{}).Deserialize(smuggled),
+		"a PolicyIdentity with an element smuggled inside the SEQUENCE must be rejected")
+}
+
+// TestAlternateStringTagRejectedOnIdentityDeserialize covers a third spelling,
+// unique to PolicyIdentity because it is the only one of the four envelopes
+// carrying a string field: encoding/asn1 accepts T61String, IA5String and
+// GeneralString wherever `asn1:"utf8"` was declared. Flipping that one tag byte
+// leaves the decoded Policy identical and the raw bytes — hence the UniqueID()
+// — different, with no trailing bytes and no length anomaly for either earlier
+// check to catch.
+func TestAlternateStringTagRejectedOnIdentityDeserialize(t *testing.T) {
+	canonical, err := (&PolicyIdentity{Policy: "$0 OR $1", Identities: [][]byte{id0, id1}}).Bytes()
+	require.NoError(t, err)
+
+	// The Policy field is the first element of the SEQUENCE, so its tag is the
+	// first byte of the body: [SEQUENCE, len, tag, ...].
+	require.Equal(t, byte(asn1.TagUTF8String), canonical[2],
+		"fixture assumption: canonical[2] is the Policy field's UTF8String tag")
+
+	for _, tag := range []byte{
+		byte(asn1.TagT61String),
+		byte(asn1.TagIA5String),
+		byte(asn1.TagGeneralString),
+	} {
+		retagged := append([]byte{}, canonical...)
+		retagged[2] = tag
+
+		// Baseline: same policy string, no leftovers.
+		var lenient PolicyIdentity
+		rest, err := asn1.Unmarshal(retagged, &lenient)
+		require.NoError(t, err, "tag 0x%02X: encoding/asn1 accepts this where UTF8String was declared", tag)
+		require.Empty(t, rest)
+		require.Equal(t, "$0 OR $1", lenient.Policy, "tag 0x%02X: same policy, different bytes", tag)
+
+		require.NotEqual(t, token.Identity(canonical).UniqueID(), token.Identity(retagged).UniqueID(),
+			"tag 0x%02X: the retagged form is a distinct cache key", tag)
+
+		require.Error(t, (&PolicyIdentity{}).Deserialize(retagged),
+			"tag 0x%02X: a non-UTF8String spelling of the same policy must be rejected", tag)
+	}
+}
+
+// policySignatureWithExtra is PolicySignature plus one undeclared element, the
+// signature-envelope analog of policyIdentityWithExtra.
+type policySignatureWithExtra struct {
+	Signatures [][]byte
+	Extra      int32
+}
+
+// TestExtraElementInsideSequenceRejectedOnSignatureFromBytes covers the same
+// vector in PolicySignature.FromBytes, which PolicyVerifier.Verify calls
+// directly on peer-supplied bytes. The nil slot mirrors an unsigned OR branch,
+// which must keep decoding.
+func TestExtraElementInsideSequenceRejectedOnSignatureFromBytes(t *testing.T) {
+	sigs := [][]byte{[]byte("sig1"), nil, []byte("sig3")}
+
+	canonical, err := (&PolicySignature{Signatures: sigs}).Bytes()
+	require.NoError(t, err)
+	require.NoError(t, (&PolicySignature{}).FromBytes(canonical),
+		"the canonical encoding, unsigned slot included, must still deserialize")
+
+	smuggled, err := asn1.Marshal(policySignatureWithExtra{Signatures: sigs, Extra: 7})
+	require.NoError(t, err)
+
+	require.Error(t, (&PolicySignature{}).FromBytes(smuggled),
+		"a PolicySignature with an element smuggled inside the SEQUENCE must be rejected")
+}

token/services/identity/boolpolicy/sig.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/LFDT-Panurus/panurus/token"
 	"github.com/LFDT-Panurus/panurus/token/driver"
+	"github.com/LFDT-Panurus/panurus/token/services/identity/marshal"
 	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 )
 
@@ -29,11 +30,15 @@ func (s *PolicySignature) Bytes() ([]byte, error) {
 	return asn1.Marshal(*s)
 }
 
-// FromBytes deserialises raw ASN.1 DER into the receiver.
+// FromBytes deserialises raw ASN.1 DER into the receiver. It rejects trailing
+// bytes after the encoded value: raw is peer-supplied, and the envelope itself
+// is always produced by our own canonical asn1.Marshal, so leftover bytes only
+// ever mean a mangled or deliberately padded signature. (This tightens the
+// PolicySignature *envelope* only — the individual signatures it carries are
+// parsed by their own verifiers, which must stay lenient for external
+// signers/HSMs.)
 func (s *PolicySignature) FromBytes(raw []byte) error {
-	_, err := asn1.Unmarshal(raw, s)
-
-	return err
+	return marshal.UnmarshalStrict(raw, s)
 }
 
 // JoinSignatures builds a PolicySignature from a map of per-identity