Address additional PR review comments

Security improvements:
- Add ReadBoundedBytes/ReadBoundedString to BCS deserializer for DoS protection
- WebAuthn: Use bounded reads to validate size BEFORE allocation
- Keyless: Use bounded reads for JWT sig/payload, uid key, etc.
- Keyless: IdCommitment now defensively copies bytes to prevent mutation

Documentation fixes:
- Fix WebAuthn verification comment to accurately describe ES256 flow
- Update concurrency safety comments for Ed25519/Secp256k1/Secp256r1 private keys
  to clarify that Inner must not be mutated during concurrent operations

Test updates:
- Update WebAuthn bounds tests to match new generic error messages

Author: gregnazario

v2/internal/bcs/deserializer.go

@@ -248,6 +248,53 @@ func (des *Deserializer) ReadBytes() []byte {
 	return des.ReadFixedBytes(int(length))
 }
 
+// ReadBoundedBytes deserializes a byte slice with bounds checking BEFORE allocation.
+// This provides DoS protection by rejecting oversized payloads before allocating memory.
+// Returns nil and sets error if length is outside [minLen, maxLen].
+func (des *Deserializer) ReadBoundedBytes(minLen, maxLen int) []byte {
+	if des.err != nil {
+		return nil
+	}
+
+	length := des.Uleb128()
+	if des.err != nil {
+		return nil
+	}
+
+	// Validate bounds BEFORE allocation to prevent DoS
+	if int(length) < minLen {
+		des.SetError(fmt.Errorf("byte slice too short: %d < %d", length, minLen))
+		return nil
+	}
+	if int(length) > maxLen {
+		des.SetError(fmt.Errorf("byte slice too large: %d > %d", length, maxLen))
+		return nil
+	}
+
+	return des.ReadFixedBytes(int(length))
+}
+
+// ReadBoundedString deserializes a string with bounds checking BEFORE allocation.
+// This provides DoS protection by rejecting oversized payloads before allocating memory.
+func (des *Deserializer) ReadBoundedString(maxLen int) string {
+	if des.err != nil {
+		return ""
+	}
+
+	length := des.Uleb128()
+	if des.err != nil {
+		return ""
+	}
+
+	// Validate bounds BEFORE allocation
+	if int(length) > maxLen {
+		des.SetError(fmt.Errorf("string too large: %d > %d", length, maxLen))
+		return ""
+	}
+
+	return string(des.ReadFixedBytes(int(length)))
+}
+
 // ReadString deserializes a UTF-8 string with a ULEB128 length prefix.
 func (des *Deserializer) ReadString() string {
 	return string(des.ReadBytes())

v2/internal/crypto/crypto_test.go

@@ -2097,7 +2097,7 @@ func TestWebAuthn_BoundsValidation(t *testing.T) {
 		paar2 := &PartialAuthenticatorAssertionResponse{}
 		err := bcs.Deserialize(paar2, data)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "authenticator data too short")
+		assert.Contains(t, err.Error(), "too short")
 	})
 
 	// Test authenticator data too large
@@ -2110,7 +2110,7 @@ func TestWebAuthn_BoundsValidation(t *testing.T) {
 		paar2 := &PartialAuthenticatorAssertionResponse{}
 		err := bcs.Deserialize(paar2, data)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "authenticator data too large")
+		assert.Contains(t, err.Error(), "too large")
 	})
 
 	// Test client data JSON too large
@@ -2124,7 +2124,7 @@ func TestWebAuthn_BoundsValidation(t *testing.T) {
 		paar2 := &PartialAuthenticatorAssertionResponse{}
 		err := bcs.Deserialize(paar2, data)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "client data JSON too large")
+		assert.Contains(t, err.Error(), "too large")
 	})
 
 	// Test empty client data JSON
@@ -2136,7 +2136,7 @@ func TestWebAuthn_BoundsValidation(t *testing.T) {
 		paar2 := &PartialAuthenticatorAssertionResponse{}
 		err := bcs.Deserialize(paar2, data)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "client data JSON is empty")
+		assert.Contains(t, err.Error(), "too short")
 	})
 }
 

v2/internal/crypto/ed25519.go

