cosign verify fails randomly when we have multiple signatures attached to image

[Mukuls77]

Description

Use case impacted: An organization uses cosign to sign the software images using its own PKI infrastructure using its Root Certificate, Intermediate Certificate and Leaf Certificate.
The software is delivered to the client. Now client want to apply its own signatures using client PKI infra using clients Root cert, Intermediate cert and Leaf cert before client can use the software.
So once client apply its signature we have now two signatures attached to the image.
Signature 1: Is signed with cert chain CompanyRootCA.crt -> CompanyIntermediate.crt -> CompanyLeaf.crt
Signature 2: Is signed with cert chain ClientRootCA.crt -> ClientIntermediate.crt -> ClientLeaf.crt

Now if we verify the image using cosign verify using Root certificate in the certChain argument the verification fails.

Like in the above case if we verify the image using Company root cert or Client root cert the verification fails some time and sometime it get success.

cosign verify ttl.sh/cosign-ci/tarball/5772530a@sha256:1d4b70ca7693a3357edb48d492bee7c4aedeb6e7a74557fd47871a81d507bfdc --certificate-identity-regexp='.' --certificate-oidc-issuer-regexp='.' --insecure-ignore-sct --insecure-ignore-tlog --cert-chain=CompanyRootCA.crt

After analysis of the code following root cause was found.
In this case as we are just passing rootCA.crt in the certChain argument following thing will happen.

showing the function stack for the issue flow

1. cli/verify/verify.go (cosign.VerifyImageSignatures) will be called

    	verified, bundleVerified, err := cosign.VerifyImageSignatures(ctx, ref, co)
    	if err != nil {
    		return cosignError.WrapError(err)
    	}
   

cosign/cmd/cosign/cli/verify/verify.go

Line 305 in 402820d

verified, bundleVerified, err := cosign.VerifyImageSignatures(ctx, ref, co)

2. pkg/cosign/verify.go (VerifyImageSignatures)
   this will call verifySignatures function
   return verifySignatures(ctx, sigs, h, co)
   https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L527C2-L527C43

3. pkg/cosign/verify.go (verifySignatures)
   in this function multiple worker threads are spawned to verify signatures. The same checkOps reference is now available with each worker thread.
   t := throttler.New(workers, len(sl))
   for i, sig := range sl {
   go func(sig oci.Signature, index int) {
   sig, err := static.Copy(sig)
   if err != nil {
   t.Done(err)
   return
   }

    	verified, err := VerifyImageSignature(ctx, sig, h, co)
    	bundlesVerified[index] = verified
    	if err != nil {
    		t.Done(err)
    		return
    	}
    	signatures[index] = sig
   
    	t.Done(nil)
    }(sig, i)
   
    // wait till workers are available
    t.Throttle()
   

   }

https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L598C1-L620C3

4. pkg/cosign/verify.go (VerifyImageSignature)
   // VerifyImageSignature verifies a signature
   func VerifyImageSignature(ctx context.Context, sig oci.Signature, h v1.Hash, co *CheckOpts) (bundleVerified bool, err error) {
   return verifyInternal(ctx, sig, h, verifyOCISignature, co)
   }
   https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L817C1-L820C2

5. pkg/cosign/verify.go (verifyInternal)
   in this function if co.IntermediateCerts is NULL as we have only passed the root certificate only. So each thread update
   co.IntermediateCerts with intermediateCert pool fetched from signature it is verifying. The problem is that this can be set by any
   of the threads which get scheduled and than the remaining thread will not set it again as they will notice the intermediates have
   been set already. this will can corrupt the verification chain if different signatures have different intermediates.

    // If there is no chain annotation present, we preserve the pools set in the CheckOpts.
    if len(chain) > 0 {
    	if len(chain) == 1 {
    		co.IntermediateCerts = nil
    	} else if co.IntermediateCerts == nil {
    		// If the intermediate certs have not been loaded in by TUF
    		pool := x509.NewCertPool()
    		for _, cert := range chain[:len(chain)-1] {
    			pool.AddCert(cert)
    		}
    		co.IntermediateCerts = pool
    	}
    }
    verifier, err = ValidateAndUnpackCert(cert, co)
    if err != nil {
    	return false, err
    }
   

https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L723C1-L739C4
6. pkg/cosign/verify.go (ValidateAndUnpackCert)
chains, err := TrustedCert(cert, co.RootCerts, co.IntermediateCerts)
if err != nil {
return nil, err
}
https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L242C1-L245C3
7. pkg/cosign/verify.go (TrustedCert)
this function fails some time as the parallel thread which ever get first change to set the co.Intermediatecerts can set it own signature Intermediate certs which may not match to the rootCert passed in the CLI argument and due to which all verification fails.

func TrustedCert(cert *x509.Certificate, roots *x509.CertPool, intermediates *x509.CertPool) ([][]*x509.Certificate, error) {
chains, err := cert.Verify(x509.VerifyOptions{
// THIS IS IMPORTANT: WE DO NOT CHECK TIMES HERE
// THE CERTIFICATE IS TREATED AS TRUSTED FOREVER
// WE CHECK THAT THE SIGNATURES WERE CREATED DURING THIS WINDOW
CurrentTime: cert.NotBefore,
Roots: roots,
Intermediates: intermediates,
KeyUsages: []x509.ExtKeyUsage{
x509.ExtKeyUsageCodeSigning,
},
})
if err != nil {
return nil, fmt.Errorf("cert verification failed: %w. Check your TUF root (see cosign initialize) or set a custom root with env var SIGSTORE_ROOT_FILE", err)
}
return chains, nil
}

