Update VerifyAttestation logic

internal/quote.go

@@ -12,6 +12,10 @@ import (
 	"github.com/google/go-tpm/tpm2"
 )
 
+// SignatureHashAlgs are the hash algorithms we support for Quote signatures, in
+// their preferred order of use.
+var SignatureHashAlgs = []tpm2.Algorithm{tpm2.AlgSHA512, tpm2.AlgSHA384, tpm2.AlgSHA256}
+
 // VerifyQuote performs the following checks to validate a Quote:
 //    - the provided signature is generated by the trusted AK public key
 //    - the signature signs the provided quote data
@@ -20,6 +24,7 @@ import (
 //    - the quote data was taken over the provided PCRs
 //    - the provided PCR values match the quote data internal digest
 //    - the provided extraData matches that in the quote data
+//    - the signature hash algorithm must be in HashAlgs
 // Note that the caller must have already established trust in the provided
 // public key before validating the Quote.
 //
@@ -30,27 +35,22 @@ func VerifyQuote(q *pb.Quote, trustedPub crypto.PublicKey, extraData []byte) err
 		return fmt.Errorf("signature decoding failed: %v", err)
 	}
 
-	var hash crypto.Hash
+	hash, err := verifyHashAlg(sig)
+	if err != nil {
+		return err
+	}
+
 	switch pub := trustedPub.(type) {
 	case *ecdsa.PublicKey:
-		hash, err = sig.ECC.HashAlg.Hash()
-		if err != nil {
-			return err
-		}
 		if err = verifyECDSAQuoteSignature(pub, hash, q.GetQuote(), sig); err != nil {
 			return err
 		}
 	case *rsa.PublicKey:
-		hash, err = sig.RSA.HashAlg.Hash()
-		if err != nil {
-			return err
-		}
 		if err = verifyRSASSAQuoteSignature(pub, hash, q.GetQuote(), sig); err != nil {
 			return err
 		}
 	default:
 		return fmt.Errorf("only RSA and ECC public keys are currently supported, received type: %T", pub)
-
 	}
 
 	// Decode and check for magic TPMS_GENERATED_VALUE.
@@ -72,6 +72,30 @@ func VerifyQuote(q *pb.Quote, trustedPub crypto.PublicKey, extraData []byte) err
 	return validatePCRDigest(attestedQuoteInfo, q.GetPcrs(), hash)
 }
 
