fix(protocol): possible panic conditions (#719)

This fixes a few possible panic conditions which would result in an unfavorable output. Since the http.Server handles panics correctly this is not a risk like it would be in other languages.

Author: james-d-elliott

protocol/attestation_tpm.go

@@ -10,6 +10,7 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
+	"math"
 	"strings"
 
 	"github.com/google/go-tpm/tpm2"
@@ -116,7 +117,16 @@ func attestationFormatValidationHandlerTPM(att AttestationObject, clientDataHash
 			return "", nil, ErrAttestationFormat.WithDetails("Mismatch between RSAParameters in pubArea and credentialPublicKey")
 		}
 
-		exp := uint32(k.Exponent[0]) + uint32(k.Exponent[1])<<8 + uint32(k.Exponent[2])<<16
+		var e int
+
+		if e, err = webauthncose.ParseRSAPublicKeyDataExponent(&k); err != nil {
+			return "", nil, ErrAttestationFormat.WithDetails("Unable to decode RSA exponent in attestation statement").WithError(err)
+		} else if uint64(e) > math.MaxUint32 { //nolint:gosec // The exponent is guaranteed to be positive by the parser.
+			return "", nil, ErrAttestationFormat.WithDetails("Invalid RSA public key size")
+		}
+
+		exp := uint32(e) //nolint:gosec // The exponent is bounds checked above.
+
 		if tpm2Exponent(params) != exp {
 			return "", nil, ErrAttestationFormat.WithDetails("Mismatch between RSAParameters in pubArea and credentialPublicKey")
 		}

protocol/attestation_tpm_test.go

@@ -737,6 +737,93 @@ func TestTPMAttestationVerificationFailPubArea(t *testing.T) {
 	}
 }
 