https://github.com/sigstore/cosign/blob/402820d97997f3a29f06b99c6fc7b7d78067ce62/pkg/cosign/verify.go#L1319C1-L1335C2

So main issue is that
function verifySignatures:

cosign/pkg/cosign/verify.go

Line 579 in 402820d

func verifySignatures(ctx context.Context, sigs oci.Signatures, h v1.Hash, co *CheckOpts) (checkedSignatures []oci.Signature, bundleVerified bool, err error) {

invokes parallel thread but passes a common checkOpts pointer to each thread.
Now in the given case where co.IntermediateCerts is Null each thread try to set the Intermediate certs from the signature it is handling. This can corrupt the verification chain.

Solution: I am proposing following solution to the issue.

changes in func verifyInternal:
instead of modifying the co.intermediatecerts we should use a local variable of *x509.CertPool to fetch the Intermediate certs from Signature and pass this as a separate argument to function ValidateAndUnpackCert

Change in func ValidateAndUnpackCert we should pass Intermediatecerts fetched from Signature as separate argument and use it if co.intermediatecerts is nil.

Version

Issue is visible in 2.2.2 release and also in previous releases

[Mukuls77]

@haydentherapper if you agree with the bug than i can propose a PR to fix it.
following are the changes i propose.
Function : ValidateAndUnpackCert

1. we should pass the intermediate cert as a separate argument so new signature of the function is
   func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts, intermediateCerts *x509.CertPool) (signature.Verifier, error) {
2. if the new argument passed is not null than we use it else we use co.intermediatecert only
   if intermediateCerts == nil {
   intermediateCerts = co.IntermediateCerts
   }
   chains, err := TrustedCert(cert, co.RootCerts, intermediateCerts)

Function: verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,

1. we remove the logic of setting the co.intermediatecerts but instead pass the pool as separate argument
   so new logic is mentioned as follows

           var pool = (*x509.CertPool)(nil)
            if len(chain) > 0 {
                    if len(chain) == 1 {
                            co.IntermediateCerts = nil
                    } else if co.IntermediateCerts == nil {
                            // If the intermediate certs have not been loaded in by TUF
                            pool = x509.NewCertPool()
                            for _, cert := range chain[:len(chain)-1] {
                                    pool.AddCert(cert)
                            }
                    }
            }
            verifier, err = ValidateAndUnpackCert(cert, co, pool)
            if err != nil {
                    return false, err
            }
   

since the signature of function ValidateAndUnpackCert is changed following files need modification

cmd/cosign/cli/verify/verify_attestation.go
cmd/cosign/cli/verify/verify.go
pkg/cosign/verify_sct_test.go
pkg/cosign/verify_test.go

by passing new argument as nill
err = cosign.ValidateAndUnpackCert(cert, co, nil)

i have done local testing with these changes and they seems to work.

[haydentherapper]

Sorry for the delayed response. Nice catch on this bug! Just to make sure I understand, summarizing the issue: We're passing a shared CheckOpts object that is mutated by a downstream function. This object is used in parallel invocations, so one invocation can overwrite another's changes.

I'd like to avoid breaking changes to the API. Looking around GitHub, this function is called in a number of libraries. Can we instead create another function, ValidateAndUnpackCertWithIntermediates, that has your proposed API? This function will then be called by verifyInternal. We can also modify ValidateAndUnpackCert to call this new function by passing in co.IntermediateCerts (this is to reduce duplicated code while preserving the public API).

Note, I did look at making a deep copy of CheckOpts, but this is not straightforward in Golang. Another option would be to create another options struct, but this is a riskier change because there are so many places that the CheckOpts struct is used downstream of ValidateAndUnpackCerts.

[haydentherapper]

One other thing to note, the CheckOpts object was not designed to be thread-safe, even if it's only used in a read-only way, so I'm not sure if there's other bugs. Worth following up.

[Mukuls77]

Hi @haydentherapper yes your understanding is correct on the issue shared the CheckOpts is not thread safe and in the given flow it get mulated by downstream function.
i agree with your proposal to create a separate function ValidateAndUnpackCertWithIntermediates having intermediatecerts as a separate argument.
your second suggestion to call this new function inside ValidateAndUnpackCert will not be possible as because we dont have signature reference passed as argument to this function so we would not be able to fetch intermediate certs inside ValidateAndUnpackCert to pass to ValidateAndUnpackCertWithIntermediates

so i think we should create a new function ValidateAndUnpackCertWithIntermediates and call it inside verifyInternal this should solve the issue.
i will proceed ahead with creating a PR based on these suggestions. thanks.

[Mukuls77]

@haydentherapper i have done the changes as discussed before and tested it on my local setup. But when i try to push the changes to cosign git repository i am getting following error

$ git push -u origin Fix-Issue-3476
remote: Permission to sigstore/cosign.git denied to Mukuls77.
fatal: unable to access 'https://github.com/sigstore/cosign.git/': The requested URL returned error: 403

I have tried the steps from github page and stackexchange to resolve this issue like

1. clearing the old stored copy of credential in windows credential manager
2. creating a new ssh key adding it to my git account.
3. creating a fresh clone and doing the changes.

but when i try to push the changes git ask for authentication using browser, i give the credentials in browser it become success in browser but still my git push fails.
can you pls guide what i am missing here.

[haydentherapper]

Are your remotes set up properly? Look at git remote -v and confirm that you are pushing to your fork, not sigstore/cosign. Otherwise I'm not sure, this is an issue with your git setup.

[Mukuls77]

@haydentherapper many thanks it was the issue i corrected the git remote to my fork and it worked now i will proceed with the pull request.

[Mukuls77]

@haydentherapper i have created the PR to fix this issue
#3486
kindly review