fix(protocol): ensure attca is parsed correctly

This fixes an issue where the attca attestation (TPM 2.0) was not properly parsed due to a nuance with how the AIK certificates conform to the spec and the critical extension parsing.
go-webauthn · Aug 25, 2024 · 6aeb62b · 6aeb62b
1 parent 33464f5
commit 6aeb62b
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 43 deletions.
diff --git a/protocol/attestation.go b/protocol/attestation.go
@@ -249,6 +249,12 @@ func (a *AttestationObject) VerifyAttestation(clientDataHash []byte, mds metadat
 			return ErrInvalidAttestation.WithDetails("Unable to parse attestation certificate from x5c during attestation validation").WithInfo(fmt.Sprintf("Error returned from x509.ParseCertificate: %+v", err))
 		}
 
+		if attestationType == string(metadata.AttCA) {
+			if err = tpmParseSANExtension(x5c); err != nil {
+				return err
+			}
+		}
+
 		if x5c.Subject.CommonName != x5c.Issuer.CommonName {
 			if !entry.MetadataStatement.AttestationTypes.HasBasicFull() {
 				return ErrInvalidAttestation.WithDetails("Unable to validate attestation statement signature during attestation validation: attestation with full attestation from authenticator that does not support full attestation")

diff --git a/protocol/attestation_tpm.go b/protocol/attestation_tpm.go
@@ -147,15 +147,15 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte, _ metadata.Pr
 			return "", nil, ErrAttestation.WithDetails("Error getting certificate from x5c cert chain")
 		}
 
-		aikCert, err := x509.ParseCertificate(aikCertBytes)
-		if err != nil {
+		var aikCert *x509.Certificate
+
+		if aikCert, err = x509.ParseCertificate(aikCertBytes); err != nil {
 			return "", nil, ErrAttestationFormat.WithDetails("Error parsing certificate from ASN.1")
 		}
 
 		sigAlg := webauthncose.SigAlgFromCOSEAlg(coseAlg)
 
-		err = aikCert.CheckSignature(x509.SignatureAlgorithm(sigAlg), certInfoBytes, sigBytes)
-		if err != nil {
+		if err = aikCert.CheckSignature(x509.SignatureAlgorithm(sigAlg), certInfoBytes, sigBytes); err != nil {
 			return "", nil, ErrAttestationFormat.WithDetails(fmt.Sprintf("Signature validation error: %+v\n", err))
 		}
 		// Verify that aikCert meets the requirements in §8.3.1 TPM Attestation Statement Certificate Requirements
@@ -164,23 +164,41 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte, _ metadata.Pr
 		if aikCert.Version != 3 {
 			return "", nil, ErrAttestationFormat.WithDetails("AIK certificate version must be 3")
 		}
+
 		// 2/6 Subject field MUST be set to empty.
 		if aikCert.Subject.String() != "" {
 			return "", nil, ErrAttestationFormat.WithDetails("AIK certificate subject must be empty")
 		}
 
-		// 3/6 The Subject Alternative Name extension MUST be set as defined in [TPMv2-EK-Profile] section 3.2.9{}
-		var manufacturer, model, version string
+		var (
+			manufacturer, model, version string
+			ekuValid                     = false
+			eku                          []asn1.ObjectIdentifier
+			constraints                  tpmBasicConstraints
+			rest                         []byte
+		)
 
 		for _, ext := range aikCert.Extensions {
-			if ext.Id.Equal([]int{2, 5, 29, 17}) {
-				manufacturer, model, version, err = parseSANExtension(ext.Value)
-				if err != nil {
+			if ext.Id.Equal(oidExtensionSubjectAltName) {
+				if manufacturer, model, version, err = parseSANExtension(ext.Value); err != nil {
 					return "", nil, err
 				}
+			} else if ext.Id.Equal(oidExtensionExtendedKeyUsage) {
+				if rest, err = asn1.Unmarshal(ext.Value, &eku); len(rest) != 0 || err != nil || !eku[0].Equal(tcgKpAIKCertificate) {
+					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate EKU missing 2.23.133.8.3")
+				}
+
+				ekuValid = true
+			} else if ext.Id.Equal(oidExtensionBasicConstraints) {
+				if rest, err = asn1.Unmarshal(ext.Value, &constraints); err != nil {
+					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate basic constraints malformed")
+				} else if len(rest) != 0 {
+					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate basic constraints contains extra data")
+				}
 			}
 		}
 
+		// 3/6 The Subject Alternative Name extension MUST be set as defined in [TPMv2-EK-Profile] section 3.2.9{}
 		if manufacturer == "" || model == "" || version == "" {
 			return "", nil, ErrAttestationFormat.WithDetails("Invalid SAN data in AIK certificate")
 		}
@@ -190,44 +208,10 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte, _ metadata.Pr
 		}
 
 		// 4/6 The Extended Key Usage extension MUST contain the "joint-iso-itu-t(2) internationalorganizations(23) 133 tcg-kp(8) tcg-kp-AIKCertificate(3)" OID.
-		var (
-			ekuValid = false
-			eku      []asn1.ObjectIdentifier
-		)
-
-		for _, ext := range aikCert.Extensions {
-			if ext.Id.Equal([]int{2, 5, 29, 37}) {
-				rest, err := asn1.Unmarshal(ext.Value, &eku)
-				if len(rest) != 0 || err != nil || !eku[0].Equal(tcgKpAIKCertificate) {
-					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate EKU missing 2.23.133.8.3")
-				}
-
-				ekuValid = true
-			}
-		}
-
 		if !ekuValid {
 			return "", nil, ErrAttestationFormat.WithDetails("AIK certificate missing EKU")
 		}
 
-		// 5/6 The Basic Constraints extension MUST have the CA component set to false.
-		type basicConstraints struct {
-			IsCA       bool `asn1:"optional"`
-			MaxPathLen int  `asn1:"optional,default:-1"`
-		}
-
-		var constraints basicConstraints
-
-		for _, ext := range aikCert.Extensions {
-			if ext.Id.Equal([]int{2, 5, 29, 19}) {
-				if rest, err := asn1.Unmarshal(ext.Value, &constraints); err != nil {
-					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate basic constraints malformed")
-				} else if len(rest) != 0 {
-					return "", nil, ErrAttestationFormat.WithDetails("AIK certificate basic constraints contains extra data")
-				}
-			}
-		}
-
 		// 6/6 An Authority Information Access (AIA) extension with entry id-ad-ocsp and a CRL Distribution Point
 		// extension [RFC5280] are both OPTIONAL as the status of many attestation certificates is available
 		// through metadata services. See, for example, the FIDO Metadata Service.
@@ -378,3 +362,46 @@ func isValidTPMManufacturer(id string) bool {
 
 	return false
 }
+
+func tpmParseSANExtension(attestation *x509.Certificate) (err error) {
+	var (
+		manufacturer, model, version string
+	)
+
+	for _, ext := range attestation.Extensions {
+		if ext.Id.Equal(oidExtensionSubjectAltName) {
+			if manufacturer, model, version, err = parseSANExtension(ext.Value); err != nil {
+				return ErrInvalidAttestation.WithDetails("Authenticator with invalid AIK SAN data encountered during attestation validation.").WithInfo(fmt.Sprintf("Error occurred parsing SAN extension: %s", err.Error()))
+			}
+		}
+	}
+
+	if manufacturer == "" || model == "" || version == "" {
+		return ErrAttestationFormat.WithDetails("Invalid SAN data in AIK certificate")
+	}
+
+	var unhandled []asn1.ObjectIdentifier
+
+	for _, uce := range attestation.UnhandledCriticalExtensions {
+		if uce.Equal(oidExtensionSubjectAltName) {
+			continue
+		}
+
+		unhandled = append(unhandled, uce)
+	}
+
+	attestation.UnhandledCriticalExtensions = unhandled
+
+	return nil
+}
+
+var (
+	oidExtensionSubjectAltName   = []int{2, 5, 29, 17}
+	oidExtensionExtendedKeyUsage = []int{2, 5, 29, 37}
+	oidExtensionBasicConstraints = []int{2, 5, 29, 19}
+)
+
+type tpmBasicConstraints struct {
+	IsCA       bool `asn1:"optional"`
+	MaxPathLen int  `asn1:"optional,default:-1"`
+}