+func TestTPMAttestationVerificationRSAExponent(t *testing.T) {
+	_, _, _, rsaKey, _, err := getTPMAttestionKeys()
+	require.NoError(t, err)
+
+	testCases := []struct {
+		name            string
+		exponent        []byte
+		pubAreaExponent uint32
+		err             string
+	}{
+		{
+			"ShouldNotPanicWithSingleByteExponentMatchingPubArea",
+			[]byte{0x03},
+			3,
+			"unmarshalling field 1 of struct of type 'tpm2.TPMSAttest', EOF",
+		},
+		{
+			"ShouldNotPanicWithSingleByteExponentMismatchingPubArea",
+			[]byte{0x03},
+			65537,
+			"Mismatch between RSAParameters in pubArea and credentialPublicKey",
+		},
+		{
+			"ShouldNotPanicWithTwoByteExponentMatchingPubArea",
+			[]byte{0x01, 0x00},
+			256,
+			"unmarshalling field 1 of struct of type 'tpm2.TPMSAttest', EOF",
+		},
+		{
+			"ShouldNotPanicWithTwoByteExponentMismatchingPubArea",
+			[]byte{0x01, 0x00},
+			1,
+			"Mismatch between RSAParameters in pubArea and credentialPublicKey",
+		},
+		{
+			"ShouldDecodeThreeByteExponentAsBigEndian",
+			[]byte{0x01, 0x00, 0x01},
+			65537,
+			"unmarshalling field 1 of struct of type 'tpm2.TPMSAttest', EOF",
+		},
+		{
+			"ShouldRejectExponentExceedingMaxUint32",
+			[]byte{0x01, 0x00, 0x00, 0x00, 0x00},
+			65537,
+			"Invalid RSA public key size",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cpk, cerr := webauthncbor.Marshal(webauthncose.RSAPublicKeyData{
+				PublicKeyData: webauthncose.PublicKeyData{
+					KeyType:   int64(webauthncose.RSAKey),
+					Algorithm: int64(webauthncose.AlgRS256),
+				},
+				Modulus:  rsaKey.N.Bytes(),
+				Exponent: tc.exponent,
+			})
+			require.NoError(t, cerr)
+
+			attStmt := make(map[string]any, len(defaultAttStatement))
+			for id, v := range defaultAttStatement {
+				attStmt[id] = v
+			}
+
+			attStmt[stmtPubArea] = tpm2.Marshal(makeTPMTPublicRSA(&TPMRSATestParameters{Modulus: rsaKey.N.Bytes(), Exponent: tc.pubAreaExponent}))
+
+			att := AttestationObject{
+				AttStatement: attStmt,
+				AuthData: AuthenticatorData{
+					AttData: AttestedCredentialData{
+						CredentialPublicKey: cpk,
+					},
+				},
+			}
+
+			require.NotPanics(t, func() {
+				attestationType, x5cs, aerr := attestationFormatValidationHandlerTPM(att, nil, nil)
+
+				assert.Empty(t, attestationType)
+				assert.Nil(t, x5cs)
+				assert.EqualError(t, aerr, tc.err)
+			})
+		})
+	}
+}
+
 func TestTPMAttestationVerificationFailCertInfo(t *testing.T) {
 	h := webauthncose.HasherFromCOSEAlg(webauthncose.AlgRS256)
 	extraData := h.Sum(nil)

protocol/authenticator.go

@@ -313,11 +313,12 @@ func (a *AuthenticatorData) Unmarshal(rawAuthData []byte) (err error) {
 
 	if a.Flags.HasAttestedCredentialData() {
 		if len(rawAuthData) > minAttestedAuthLength {
-			if err = a.unmarshalAttestedData(rawAuthData); err != nil {
+			var attDataLen int
+
+			if attDataLen, err = a.unmarshalAttestedData(rawAuthData); err != nil {
 				return err
 			}
 
-			attDataLen := len(a.AttData.AAGUID) + 2 + len(a.AttData.CredentialID) + len(a.AttData.CredentialPublicKey)
 			remaining -= attDataLen
 		} else {
 			return ErrBadRequest.WithDetails("Attested credential flag set but data is missing")
@@ -344,42 +345,49 @@ func (a *AuthenticatorData) Unmarshal(rawAuthData []byte) (err error) {
 	return nil
 }
 
-// If Attestation Data is present, unmarshall that into the appropriate public key structure.
-func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) (err error) {
+// If Attestation Data is present, unmarshall that into the appropriate public key structure. Returns the number of
+// bytes of rawAuthData which the attested credential data occupied, measured from the start of the AAGUID.
+func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) (n int, err error) {
 	a.AttData.AAGUID = rawAuthData[37:53]
 
-	idLength := binary.BigEndian.Uint16(rawAuthData[53:55])
-	if len(rawAuthData) < int(55+idLength) {
-		return ErrBadRequest.WithDetails("Authenticator attestation data length too short")
-	}
+	idLength := int(binary.BigEndian.Uint16(rawAuthData[53:55]))
 
 	if idLength > maxCredentialIDLength {
-		return ErrBadRequest.WithDetails("Authenticator attestation data credential id length too long")
+		return 0, ErrBadRequest.WithDetails("Authenticator attestation data credential id length too long")
+	}
+
+	if len(rawAuthData) < 55+idLength {
+		return 0, ErrBadRequest.WithDetails("Authenticator attestation data length too short")
 	}
 
 	a.AttData.CredentialID = rawAuthData[55 : 55+idLength]
 
-	a.AttData.CredentialPublicKey, err = unmarshalCredentialPublicKey(rawAuthData[55+idLength:])
-	if err != nil {
-		return ErrBadRequest.WithDetails(fmt.Sprintf("Could not unmarshal Credential Public Key: %v", err)).WithError(err)
+	var keyLength int
+
+	if a.AttData.CredentialPublicKey, keyLength, err = unmarshalCredentialPublicKey(rawAuthData[55+idLength:]); err != nil {
+		return 0, ErrBadRequest.WithDetails(fmt.Sprintf("Could not unmarshal Credential Public Key: %v", err)).WithError(err)
 	}
 
-	return nil
+	// The AAGUID is 16 bytes and the credential id length prefix is 2 bytes.
+	return 16 + 2 + idLength + keyLength, nil
 }
 
-// Unmarshall the credential's Public Key into CBOR encoding.
-func unmarshalCredentialPublicKey(keyBytes []byte) (rawBytes []byte, err error) {
+// Unmarshall the credential's Public Key into CBOR encoding. Returns the re-encoded key alongside the number of bytes
+// of keyBytes which the key occupied on the wire. These lengths are not necessarily equal as the CTAP2 canonical form
+// produced by Marshal may be longer or shorter than the form which was decoded, so the consumed length must be used
+// when locating any data which follows the key.
+func unmarshalCredentialPublicKey(keyBytes []byte) (rawBytes []byte, n int, err error) {
 	var m any
 
-	if err = webauthncbor.Unmarshal(keyBytes, &m); err != nil {
-		return nil, err
+	if n, err = webauthncbor.UnmarshalFirst(keyBytes, &m); err != nil {
+		return nil, 0, err
 	}
 
 	if rawBytes, err = webauthncbor.Marshal(m); err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 
-	return rawBytes, nil
+	return rawBytes, n, nil
 }
 
 // ResidentKeyRequired - Require that the key be private key resident to the client device.

protocol/authenticator_oob_test.go

@@ -0,0 +1,69 @@
+package protocol
+
+import (
+	"encoding/hex"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestAuthenticatorData_Unmarshal_CredentialPublicKeyReencodingExpands(t *testing.T) {
+	rawPublicKey, err := hex.DecodeString("f9f4cb")
+	require.NoError(t, err)
+
+	rawAuthData := buildAuthData(0xC0, nil, rawPublicKey)
+
+	var a AuthenticatorData
+
+	require.NotPanics(t, func() {
+		err = a.Unmarshal(rawAuthData)
+	})
+
+	assert.Error(t, err)
+}
+
+func TestAuthenticatorData_Unmarshal_CredentialPublicKeyReencodingShrinks(t *testing.T) {
+	rawPublicKey, err := hex.DecodeString("a119000102")
+	require.NoError(t, err)
+
+	rawAuthData := buildAuthData(0xC0, nil, rawPublicKey)
+
+	var a AuthenticatorData
+
+	require.NotPanics(t, func() {
+		err = a.Unmarshal(rawAuthData)
+	})
+
+	assert.Error(t, err)
+	assert.Empty(t, a.ExtData)
+}
+
+func TestAuthenticatorData_Unmarshal_TrailingBytesRejected(t *testing.T) {
+	rawPublicKey, err := hex.DecodeString("a10102ff")
+	require.NoError(t, err)
+
+	rawAuthData := buildAuthData(0x40, nil, rawPublicKey)
+
+	var a AuthenticatorData
+
+	require.NotPanics(t, func() {
+		err = a.Unmarshal(rawAuthData)
+	})
+
+	assert.EqualError(t, err, "Leftover bytes decoding AuthenticatorData")
+}
+
+func buildAuthData(flags byte, credentialID, rawPublicKey []byte) []byte {
+	data := make([]byte, 0, 55+len(credentialID)+len(rawPublicKey))
+
+	data = append(data, make([]byte, 32)...)    // RPIDHash.
+	data = append(data, flags)                  // Flags.
+	data = append(data, 0x00, 0x00, 0x00, 0x01) // Counter.
+	data = append(data, make([]byte, 16)...)    // AAGUID.
+	data = append(data, byte(len(credentialID)>>8), byte(len(credentialID))) //nolint:gosec // Test data is bounded.
+	data = append(data, credentialID...)
+	data = append(data, rawPublicKey...)
+
+	return data
+}

protocol/authenticator_test.go

@@ -358,7 +358,7 @@ func TestAuthenticatorData_unmarshalAttestedData(t *testing.T) {
 				ExtData:  tc.fields.ExtData,
 			}
 
-			err := actual.unmarshalAttestedData(tc.args.rawAuthData)
+			_, err := actual.unmarshalAttestedData(tc.args.rawAuthData)
 
 			if tc.err != "" {
 				assert.EqualError(t, err, tc.err)

protocol/webauthncbor/webauthncbor.go

@@ -19,12 +19,25 @@ var ctap2CBOREncMode, _ = cbor.CTAP2EncOptions().EncMode()
 // following the CTAP2 canonical CBOR encoding form.
 // (https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#message-encoding)
 func Unmarshal(data []byte, v any) error {
-	// TODO (james-d-elliott): investigate the specific use case for Unmarshal vs UnmarshalFirst to determine the edge cases where this may be useful.
 	_, err := ctap2CBORDecMode.UnmarshalFirst(data, v)
 
 	return err
 }
 
+// UnmarshalFirst parses the first CBOR-encoded item in data into the value pointed to by v following the CTAP2
+// canonical CBOR encoding form, and returns the number of bytes of data which that item consumed. Callers which decode
+// an item embedded in a larger byte sequence must use this rather than deriving a length from the re-encoded output of
+// Marshal, as the encoded form is not guaranteed to be the same size as the form which was decoded.
+func UnmarshalFirst(data []byte, v any) (n int, err error) {
+	var rest []byte
+
+	if rest, err = ctap2CBORDecMode.UnmarshalFirst(data, v); err != nil {
+		return 0, err
+	}
+
+	return len(data) - len(rest), nil
+}
+
 // Marshal encodes the value pointed to by v
 // following the CTAP2 canonical CBOR encoding form.
 // (https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#message-encoding)

protocol/webauthncose/webauthncose.go

@@ -14,11 +14,10 @@ import (
 	"math"
 	"math/big"
 
-	"github.com/go-webauthn/x/encoding/asn1"
-
 	"github.com/google/go-tpm/tpm2"
 
 	"github.com/go-webauthn/webauthn/protocol/webauthncbor"
+	"github.com/go-webauthn/x/encoding/asn1"
 )
 
 // PublicKeyData The public key portion of a Relying Party-specific credential key pair, generated
@@ -129,12 +128,12 @@ func (k *EC2PublicKeyData) ToECDSA() (key *ecdsa.PublicKey, err error) {
 
 // Verify RSA Public Key Signature.
 func (k *RSAPublicKeyData) Verify(data []byte, sig []byte) (valid bool, err error) {
-	if err = validateRSAPublicKey(k); err != nil {
+	var e int
+
+	if e, err = validateRSAPublicKey(k); err != nil {
 		return false, err
 	}
 
-	e, _ := parseRSAPublicKeyDataExponent(k)
-
 	pubkey := &rsa.PublicKey{
 		N: big.NewInt(0).SetBytes(k.Modulus),
 		E: e,
@@ -211,7 +210,7 @@ func ParsePublicKey(keyBytes []byte) (publicKey any, err error) {
 
 		r.PublicKeyData = pk
 
-		if err = validateRSAPublicKey(&r); err != nil {
+		if _, err = validateRSAPublicKey(&r); err != nil {
 			return nil, err
 		}
 
@@ -267,7 +266,7 @@ func DisplayPublicKey(cpk []byte) string {
 	case RSAPublicKeyData:
 		var e int
 
-		if e, err = parseRSAPublicKeyDataExponent(&k); err != nil {
+		if e, err = ParseRSAPublicKeyDataExponent(&k); err != nil {
 			return keyCannotDisplay
 		}
 
@@ -454,20 +453,21 @@ func validateEC2PublicKey(k *EC2PublicKeyData) error {
 	return nil
 }
 
-func validateRSAPublicKey(k *RSAPublicKeyData) error {
+func validateRSAPublicKey(k *RSAPublicKeyData) (e int, err error) {
 	n := new(big.Int).SetBytes(k.Modulus)
 	if n.Sign() <= 0 {
-		return ErrUnsupportedKey.WithDetails("RSA key contains zero or empty modulus")
+		return e, ErrUnsupportedKey.WithDetails("RSA key contains zero or empty modulus")
 	}
 
-	if _, err := parseRSAPublicKeyDataExponent(k); err != nil {
-		return ErrUnsupportedKey.WithDetails(fmt.Sprintf("RSA key contains invalid exponent: %v", err))
+	if e, err = ParseRSAPublicKeyDataExponent(k); err != nil {
+		return e, ErrUnsupportedKey.WithDetails(fmt.Sprintf("RSA key contains invalid exponent: %v", err))
 	}
 
-	return nil
+	return e, nil
 }
 
-func parseRSAPublicKeyDataExponent(k *RSAPublicKeyData) (exp int, err error) {
+// ParseRSAPublicKeyDataExponent safely parses the exponent value of the provided RSAPublicKeyData.
+func ParseRSAPublicKeyDataExponent(k *RSAPublicKeyData) (exp int, err error) {
 	if k == nil {
 		return 0, fmt.Errorf("invalid key")
 	}

protocol/webauthncose/webauthncose_test.go

@@ -182,7 +182,7 @@ func TestRSAExponent(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			actual, err := parseRSAPublicKeyDataExponent(tc.have)
+			actual, err := ParseRSAPublicKeyDataExponent(tc.have)
 
 			if tc.err != "" {
 				assert.EqualError(t, err, tc.err)