From fad06c765b91cfde85b02ab8aa18cb3e21348d49 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sat, 5 Feb 2022 01:26:25 +1100 Subject: [PATCH] refactor: apply several linting fixes Handle errs, simplify logic, remove checks that are not necessary. --- protocol/attestation_androidkey.go | 2 +- protocol/attestation_packed.go | 18 +++++++++++++----- protocol/attestation_safetynet.go | 8 ++++++++ protocol/attestation_test.go | 2 +- protocol/attestation_tpm.go | 15 ++++++++++++--- protocol/client.go | 4 +--- protocol/webauthncose/webauthncose.go | 25 ++++++++++++++++++++++--- webauthn/login.go | 4 ++-- 8 files changed, 60 insertions(+), 18 deletions(-) diff --git a/protocol/attestation_androidkey.go b/protocol/attestation_androidkey.go index 674cebc..99a81f9 100644 --- a/protocol/attestation_androidkey.go +++ b/protocol/attestation_androidkey.go @@ -78,7 +78,7 @@ func verifyAndroidKeyFormat(att AttestationObject, clientDataHash []byte) (strin } e := pubKey.(webauthncose.EC2PublicKeyData) valid, err = e.Verify(signatureData, sig) - if err != nil || valid != true { + if err != nil || !valid { return androidAttestationKey, nil, ErrInvalidAttestation.WithDetails(fmt.Sprintf("Error parsing public key: %+v\n", err)) } // ยง8.4.3. Verify that the attestationChallenge field in the attestation certificate extension data is identical to clientDataHash. diff --git a/protocol/attestation_packed.go b/protocol/attestation_packed.go index c8a6d1e..7ae90ae 100644 --- a/protocol/attestation_packed.go +++ b/protocol/attestation_packed.go @@ -174,14 +174,22 @@ func handleBasicAttestation(signature, clientDataHash, authData, aaguid []byte, // AAGUID MUST be wrapped in two OCTET STRINGS to be valid. if len(foundAAGUID) > 0 { unMarshalledAAGUID := []byte{} - asn1.Unmarshal(foundAAGUID, &unMarshalledAAGUID) + _, err = asn1.Unmarshal(foundAAGUID, &unMarshalledAAGUID) + if err != nil { + return attestationType, x5c, ErrInvalidAttestation.WithDetails(fmt.Sprintf("AAGUID could not be unmarshalled: %v", err)) + } + if !bytes.Equal(aaguid, unMarshalledAAGUID) { return attestationType, x5c, ErrInvalidAttestation.WithDetails("Certificate AAGUID does not match Auth Data certificate") } } - uuid, err := uuid.FromBytes(aaguid) - if meta, ok := metadata.Metadata[uuid]; ok { + parsedAAGUID, err := uuid.FromBytes(aaguid) + if err != nil { + return attestationType, x5c, ErrInvalidAttestation.WithDetails(fmt.Sprintf("AAGUID could not be parsed: %v", err)) + } + + if meta, ok := metadata.Metadata[parsedAAGUID]; ok { for _, s := range meta.StatusReports { if metadata.IsUndesiredAuthenticatorStatus(metadata.AuthenticatorStatus(s.Status)) { return attestationType, x5c, ErrInvalidAttestation.WithDetails("Authenticator with undesirable status encountered") @@ -191,12 +199,12 @@ func handleBasicAttestation(signature, clientDataHash, authData, aaguid []byte, if attCert.Subject.CommonName != attCert.Issuer.CommonName { var hasBasicFull = false for _, a := range meta.MetadataStatement.AttestationTypes { - if metadata.AuthenticatorAttestationType(a) == metadata.AuthenticatorAttestationType(metadata.BasicFull) { + if metadata.AuthenticatorAttestationType(a) == metadata.BasicFull { hasBasicFull = true } } if !hasBasicFull { - return attestationType, x5c, ErrInvalidAttestation.WithDetails("Attestation with full attestation from authentictor that does not support full attestation") + return attestationType, x5c, ErrInvalidAttestation.WithDetails("Attestation with full attestation from authenticator that does not support full attestation") } } } else { diff --git a/protocol/attestation_safetynet.go b/protocol/attestation_safetynet.go index c5e471a..5a06fb2 100644 --- a/protocol/attestation_safetynet.go +++ b/protocol/attestation_safetynet.go @@ -73,11 +73,19 @@ func verifySafetyNetFormat(att AttestationObject, clientDataHash []byte) (string token, err := jwt.Parse(string(response), func(token *jwt.Token) (interface{}, error) { chain := token.Header["x5c"].([]interface{}) + o := make([]byte, base64.StdEncoding.DecodedLen(len(chain[0].(string)))) + n, err := base64.StdEncoding.Decode(o, []byte(chain[0].(string))) + if err != nil { + return nil, err + } + cert, err := x509.ParseCertificate(o[:n]) + return cert.PublicKey, err }) + if err != nil { return safetyNetAttestationKey, nil, ErrInvalidAttestation.WithDetails(fmt.Sprintf("Error finding cert issued to correct hostname: %+v", err)) } diff --git a/protocol/attestation_test.go b/protocol/attestation_test.go index fb6f916..85dd7eb 100644 --- a/protocol/attestation_test.go +++ b/protocol/attestation_test.go @@ -66,7 +66,7 @@ func attestationTestUnpackResponse(t *testing.T, response string) ParsedCredenti } func TestPackedAttestationVerification(t *testing.T) { - t.Run(fmt.Sprintf("Testing Self Packed"), func(t *testing.T) { + t.Run("Testing Self Packed", func(t *testing.T) { pcc := attestationTestUnpackResponse(t, testAttestationResponses[0]) // Test Packed Verification diff --git a/protocol/attestation_tpm.go b/protocol/attestation_tpm.go index 7e5cea4..f84efd0 100644 --- a/protocol/attestation_tpm.go +++ b/protocol/attestation_tpm.go @@ -78,6 +78,10 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte) (string, []in } key, err := webauthncose.ParsePublicKey(att.AuthData.AttData.CredentialPublicKey) + if err != nil { + return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails(fmt.Sprintf("Public Key could not be parsed in the COSE format: %v", err)) + } + switch k := key.(type) { case webauthncose.EC2PublicKeyData: if pubArea.ECCParameters.CurveID != googletpm.EllipticCurve(k.Curve) || @@ -101,6 +105,10 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte) (string, []in // Validate that certInfo is valid: certInfo, err := googletpm.DecodeAttestationData(certInfoBytes) + if err != nil { + return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails(fmt.Sprintf("Could not decode Attestation data, %v", err)) + } + // 1/4 Verify that magic is set to TPM_GENERATED_VALUE. if certInfo.Magic != 0xff544347 { return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails("Magic is not set to TPM_GENERATED_VALUE") @@ -174,7 +182,7 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte) (string, []in return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails("Invalid SAN data in AIK certificate") } - if false == isValidTPMManufacturer(manufacturer) { + if !isValidTPMManufacturer(manufacturer) { return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails("Invalid TPM manufacturer") } @@ -190,7 +198,7 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte) (string, []in ekuValid = true } } - if false == ekuValid { + if !ekuValid { return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails("AIK certificate missing EKU") } @@ -209,7 +217,8 @@ func verifyTPMFormat(att AttestationObject, clientDataHash []byte) (string, []in } } } - if constraints.IsCA != false { + + if constraints.IsCA { return tpmAttestationKey, nil, ErrAttestationFormat.WithDetails("AIK certificate basic constraints missing or CA is true") } // 6/6 An Authority Information Access (AIA) extension with entry id-ad-ocsp and a CRL Distribution Point diff --git a/protocol/client.go b/protocol/client.go index 73ddf53..0fcbe23 100644 --- a/protocol/client.go +++ b/protocol/client.go @@ -65,9 +65,7 @@ func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyTy // Assertion Step 7. Verify that the value of C.type is the string webauthn.get. if c.Type != ceremony { - err := ErrVerification.WithDetails("Error validating ceremony type") - err.WithInfo(fmt.Sprintf("Expected Value: %s\n Received: %s\n", ceremony, c.Type)) - return err + return ErrVerification.WithDetails("Error validating ceremony type").WithInfo(fmt.Sprintf("Expected Value: %s\n Received: %s\n", ceremony, c.Type)) } // Registration Step 4. Verify that the value of C.challenge matches the challenge diff --git a/protocol/webauthncose/webauthncose.go b/protocol/webauthncose/webauthncose.go index ae5466e..59a311a 100644 --- a/protocol/webauthncose/webauthncose.go +++ b/protocol/webauthncose/webauthncose.go @@ -8,6 +8,7 @@ import ( "crypto/x509" "encoding/asn1" "encoding/pem" + "fmt" "hash" "math/big" @@ -163,18 +164,36 @@ func ParsePublicKey(keyBytes []byte) (interface{}, error) { switch COSEKeyType(pk.KeyType) { case OctetKey: var o OKPPublicKeyData - webauthncbor.Unmarshal(keyBytes, &o) + + err := webauthncbor.Unmarshal(keyBytes, &o) + if err != nil { + return nil, ErrUnsupportedKey.WithDetails(fmt.Sprintf("Could not unmarshall Public Key data: %v", err)) + } + o.PublicKeyData = pk + return o, nil case EllipticKey: var e EC2PublicKeyData - webauthncbor.Unmarshal(keyBytes, &e) + + err := webauthncbor.Unmarshal(keyBytes, &e) + if err != nil { + return nil, ErrUnsupportedKey.WithDetails(fmt.Sprintf("Could not unmarshall Public Key data: %v", err)) + } + e.PublicKeyData = pk + return e, nil case RSAKey: var r RSAPublicKeyData - webauthncbor.Unmarshal(keyBytes, &r) + + err := webauthncbor.Unmarshal(keyBytes, &r) + if err != nil { + return nil, ErrUnsupportedKey.WithDetails(fmt.Sprintf("Could not unmarshall key: %v", err)) + } + r.PublicKeyData = pk + return r, nil default: return nil, ErrUnsupportedKey diff --git a/webauthn/login.go b/webauthn/login.go index a1eb544..44860a6 100644 --- a/webauthn/login.go +++ b/webauthn/login.go @@ -62,7 +62,7 @@ func (webauthn *WebAuthn) BeginLogin(user User, opts ...LoginOption) (*protocol. Extensions: requestOptions.Extensions, } - response := protocol.CredentialAssertion{requestOptions} + response := protocol.CredentialAssertion{Response: requestOptions} return &response, &newSessionData, nil } @@ -143,7 +143,7 @@ func (webauthn *WebAuthn) ValidateLogin(user User, session SessionData, parsedRe // This is in part handled by our Step 1 userHandle := parsedResponse.Response.UserHandle - if userHandle != nil && len(userHandle) > 0 { + if len(userHandle) > 0 { if !bytes.Equal(userHandle, user.WebAuthnID()) { return nil, protocol.ErrBadRequest.WithDetails("userHandle and User ID do not match") }