Cosign Certificate Chain Expiry Validation Issue Allows Issuing Certificate Expiry to Be Overlooked

@@ -333,16 +333,18 @@ func verifyOCISignature(ctx context.Context, verifier signature.Verifier, sig pa
 // certificate chains up to a trusted root using intermediate certificate chain coming from CheckOpts.
 // Optionally verifies the subject and issuer of the certificate.
 func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Verifier, error) {
-	return ValidateAndUnpackCertWithIntermediates(cert, co, co.IntermediateCerts)
+	verifier, _, err := ValidateAndUnpackCertWithIntermediates(cert, co, co.IntermediateCerts)
+	return verifier, err
 }
 
 // ValidateAndUnpackCertWithIntermediates creates a Verifier from a certificate. Verifies that the
 // certificate chains up to a trusted root using intermediate cert passed as separate argument.
-// Optionally verifies the subject and issuer of the certificate.
-func ValidateAndUnpackCertWithIntermediates(cert *x509.Certificate, co *CheckOpts, intermediateCerts *x509.CertPool) (signature.Verifier, error) {
+// Optionally verifies the subject and issuer of the certificate. Returns the chain built from the
+// certificate pools. Clients must verify the validity of this chain against a provided timestamp.
+func ValidateAndUnpackCertWithIntermediates(cert *x509.Certificate, co *CheckOpts, intermediateCerts *x509.CertPool) (signature.Verifier, []*x509.Certificate, error) {
 	verifier, err := signature.LoadVerifier(cert.PublicKey, crypto.SHA256)
 	if err != nil {
-		return nil, fmt.Errorf("invalid certificate found on signature: %w", err)
+		return nil, nil, fmt.Errorf("invalid certificate found on signature: %w", err)
 	}
 
 	// Handle certificates where the Subject Alternative Name is not set to a supported
@@ -365,70 +367,71 @@ func ValidateAndUnpackCertWithIntermediates(cert *x509.Certificate, co *CheckOpt
 	var chains [][]*x509.Certificate
 	if co.TrustedMaterial != nil {
 		if chains, err = verify.VerifyLeafCertificate(cert.NotBefore, cert, co.TrustedMaterial); err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	} else {
 		// If the trusted root is not available, use the verifiers from cosign (legacy).
 		chains, err = TrustedCert(cert, co.RootCerts, intermediateCerts)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 
+	// handle if chains has more than one chain - grab first and print message
+	if len(chains) > 1 {
+		fmt.Fprintf(os.Stderr, "**Info** Multiple valid certificate chains found. Selecting the first for further verification.\n")
+	}
+	chain := chains[0]
+
 	err = CheckCertificatePolicy(cert, co)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// If IgnoreSCT is set, skip the SCT check
 	if co.IgnoreSCT {
-		return verifier, nil
+		return verifier, chains[0], nil
 	}
 	contains, err := ContainsSCT(cert.Raw)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	if !contains && len(co.SCT) == 0 {
-		return nil, &VerificationFailure{
+		return nil, nil, &VerificationFailure{
 			fmt.Errorf("certificate does not include required embedded SCT and no detached SCT was set"),
 		}
 	}
 
 	// If trusted root is available and the SCT is embedded, use the verifiers from sigstore-go (preferred).
 	if co.TrustedMaterial != nil && contains {
 		if err := verify.VerifySignedCertificateTimestamp(chains, 1, co.TrustedMaterial); err != nil {
-			return nil, err
+			return nil, nil, err
 		}
-		return verifier, nil
+		return verifier, chain, nil
 	}
 
-	// handle if chains has more than one chain - grab first and print message
-	if len(chains) > 1 {
-		fmt.Fprintf(os.Stderr, "**Info** Multiple valid certificate chains found. Selecting the first to verify the SCT.\n")
+	if len(chain) < 2 {
+		return nil, nil, errors.New("certificate chain must contain at least a certificate and its issuer")
 	}
 	if contains {
-		if err := VerifyEmbeddedSCT(context.Background(), chains[0], co.CTLogPubKeys); err != nil {
-			return nil, err
+		if err := VerifyEmbeddedSCT(context.Background(), chain, co.CTLogPubKeys); err != nil {
+			return nil, nil, err
 		}
-		return verifier, nil
-	}
-	chain := chains[0]
-	if len(chain) < 2 {
-		return nil, errors.New("certificate chain must contain at least a certificate and its issuer")
+		return verifier, chain, nil
 	}
 	certPEM, err := cryptoutils.MarshalCertificateToPEM(chain[0])
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	chainPEM, err := cryptoutils.MarshalCertificatesToPEM(chain[1:])
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	if err := VerifySCT(context.Background(), certPEM, chainPEM, co.SCT, co.CTLogPubKeys); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return verifier, nil
+	return verifier, chain, nil
 }
 
 // CheckCertificatePolicy checks that the certificate subject and issuer match
@@ -852,6 +855,7 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
 	}
 
 	verifier := co.SigVerifier
+	var verifierChain []*x509.Certificate
 	if verifier == nil {
 		// If we don't have a public key to check against, we can try a root cert.
 		cert, err := sig.Cert()
@@ -883,10 +887,12 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
 		if pool == nil {
 			pool = co.IntermediateCerts
 		}
-		verifier, err = ValidateAndUnpackCertWithIntermediates(cert, co, pool)
+		verifier, chain, err = ValidateAndUnpackCertWithIntermediates(cert, co, pool)
 		if err != nil {
 			return false, err
 		}
+		// Remove the end-entity certificate from the chain, as that will come from sig.Cert()
+		verifierChain = chain[1:]
 	}
 
 	// 1. Perform cryptographic verification of the signature using the certificate's public key.
@@ -912,22 +918,22 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
 
 		if acceptableRFC3161Time != nil {
 			// Verify the cert against the timestamp time.
-			if err := CheckExpiry(cert, *acceptableRFC3161Time); err != nil {
+			if err := CheckExpiry(cert, verifierChain, *acceptableRFC3161Time); err != nil {
 				return false, fmt.Errorf("checking expiry on certificate with timestamp: %w", err)
 			}
 			expirationChecked = true
 		}
 
 		if acceptableRekorBundleTime != nil {
-			if err := CheckExpiry(cert, *acceptableRekorBundleTime); err != nil {
+			if err := CheckExpiry(cert, verifierChain, *acceptableRekorBundleTime); err != nil {
 				return false, fmt.Errorf("checking expiry on certificate with bundle: %w", err)
 			}
 			expirationChecked = true
 		}
 
 		// if no timestamp has been provided, use the current time
 		if !expirationChecked {
-			if err := CheckExpiry(cert, time.Now()); err != nil {
+			if err := CheckExpiry(cert, verifierChain, time.Now()); err != nil {
 				// If certificate is expired and not signed timestamp was provided then error the following message. Otherwise throw an expiration error.
 				if co.IgnoreTlog && acceptableRFC3161Time == nil {
 					return false, &VerificationFailure{
@@ -1176,21 +1182,36 @@ func VerifyImageAttestation(ctx context.Context, atts oci.Signatures, h v1.Hash,
 	return checkedAttestations, bundleVerified, nil
 }
 
-// CheckExpiry confirms the time provided is within the valid period of the cert
-func CheckExpiry(cert *x509.Certificate, it time.Time) error {
+// CheckExpiry confirms the time provided is within the valid period of the certificate and optionally
+// all issuing CA certificates.
+func CheckExpiry(cert *x509.Certificate, issuingChain []*x509.Certificate, it time.Time) error {
 	ft := func(t time.Time) string {
 		return t.Format(time.RFC3339)
 	}
 	if cert.NotAfter.Before(it) {
 		return &VerificationFailure{
-			fmt.Errorf("certificate expired before signatures were entered in log: %s is before %s",
+			fmt.Errorf("certificate expired before observed time: %s is before %s",
 				ft(cert.NotAfter), ft(it)),
 		}
 	}
 	if cert.NotBefore.After(it) {
 		return &VerificationFailure{
-			fmt.Errorf("certificate was issued after signatures were entered in log: %s is after %s",
-				ft(cert.NotAfter), ft(it)),
+			fmt.Errorf("certificate was issued after observed time: %s is after %s",
+				ft(cert.NotBefore), ft(it)),
+		}
+	}
+	for _, c := range issuingChain {
+		if c.NotAfter.Before(it) {
+			return &VerificationFailure{
+				fmt.Errorf("issuing CA certificate expired before observed time: %s is before %s",
+					ft(c.NotAfter), ft(it)),
+			}
+		}
+		if c.NotBefore.After(it) {
+			return &VerificationFailure{
+				fmt.Errorf("issuing CA certificate was issued after observed time: %s is after %s",
+					ft(c.NotBefore), ft(it)),
+			}
 		}
 	}
 	return nil

@@ -25,13 +25,15 @@ import (
 	"crypto/sha256"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/asn1"
 	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"encoding/pem"
 	"errors"
 	"fmt"
 	"io"
+	"math/big"
 	"net"
 	"net/url"
 	"os"
@@ -1550,10 +1552,13 @@ func TestValidateAndUnpackCertWithIntermediatesSuccess(t *testing.T) {
 		Identities: []Identity{{Subject: subject, Issuer: oidcIssuer}},
 	}
 
-	_, err := ValidateAndUnpackCertWithIntermediates(leafCert, co, subPool)
+	_, chain, err := ValidateAndUnpackCertWithIntermediates(leafCert, co, subPool)
 	if err != nil {
 		t.Errorf("ValidateAndUnpackCertWithIntermediates expected no error, got err = %v", err)
 	}
+	if len(chain) == 0 {
+		t.Errorf("expected certificate chain")
+	}
 	err = CheckCertificatePolicy(leafCert, co)
 	if err != nil {
 		t.Errorf("CheckCertificatePolicy expected no error, got err = %v", err)
@@ -1766,7 +1771,7 @@ func TestVerifyRFC3161Timestamp(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error verifying timestamp with signature: %v", err)
 	}
-	if err := CheckExpiry(leafCert, ts.Time); err != nil {
+	if err := CheckExpiry(leafCert, nil, ts.Time); err != nil {
 		t.Fatalf("unexpected error using time from timestamp to verify certificate: %v", err)
 	}
 
@@ -1834,6 +1839,170 @@ func TestVerifyRFC3161Timestamp(t *testing.T) {
 	}
 }
 
+// This test verifies that artifact verification rejects signatures
+// where a CA certificate in the issuing chain is expired. This is a contrived
+// example because CAs shouldn't issue certificates where the leaf's validity
+// outlives any certificate in the chain, but this is checked for thoroughness.
+func TestVerifyImageSignatureExpiredCACertificate(t *testing.T) {
+	now := time.Now().UTC()
+
+	rootCert, rootKey, _ := test.GenerateRootCa() // Valid +/- 5 hours
+
+	subTemplate := &x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject: pkix.Name{
+			CommonName:   "sigstore-sub-expired",
+			Organization: []string{"sigstore.dev"},
+		},
+		NotBefore:             now.Add(-2 * time.Hour), // Valid during root validity
+		NotAfter:              now.Add(-5 * time.Minute),
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning},
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+	}
+	subKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating subordinate key: %v", err)
+	}
+	subBytes, err := x509.CreateCertificate(rand.Reader, subTemplate, rootCert, &subKey.PublicKey, rootKey)
+	if err != nil {
+		t.Fatalf("creating subordinate cert: %v", err)
+	}
+	subCert, err := x509.ParseCertificate(subBytes)
+	if err != nil {
+		t.Fatalf("parsing subordinate cert: %v", err)
+	}
+
+	leafTemplate := &x509.Certificate{
+		SerialNumber:   big.NewInt(2),
+		EmailAddresses: []string{"subject@mail.com"},
+		NotBefore:      now.Add(-30 * time.Minute), // Valid during intermediate...
+		NotAfter:       now.Add(30 * time.Minute),  // but is valid past intermediate expiration
+		KeyUsage:       x509.KeyUsageDigitalSignature,
+		ExtKeyUsage:    []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning},
+		IsCA:           false,
+		ExtraExtensions: []pkix.Extension{{
+			Id:       asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 1},
+			Critical: false,
+			Value:    []byte("oidc-issuer"),
+		}},
+	}
+	leafKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating leaf key: %v", err)
+	}
+	leafBytes, err := x509.CreateCertificate(rand.Reader, leafTemplate, subCert, &leafKey.PublicKey, subKey)
+	if err != nil {
+		t.Fatalf("creating leaf cert: %v", err)
+	}
+	leafCert, err := x509.ParseCertificate(leafBytes)
+	if err != nil {
+		t.Fatalf("parsing leaf cert: %v", err)
+	}
+	pemRoot := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: rootCert.Raw})
+	pemSub := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: subCert.Raw})
+	pemLeaf := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: leafCert.Raw})
+
+	rootPool := x509.NewCertPool()
+	rootPool.AddCert(rootCert)
+
+	payload := []byte{1, 2, 3, 4}
+	h := sha256.Sum256(payload)
+	sigBytes, _ := leafKey.Sign(rand.Reader, h[:], crypto.SHA256)
+
+	ociSig, _ := static.NewSignature(payload,
+		base64.StdEncoding.EncodeToString(sigBytes),
+		static.WithCertChain(pemLeaf, appendSlices([][]byte{pemSub, pemRoot})))
+
+	co := &CheckOpts{
+		RootCerts:  rootPool,
+		IgnoreSCT:  true,
+		IgnoreTlog: true,
+		Identities: []Identity{{Subject: "subject@mail.com", Issuer: "oidc-issuer"}},
+	}
+
+	// Verify expected failure, where the current time is used
+	_, err = VerifyImageSignature(context.TODO(), ociSig, v1.Hash{}, co)
+	if err == nil {
+		t.Fatalf("expected error verifying signature with expired intermediate")
+	}
+	var vf *VerificationFailure
+	if !errors.As(err, &vf) {
+		t.Fatalf("expected %T, got %T (%v)", &VerificationFailure{}, err, err)
+	}
+
+	// Verify expected failure with time provided by a signed timestamp
+	client, err := tsaMock.NewTSAClient((tsaMock.TSAClientOptions{Time: time.Now()}))
+	if err != nil {
+		t.Fatal(err)
+	}
+	tsBytes, err := tsa.GetTimestampedSignature(payload, client)
+	if err != nil {
+		t.Fatalf("unexpected error creating timestamp: %v", err)
+	}
+	rfc3161TS := bundle.RFC3161Timestamp{SignedRFC3161Timestamp: tsBytes}
+
+	certChainPEM, err := cryptoutils.MarshalCertificatesToPEM(client.CertChain)
+	if err != nil {
+		t.Fatalf("unexpected error marshalling cert chain: %v", err)
+	}
+
+	leaves, intermediates, roots, err := tsa.SplitPEMCertificateChain(certChainPEM)
+	if err != nil {
+		t.Fatal("error splitting response into certificate chain")
+	}
+	co.TSACertificate = leaves[0]
+	co.TSAIntermediateCertificates = intermediates
+	co.TSARootCertificates = roots
+
+	ociSig, _ = static.NewSignature(payload,
+		base64.StdEncoding.EncodeToString(sigBytes),
+		static.WithCertChain(pemLeaf, appendSlices([][]byte{pemSub, pemRoot})),
+		static.WithRFC3161Timestamp(&rfc3161TS))
+	_, err = VerifyImageSignature(context.TODO(), ociSig, v1.Hash{}, co)
+	if err == nil {
+		t.Fatalf("expected error verifying signature with expired intermediate")
+	}
+	if !errors.As(err, &vf) {
+		t.Fatalf("expected %T, got %T (%v)", &VerificationFailure{}, err, err)
+	}
+
+	// Verify expected failure where the chain is provided via trusted material
+	// rather than bundled with the image
+	tm := &trustedMaterialWithFulcioCAs{
+		cas: []root.CertificateAuthority{
+			&root.FulcioCertificateAuthority{
+				Root:          rootCert,
+				Intermediates: []*x509.Certificate{subCert},
+			},
+		},
+	}
+	co.TrustedMaterial = tm
+	co.RootCerts = nil
+
+	ociSig, _ = static.NewSignature(payload,
+		base64.StdEncoding.EncodeToString(sigBytes),
+		static.WithCertChain(pemLeaf, appendSlices([][]byte{})),
+		static.WithRFC3161Timestamp(&rfc3161TS))
+	_, err = VerifyImageSignature(context.TODO(), ociSig, v1.Hash{}, co)
+	if err == nil {
+		t.Fatalf("expected error verifying signature with expired intermediate")
+	}
+	if !errors.As(err, &vf) {
+		t.Fatalf("expected %T, got %T (%v)", &VerificationFailure{}, err, err)
+	}
+}
+
+type trustedMaterialWithFulcioCAs struct {
+	root.BaseTrustedMaterial
+	cas []root.CertificateAuthority
+}
+
+func (tm *trustedMaterialWithFulcioCAs) FulcioCertificateAuthorities() []root.CertificateAuthority {
+	return tm.cas
+}
+
 func TestVerifyImageAttestation(t *testing.T) {
 	if _, _, err := VerifyImageAttestation(context.TODO(), nil, v1.Hash{}, nil); err == nil {
 		t.Error("VerifyImageAttestation() should error when given nil attestations")