@@ -18,7 +18,9 @@ import (
 // to be wrapped with SingleSigner). This uses the legacy Ed25519Scheme for
 // backward compatibility.
 //
-// Ed25519PrivateKey is safe for concurrent use. Cached values are protected by a mutex.
+// Ed25519PrivateKey protects its cached values with a mutex for concurrent reads.
+// The underlying key material in Inner must not be mutated concurrently with
+// operations that read it (such as signing).
 //
 // Implements:
 //   - [Signer]

v2/internal/crypto/keyless.go

@@ -31,6 +31,23 @@ const (
 
 	// EpkBlinderNumBytes is the size of the EPK blinder.
 	EpkBlinderNumBytes = 31
+
+	// MaxJwtSigLen is the maximum length of a JWT signature (DoS protection).
+	// RSA-4096 signatures are 512 bytes; this allows room for future algorithms.
+	MaxJwtSigLen = 1024
+
+	// MaxJwtPayloadLen is the maximum length of a JWT payload JSON (DoS protection).
+	// Standard JWTs should be under 8KB.
+	MaxJwtPayloadLen = 16384
+
+	// MaxUidKeyLen is the maximum length of a uid key string.
+	MaxUidKeyLen = 128
+
+	// MaxAudValLen is the maximum length of an audience value override.
+	MaxAudValLen = 512
+
+	// MaxIssValLen is the maximum length of an issuer value.
+	MaxIssValLen = 200
 )
 
 // Pepper is used to create a hiding identity commitment (IDC) when deriving a keyless address.
@@ -53,16 +70,26 @@ type IdCommitment struct {
 }
 
 // NewIdCommitment creates a new identity commitment from bytes.
+// The input bytes are copied to prevent external mutation after construction.
 func NewIdCommitment(bytes []byte) (*IdCommitment, error) {
 	if len(bytes) != IdCommitmentNumBytes {
 		return nil, fmt.Errorf("invalid identity commitment length: expected %d, got %d", IdCommitmentNumBytes, len(bytes))
 	}
-	return &IdCommitment{inner: bytes}, nil
+	// Defensively copy the input bytes so callers cannot mutate the commitment after construction
+	inner := make([]byte, IdCommitmentNumBytes)
+	copy(inner, bytes)
+	return &IdCommitment{inner: inner}, nil
 }
 
-// Bytes returns the raw bytes of the identity commitment.
+// Bytes returns a copy of the raw bytes of the identity commitment.
+// The returned slice is a copy to prevent external mutation.
 func (idc *IdCommitment) Bytes() []byte {
-	return idc.inner
+	if idc == nil || idc.inner == nil {
+		return nil
+	}
+	out := make([]byte, len(idc.inner))
+	copy(out, idc.inner)
+	return out
 }
 
 // MarshalBCS serializes the identity commitment to BCS.
@@ -155,7 +182,8 @@ func (key *KeylessPublicKey) MarshalBCS(ser *bcs.Serializer) {
 
 // UnmarshalBCS deserializes the keyless public key from BCS.
 func (key *KeylessPublicKey) UnmarshalBCS(des *bcs.Deserializer) {
-	key.IssVal = des.ReadString()
+	// Use bounded read for issuer value (DoS protection)
+	key.IssVal = des.ReadBoundedString(MaxIssValLen)
 	if des.Error() != nil {
 		return
 	}
@@ -436,23 +464,26 @@ func (o *OpenIdSig) MarshalBCS(ser *bcs.Serializer) {
 }
 
 // UnmarshalBCS deserializes the OpenIdSig from BCS.
+// Uses bounded reads to prevent DoS attacks from oversized payloads.
 func (o *OpenIdSig) UnmarshalBCS(des *bcs.Deserializer) {
-	o.JwtSig = des.ReadBytes()
+	// Use bounded reads to validate size BEFORE allocation (DoS protection)
+	o.JwtSig = des.ReadBoundedBytes(1, MaxJwtSigLen)
 	if des.Error() != nil {
 		return
 	}
 
-	o.JwtPayloadJSON = des.ReadString()
+	o.JwtPayloadJSON = des.ReadBoundedString(MaxJwtPayloadLen)
 	if des.Error() != nil {
 		return
 	}
 
-	o.UidKey = des.ReadString()
+	o.UidKey = des.ReadBoundedString(MaxUidKeyLen)
 	if des.Error() != nil {
 		return
 	}
 
-	o.EpkBlinder = des.ReadBytes()
+	// EPK blinder has a fixed size
+	o.EpkBlinder = des.ReadBoundedBytes(EpkBlinderNumBytes, EpkBlinderNumBytes)
 	if des.Error() != nil {
 		return
 	}
@@ -464,7 +495,7 @@ func (o *OpenIdSig) UnmarshalBCS(des *bcs.Deserializer) {
 
 	// Optional idc_aud_val
 	if des.Bool() {
-		s := des.ReadString()
+		s := des.ReadBoundedString(MaxAudValLen)
 		o.IdcAudVal = &s
 	}
 }

v2/internal/crypto/secp256k1.go

@@ -27,7 +27,10 @@ const (
 //	signer := crypto.NewSingleSigner(key)
 //	auth, _ := signer.Sign(txnBytes)
 //
-// Secp256k1PrivateKey is safe for concurrent use. Cached values are protected by a mutex.
+// Secp256k1PrivateKey supports concurrent read-only use after initialization.
+// The mutex protects cached values (such as the derived public key), but callers
+// must not mutate the underlying key material (e.g., via FromBytes) concurrently
+// with signing or other operations that use the key.
 //
 // Implements:
 //   - [MessageSigner]

v2/internal/crypto/secp256r1.go

@@ -36,7 +36,11 @@ const (
 //	signer := crypto.NewSingleSigner(key)
 //	auth, _ := signer.Sign(txnBytes)
 //
-// Secp256r1PrivateKey is safe for concurrent use. Cached values are protected by a mutex.
+// Secp256r1PrivateKey uses a mutex to protect cached values (such as cachedPubKey)
+// for concurrent read access. The underlying Inner key is not intended to be
+// mutated or replaced concurrently with operations like SignMessage; callers
+// must coordinate any re-keying externally (for example, by publishing a new
+// Secp256r1PrivateKey instance rather than mutating an existing one).
 //
 // Implements:
 //   - [MessageSigner]

v2/internal/crypto/webauthn.go

@@ -158,35 +158,17 @@ func (p *PartialAuthenticatorAssertionResponse) UnmarshalBCS(des *bcs.Deserializ
 		return
 	}
 
-	p.AuthenticatorData = des.ReadBytes()
+	// Use bounded read to validate size BEFORE allocation (DoS protection)
+	p.AuthenticatorData = des.ReadBoundedBytes(MinAuthenticatorDataBytes, MaxAuthenticatorDataBytes)
 	if des.Error() != nil {
 		return
 	}
 
-	// Validate authenticator data bounds
-	if len(p.AuthenticatorData) < MinAuthenticatorDataBytes {
-		des.SetError(fmt.Errorf("authenticator data too short: %d < %d", len(p.AuthenticatorData), MinAuthenticatorDataBytes))
-		return
-	}
-	if len(p.AuthenticatorData) > MaxAuthenticatorDataBytes {
-		des.SetError(fmt.Errorf("authenticator data too large: %d > %d", len(p.AuthenticatorData), MaxAuthenticatorDataBytes))
-		return
-	}
-
-	p.ClientDataJSON = des.ReadBytes()
+	// Use bounded read for client data JSON (DoS protection)
+	p.ClientDataJSON = des.ReadBoundedBytes(1, MaxClientDataJSONBytes)
 	if des.Error() != nil {
 		return
 	}
-
-	// Validate client data JSON bounds
-	if len(p.ClientDataJSON) == 0 {
-		des.SetError(fmt.Errorf("client data JSON is empty"))
-		return
-	}
-	if len(p.ClientDataJSON) > MaxClientDataJSONBytes {
-		des.SetError(fmt.Errorf("client data JSON too large: %d > %d", len(p.ClientDataJSON), MaxClientDataJSONBytes))
-		return
-	}
 }
 
 // GetChallenge extracts and decodes the challenge from the client data JSON.
@@ -281,7 +263,9 @@ func (p *PartialAuthenticatorAssertionResponse) Verify(msg []byte, pubKey *AnyPu
 		if !ok {
 			return false
 		}
-		// For WebAuthn, we verify directly over the verification data without additional hashing
+		// For WebAuthn (ES256), the signature is over SHA-256(authenticatorData || clientDataHash).
+		// Here, verificationData is (authenticatorData || clientDataHash), and verifySecp256r1Raw
+		// performs the required SHA-256 before calling ecdsa.Verify.
 		return p.verifySecp256r1Raw(verificationData, secp256r1PubKey)
 	default:
 		return false