+// Get the cryptographic hash used for the signature and make sure we support it
+func verifyHashAlg(sig *tpm2.Signature) (crypto.Hash, error) {
+	var hashAlg tpm2.Algorithm
+	if sig.ECC != nil {
+		hashAlg = sig.ECC.HashAlg
+	} else if sig.RSA != nil {
+		hashAlg = sig.RSA.HashAlg
+	} else {
+		return 0, fmt.Errorf("signature is missing hash algorithm")
+	}
+
+	// Convert from TPM2 hash algorithm to a Golang hash algorithm
+	hash, err := hashAlg.Hash()
+	if err != nil {
+		return 0, err
+	}
+	for _, alg := range SignatureHashAlgs {
+		if hashAlg == alg {
+			return hash, nil
+		}
+	}
+	return 0, fmt.Errorf("unsupported signature hash algorithm: %v", hash)
+}
+
 func verifyECDSAQuoteSignature(ecdsaPub *ecdsa.PublicKey, hash crypto.Hash, quoted []byte, sig *tpm2.Signature) error {
 	if sig.Alg != tpm2.AlgECDSA {
 		return fmt.Errorf("signature scheme 0x%x is not supported, only ECDSA is supported", sig.Alg)

server/verify.go

@@ -5,7 +5,6 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
-	"errors"
 	"fmt"
 
 	"github.com/google/go-tpm-tools/internal"
@@ -15,12 +14,10 @@ import (
 	"google.golang.org/protobuf/proto"
 )
 
-var oidExtensionSubjectAltName = []int{2, 5, 29, 17}
+// We conditinally support SHA-1 for PCR hashes, but at the lowest priority.
+var pcrHashAlgs = append(internal.SignatureHashAlgs, tpm2.AlgSHA1)
 
-// The hash algorithms we support, in their preferred order of use.
-var supportedHashAlgs = []tpm2.Algorithm{
-	tpm2.AlgSHA512, tpm2.AlgSHA384, tpm2.AlgSHA256, tpm2.AlgSHA1,
-}
+var oidExtensionSubjectAltName = []int{2, 5, 29, 17}
 
 var cloudComputeInstanceIdentifierOID asn1.ObjectIdentifier = []int{1, 3, 6, 1, 4, 1, 11129, 2, 1, 21}
 
@@ -32,11 +29,12 @@ type VerifyOpts struct {
 	// attestation. This option should be used if you already know the AK, as
 	// it provides the highest level of assurance.
 	TrustedAKs []crypto.PublicKey
-	// Allow attestations to be verified using SHA-1. This defaults to false
+	// Allow using SHA-1 PCRs to verify attestations. This defaults to false
 	// because SHA-1 is a weak hash algorithm with known collision attacks.
 	// However, setting this to true may be necessary if the client only
 	// supports the legacy event log format. This is the case on older Linux
-	// distributions (such as Debian 10).
+	// distributions (such as Debian 10). Note that this will NOT allow
+	// SHA-1 signatures to be used, just SHA-1 PCRs.
 	AllowSHA1 bool
 	// A collection of trusted root CAs that are used to sign AK certificates.
 	// The TrustedAKs are used first, followed by TrustRootCerts and
@@ -76,43 +74,50 @@ type gceInstanceInfo struct {
 // After this, the eventlog is parsed and the corresponding MachineState is
 // returned. This design prevents unverified MachineStates from being used.
 func VerifyAttestation(attestation *pb.Attestation, opts VerifyOpts) (*pb.MachineState, error) {
-	// Verify the AK
-	akPubArea, err := tpm2.DecodePublic(attestation.GetAkPub())
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode AK public area: %w", err)
-	}
-	akPubKey, err := akPubArea.Key()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get AK public key: %w", err)
+	if err := validateOpts(opts); err != nil {
+		return nil, fmt.Errorf("bad options: %w", err)
 	}
 
-	// Add intermediate certs in the attestation if they exist.
-	certs, err := parseCerts(attestation.IntermediateCerts)
-	if err != nil {
-		return nil, fmt.Errorf("attestation intermediates: %w", err)
-	}
-
-	opts.IntermediateCerts = append(opts.IntermediateCerts, certs...)
-
-	machineState, err := validateAK(akPubKey, attestation.GetAkCert(), opts)
-	if err != nil {
-		return nil, fmt.Errorf("failed to validate AK: %w", err)
-	}
+	var akPubKey crypto.PublicKey
+	machineState := &pb.MachineState{}
+	if len(attestation.GetAkCert()) == 0 {
+		// If the AK Cert is not in the attestation, use the AK Public Area.
+		akPubArea, err := tpm2.DecodePublic(attestation.GetAkPub())
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode AK public area: %w", err)
+		}
+		akPubKey, err = akPubArea.Key()
+		if err != nil {
+			return nil, fmt.Errorf("failed to get AK public key: %w", err)
+		}
+		if err := validateAKPub(akPubKey, opts); err != nil {
+			return nil, fmt.Errorf("failed to validate AK public key: %w", err)
+		}
+	} else {
+		// If AK Cert is presented, ignore the AK Public Area.
+		akCert, err := x509.ParseCertificate(attestation.GetAkCert())
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse AK certificate: %w", err)
+		}
+		// Use intermediate certs from the attestation if they exist.
+		certs, err := parseCerts(attestation.IntermediateCerts)
+		if err != nil {
+			return nil, fmt.Errorf("attestation intermediates: %w", err)
+		}
+		opts.IntermediateCerts = append(opts.IntermediateCerts, certs...)
 
-	// Verify the signing hash algorithm
-	signHashAlg, err := internal.GetSigningHashAlg(akPubArea)
-	if err != nil {
-		return nil, fmt.Errorf("bad AK public area: %w", err)
-	}
-	if err = checkHashAlgSupported(signHashAlg, opts); err != nil {
-		return nil, fmt.Errorf("in AK public area: %w", err)
+		machineState, err = validateAKCert(akCert, opts)
+		if err != nil {
+			return nil, fmt.Errorf("failed to validate AK certificate: %w", err)
+		}
+		akPubKey = akCert.PublicKey.(crypto.PublicKey)
 	}
 
 	// Attempt to replay the log against our PCRs in order of hash preference
 	var lastErr error
 	for _, quote := range supportedQuotes(attestation.GetQuotes()) {
 		// Verify the Quote
-		if err = internal.VerifyQuote(quote, akPubKey, opts.Nonce); err != nil {
+		if err := internal.VerifyQuote(quote, akPubKey, opts.Nonce); err != nil {
 			lastErr = fmt.Errorf("failed to verify quote: %w", err)
 			continue
 		}
@@ -135,9 +140,8 @@ func VerifyAttestation(attestation *pb.Attestation, opts VerifyOpts) (*pb.Machin
 		// the start of the loop) so that the user gets a "SHA-1 not supported"
 		// error only if allowing SHA-1 support would actually allow the log
 		// to be verified. This makes debugging failed verifications easier.
-		pcrHashAlg := tpm2.Algorithm(pcrs.GetHash())
-		if err = checkHashAlgSupported(pcrHashAlg, opts); err != nil {
-			lastErr = fmt.Errorf("when verifying PCRs: %w", err)
+		if !opts.AllowSHA1 && tpm2.Algorithm(pcrs.GetHash()) == tpm2.AlgSHA1 {
+			lastErr = fmt.Errorf("SHA-1 is not allowed for verification (set VerifyOpts.AllowSHA1 to true to allow)")
 			continue
 		}
 
@@ -191,38 +195,31 @@ func getInstanceInfo(extensions []pkix.Extension) (*pb.GCEInstanceInfo, error) {
 	}, nil
 }
 
-// Checks if the provided AK public key can be trusted
-func validateAK(ak crypto.PublicKey, akCertBytes []byte, opts VerifyOpts) (*pb.MachineState, error) {
+// Check that we are passing in a valid VerifyOpts structure
+func validateOpts(opts VerifyOpts) error {
 	checkPub := len(opts.TrustedAKs) > 0
-	checkCert := opts.TrustedRootCerts != nil
+	checkCert := len(opts.TrustedRootCerts) > 0
 	if !checkPub && !checkCert {
-		return nil, fmt.Errorf("no trust mechanism provided, either use TrustedAKs or TrustedRootCerts")
+		return fmt.Errorf("no trust mechanism provided, either use TrustedAKs or TrustedRootCerts")
 	}
 	if checkPub && checkCert {
-		return nil, fmt.Errorf("multiple trust mechanisms provided, only use one of TrustedAKs or TrustedRootCerts")
+		return fmt.Errorf("multiple trust mechanisms provided, only use one of TrustedAKs or TrustedRootCerts")
 	}
+	return nil
+}
 
-	// Check against known AKs
-	if checkPub {
-		for _, trusted := range opts.TrustedAKs {
-			if internal.PubKeysEqual(ak, trusted) {
-				return &pb.MachineState{}, nil
-			}
+func validateAKPub(ak crypto.PublicKey, opts VerifyOpts) error {
+	for _, trusted := range opts.TrustedAKs {
+		if internal.PubKeysEqual(ak, trusted) {
+			return nil
 		}
-		return nil, fmt.Errorf("public key is not trusted")
-	}
-
-	// Check if the AK Cert chains to a trusted root
-	if len(akCertBytes) == 0 {
-		return nil, errors.New("no certificate provided in attestation")
-	}
-	akCert, err := x509.ParseCertificate(akCertBytes)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse certificate: %w", err)
 	}
+	return fmt.Errorf("key not trusted")
+}
 
-	if !internal.PubKeysEqual(ak, akCert.PublicKey) {
-		return nil, fmt.Errorf("mismatch between public key and certificate")
+func validateAKCert(akCert *x509.Certificate, opts VerifyOpts) (*pb.MachineState, error) {
+	if len(opts.TrustedRootCerts) == 0 {
+		return nil, validateAKPub(akCert.PublicKey.(crypto.PublicKey), opts)
 	}
 
 	// We manually handle the SAN extension because x509 marks it unhandled if
@@ -248,7 +245,7 @@ func validateAK(ak crypto.PublicKey, akCertBytes []byte, opts VerifyOpts) (*pb.M
 		KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsage(x509.ExtKeyUsageAny)},
 	}
 	if _, err := akCert.Verify(x509Opts); err != nil {
-		return nil, fmt.Errorf("failed to verify certificate against trusted roots: %v", err)
+		return nil, fmt.Errorf("certificate did not chain to a trusted root: %v", err)
 	}
 
 	instanceInfo, err := getInstanceInfo(akCert.Extensions)
@@ -259,22 +256,10 @@ func validateAK(ak crypto.PublicKey, akCertBytes []byte, opts VerifyOpts) (*pb.M
 	return &pb.MachineState{Platform: &pb.PlatformState{InstanceInfo: instanceInfo}}, nil
 }
 
-func checkHashAlgSupported(hash tpm2.Algorithm, opts VerifyOpts) error {
-	if hash == tpm2.AlgSHA1 && !opts.AllowSHA1 {
-		return fmt.Errorf("SHA-1 is not allowed for verification (set VerifyOpts.AllowSHA1 to true to allow)")
-	}
-	for _, alg := range supportedHashAlgs {
-		if hash == alg {
-			return nil
-		}
-	}
-	return fmt.Errorf("unsupported hash algorithm: %v", hash)
-}
-
-// Retrieve the supported quotes in order of hash preference
+// Retrieve the supported quotes in order of hash preference.
 func supportedQuotes(quotes []*tpmpb.Quote) []*tpmpb.Quote {
 	out := make([]*tpmpb.Quote, 0, len(quotes))
-	for _, alg := range supportedHashAlgs {
+	for _, alg := range pcrHashAlgs {
 		for _, quote := range quotes {
 			if tpm2.Algorithm(quote.GetPcrs().GetHash()) == alg {
 				out = append(out, quote)

server/verify_test.go

@@ -570,8 +570,8 @@ func TestVerifyAttestationMissingIntermediates(t *testing.T) {
 	}
 }
 
-func TestVerifyMismatchedAKPubAndAKCert(t *testing.T) {
-	// Make sure that we fail verification if the AKPub and AKCert don't match
+func TestVerifyIgnoreAKPubWithAKCert(t *testing.T) {
+	// Make sure that we ignore the AKPub if the AKCert is presented
 	rwc := test.GetTPM(t)
 	defer client.CheckedClose(t, rwc)
 
@@ -599,7 +599,7 @@ func TestVerifyMismatchedAKPubAndAKCert(t *testing.T) {
 		IntermediateCerts: GceEKIntermediates,
 	}
 	if _, err := VerifyAttestation(badAtt, opts); err == nil {
-		t.Error("expected error when calling VerifyAttestation with mismatched public key and cert")
+		t.Error("expected error when calling VerifyAttestation, because the cert is replaced")
 	}
 }