Improper Certificate Validation in Cosign

@@ -127,6 +127,13 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e
 		}
 	}
 
+	// NB: There are only 2 kinds of verification right now:
+	// 1. You gave us the public key explicitly to verify against so co.SigVerifier is non-nil or,
+	// 2. We're going to find an x509 certificate on the signature and verify against Fulcio root trust
+	// TODO(nsmith5): Refactor this verification logic to pass back _how_ verification
+	// was performed so we don't need to use this fragile logic here.
+	fulcioVerified := (co.SigVerifier == nil)
+
 	for _, imageRef := range images {
 		var verified []oci.Signature
 		var bundleVerified bool
@@ -267,7 +274,7 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e
 		}
 
 		// TODO: add CUE validation report to `PrintVerificationHeader`.
-		PrintVerificationHeader(imageRef, co, bundleVerified)
+		PrintVerificationHeader(imageRef, co, bundleVerified, fulcioVerified)
 		// The attestations are always JSON, so use the raw "text" mode for outputting them instead of conversion
 		PrintVerification(imageRef, verified, "text")
 	}

@@ -146,13 +146,20 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
 	}
 	co.SigVerifier = pubKey
 
+	// NB: There are only 2 kinds of verification right now:
+	// 1. You gave us the public key explicitly to verify against so co.SigVerifier is non-nil or,
+	// 2. We're going to find an x509 certificate on the signature and verify against Fulcio root trust
+	// TODO(nsmith5): Refactor this verification logic to pass back _how_ verification
+	// was performed so we don't need to use this fragile logic here.
+	fulcioVerified := (co.SigVerifier == nil)
+
 	for _, img := range images {
 		if c.LocalImage {
 			verified, bundleVerified, err := cosign.VerifyLocalImageSignatures(ctx, img, co)
 			if err != nil {
 				return err
 			}
-			PrintVerificationHeader(img, co, bundleVerified)
+			PrintVerificationHeader(img, co, bundleVerified, fulcioVerified)
 			PrintVerification(img, verified, c.Output)
 		} else {
 			ref, err := name.ParseReference(img)
@@ -169,15 +176,15 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
 				return err
 			}
 
-			PrintVerificationHeader(ref.Name(), co, bundleVerified)
+			PrintVerificationHeader(ref.Name(), co, bundleVerified, fulcioVerified)
 			PrintVerification(ref.Name(), verified, c.Output)
 		}
 	}
 
 	return nil
 }
 
-func PrintVerificationHeader(imgRef string, co *cosign.CheckOpts, bundleVerified bool) {
+func PrintVerificationHeader(imgRef string, co *cosign.CheckOpts, bundleVerified, fulcioVerified bool) {
 	fmt.Fprintf(os.Stderr, "\nVerification for %s --\n", imgRef)
 	fmt.Fprintln(os.Stderr, "The following checks were performed on each of these signatures:")
 	if co.ClaimVerifier != nil {
@@ -195,7 +202,9 @@ func PrintVerificationHeader(imgRef string, co *cosign.CheckOpts, bundleVerified
 	if co.SigVerifier != nil {
 		fmt.Fprintln(os.Stderr, "  - The signatures were verified against the specified public key")
 	}
-	fmt.Fprintln(os.Stderr, "  - Any certificates were verified against the Fulcio roots.")
+	if fulcioVerified {
+		fmt.Fprintln(os.Stderr, "  - Any certificates were verified against the Fulcio roots.")
+	}
 }
 
 // PrintVerification logs details about the verification to stdout

@@ -586,6 +586,10 @@ func VerifyBundle(ctx context.Context, sig oci.Signature) (bool, error) {
 		return false, nil
 	}
 
+	if err := compareSigs(bundle.Payload.Body.(string), sig); err != nil {
+		return false, err
+	}
+
 	publicKeys, err := GetRekorPubs(ctx)
 	if err != nil {
 		return false, errors.Wrap(err, "retrieving rekor public key")
@@ -637,6 +641,31 @@ func VerifyBundle(ctx context.Context, sig oci.Signature) (bool, error) {
 	return true, nil
 }
 
+// compare bundle signature to the signature we are verifying
+func compareSigs(bundleBody string, sig oci.Signature) error {
+	// TODO(nsmith5): modify function signature to make it more clear _why_
+	// we've returned nil (there are several reasons possible here).
+	actualSig, err := sig.Base64Signature()
+	if err != nil {
+		return errors.Wrap(err, "base64 signature")
+	}
+	if actualSig == "" {
+		// NB: empty sig means this is an attestation
+		return nil
+	}
+	bundleSignature, err := bundleSig(bundleBody)
+	if err != nil {
+		return errors.Wrap(err, "failed to extract signature from bundle")
+	}
+	if bundleSignature == "" {
+		return nil
+	}
+	if bundleSignature != actualSig {
+		return fmt.Errorf("signature in bundle does not match signature being verified")
+	}
+	return nil
+}
+
 func bundleHash(bundleBody, signature string) (string, string, error) {
 	var toto models.Intoto
 	var rekord models.Rekord
@@ -698,6 +727,44 @@ func bundleHash(bundleBody, signature string) (string, string, error) {
 	return *hrekordObj.Data.Hash.Algorithm, *hrekordObj.Data.Hash.Value, nil
 }
 
+// bundleSig extracts the signature from the rekor bundle body
+func bundleSig(bundleBody string) (string, error) {
+	var rekord models.Rekord
+	var hrekord models.Hashedrekord
+	var rekordObj models.RekordV001Schema
+	var hrekordObj models.HashedrekordV001Schema
+
+	bodyDecoded, err := base64.StdEncoding.DecodeString(bundleBody)
+	if err != nil {
+		return "", errors.Wrap(err, "decoding bundleBody")
+	}
+
+	// Try Rekord
+	if err := json.Unmarshal(bodyDecoded, &rekord); err == nil {
+		specMarshal, err := json.Marshal(rekord.Spec)
+		if err != nil {
+			return "", err
+		}
+		if err := json.Unmarshal(specMarshal, &rekordObj); err != nil {
+			return "", err
+		}
+		return rekordObj.Signature.Content.String(), nil
+	}
+
+	// Try hashedRekordObj
+	if err := json.Unmarshal(bodyDecoded, &hrekord); err != nil {
+		return "", err
+	}
+	specMarshal, err := json.Marshal(hrekord.Spec)
+	if err != nil {
+		return "", err
+	}
+	if err := json.Unmarshal(specMarshal, &hrekordObj); err != nil {
+		return "", err
+	}
+	return hrekordObj.Signature.Content.String(), nil
+}
+
 func VerifySET(bundlePayload cbundle.RekorPayload, signature []byte, pub *ecdsa.PublicKey) error {
 	contents, err := json.Marshal(bundlePayload)
 	if err != nil {

@@ -26,6 +26,7 @@ import (
 	"github.com/in-toto/in-toto-golang/in_toto"
 	"github.com/pkg/errors"
 	"github.com/secure-systems-lab/go-securesystemslib/dsse"
+	"github.com/sigstore/cosign/pkg/oci/static"
 	"github.com/sigstore/cosign/pkg/types"
 	"github.com/sigstore/cosign/test"
 	"github.com/sigstore/sigstore/pkg/signature"
@@ -196,3 +197,40 @@ func TestValidateAndUnpackCertInvalidEmail(t *testing.T) {
 	_, err := ValidateAndUnpackCert(leafCert, co)
 	require.Contains(t, err.Error(), "expected email not found in certificate")
 }
+
+func TestCompareSigs(t *testing.T) {
+	//TODO(nsmith5): Add test cases for invalid signature, missing signature etc
+	tests := []struct {
+		description string
+		b64sig      string
+		bundleBody  string
+		shouldErr   bool
+	}{
+		{
+			description: "sigs match",
+			b64sig:      "MEQCIDO3XHbLovPWK+bk8ItCig2cwlr/8MXbLvz3UFzxMGIMAiA1lqdM9IqqUvCUqzOjufTq3sKU3qSn7R5tPqPz0ddNwQ==",
+			bundleBody:  `eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiIzODE1MmQxZGQzMjZhZjQwNWY4OTlkYmNjMmNlMzUwYjVmMTZkNDVkZjdmMjNjNDg4ZjQ4NTBhZmExY2Q4NmQxIn19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FUUNJRE8zWEhiTG92UFdLK2JrOEl0Q2lnMmN3bHIvOE1YYkx2ejNVRnp4TUdJTUFpQTFscWRNOUlxcVV2Q1Vxek9qdWZUcTNzS1UzcVNuN1I1dFBxUHowZGROd1E9PSIsInB1YmxpY0tleSI6eyJjb250ZW50IjoiTFMwdExTMUNSVWRKVGlCUVZVSk1TVU1nUzBWWkxTMHRMUzBLVFVacmQwVjNXVWhMYjFwSmVtb3dRMEZSV1VsTGIxcEplbW93UkVGUlkwUlJaMEZGVUN0RVIyb3ZXWFV4VG5vd01XVjVSV2hVZDNRMlQya3hXV3BGWXdwSloxRldjRlZTTjB0bUwwSm1hVk16Y1ZReFVHd3dkbGh3ZUZwNVMyWkpSMHMyZWxoQ04ybE5aV3RFVTA1M1dHWldPSEpKYUdaMmRrOW5QVDBLTFMwdExTMUZUa1FnVUZWQ1RFbERJRXRGV1MwdExTMHRDZz09In19fX0=`,
+		},
+		{
+			description: "sigs don't match",
+			b64sig:      "bm9wZQo=",
+			bundleBody:  `eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiIzODE1MmQxZGQzMjZhZjQwNWY4OTlkYmNjMmNlMzUwYjVmMTZkNDVkZjdmMjNjNDg4ZjQ4NTBhZmExY2Q4NmQxIn19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FUUNJRE8zWEhiTG92UFdLK2JrOEl0Q2lnMmN3bHIvOE1YYkx2ejNVRnp4TUdJTUFpQTFscWRNOUlxcVV2Q1Vxek9qdWZUcTNzS1UzcVNuN1I1dFBxUHowZGROd1E9PSIsInB1YmxpY0tleSI6eyJjb250ZW50IjoiTFMwdExTMUNSVWRKVGlCUVZVSk1TVU1nUzBWWkxTMHRMUzBLVFVacmQwVjNXVWhMYjFwSmVtb3dRMEZSV1VsTGIxcEplbW93UkVGUlkwUlJaMEZGVUN0RVIyb3ZXWFV4VG5vd01XVjVSV2hVZDNRMlQya3hXV3BGWXdwSloxRldjRlZTTjB0bUwwSm1hVk16Y1ZReFVHd3dkbGh3ZUZwNVMyWkpSMHMyZWxoQ04ybE5aV3RFVTA1M1dHWldPSEpKYUdaMmRrOW5QVDBLTFMwdExTMUZUa1FnVUZWQ1RFbERJRXRGV1MwdExTMHRDZz09In19fX0=`,
+			shouldErr:   true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			sig, err := static.NewSignature([]byte("payload"), test.b64sig)
+			if err != nil {
+				t.Fatalf("failed to create static signature: %v", err)
+			}
+			err = compareSigs(test.bundleBody, sig)
+			if err == nil && test.shouldErr {
+				t.Fatal("test should have errored")
+			}
+			if err != nil && !test.shouldErr {
+				t.Fatal(err)
+			}
+		})
+	}
+}

@@ -83,13 +83,20 @@ func (sg *SecureGet) Do(ctx context.Context) error {
 	}
 
 	if co.SigVerifier != nil || options.EnableExperimental() {
+		// NB: There are only 2 kinds of verification right now:
+		// 1. You gave us the public key explicitly to verify against so co.SigVerifier is non-nil or,
+		// 2. We're going to find an x509 certificate on the signature and verify against Fulcio root trust
+		// TODO(nsmith5): Refactor this verification logic to pass back _how_ verification
+		// was performed so we don't need to use this fragile logic here.
+		fulcioVerified := (co.SigVerifier == nil)
+
 		co.RootCerts = fulcio.GetRoots()
 
 		sp, bundleVerified, err := cosign.VerifyImageSignatures(ctx, ref, co)
 		if err != nil {
 			return err
 		}
-		verify.PrintVerificationHeader(sg.ImageRef, co, bundleVerified)
+		verify.PrintVerificationHeader(sg.ImageRef, co, bundleVerified, fulcioVerified)
 		verify.PrintVerification(sg.ImageRef, sp, "text")
 	}
 

@@ -55,6 +55,7 @@ import (
 	"github.com/sigstore/cosign/pkg/cosign"
 	"github.com/sigstore/cosign/pkg/cosign/kubernetes"
 	cremote "github.com/sigstore/cosign/pkg/cosign/remote"
+	"github.com/sigstore/cosign/pkg/oci/mutate"
 	ociremote "github.com/sigstore/cosign/pkg/oci/remote"
 	"github.com/sigstore/cosign/pkg/sget"
 	sigs "github.com/sigstore/cosign/pkg/signature"
@@ -1156,3 +1157,94 @@ func registryClientOpts(ctx context.Context) []remote.Option {
 		remote.WithContext(ctx),
 	}
 }
+
+// If a signature has a bundle, but *not for that signature*, cosign verification should fail
+// This test is pretty long, so here are the basic points:
+//    1. Sign image1 with a keypair, store entry in rekor
+//    2. Sign image2 with keypair, DO NOT store entry in rekor
+//    3. Take the bundle from image1 and store it on the signature in image2
+//    4. Verification of image2 should now fail, since the bundle is for a different signature
+func TestInvalidBundle(t *testing.T) {
+	regName, stop := reg(t)
+	defer stop()
+	td := t.TempDir()
+
+	img1 := path.Join(regName, "cosign-e2e")
+
+	imgRef, _, cleanup := mkimage(t, img1)
+	defer cleanup()
+
+	_, privKeyPath, pubKeyPath := keypair(t, td)
+
+	ctx := context.Background()
+
+	// Sign image1 and store the entry in rekor
+	// (we're just using it for its bundle)
+	defer setenv(t, options.ExperimentalEnv, "1")()
+	remoteOpts := ociremote.WithRemoteOptions(registryClientOpts(ctx)...)
+	ko := sign.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc, RekorURL: rekorURL}
+	regOpts := options.RegistryOptions{}
+
+	must(sign.SignCmd(ro, ko, regOpts, nil, []string{img1}, "", true, "", "", "", true, false, ""), t)
+	// verify image1
+	must(verify(pubKeyPath, img1, true, nil, ""), t)
+	// extract the bundle from image1
+	si, err := ociremote.SignedImage(imgRef, remoteOpts)
+	must(err, t)
+	imgSigs, err := si.Signatures()
+	must(err, t)
+	sigs, err := imgSigs.Get()
+	must(err, t)
+	if l := len(sigs); l != 1 {
+		t.Error("expected one signature")
+	}
+	bund, err := sigs[0].Bundle()
+	must(err, t)
+	if bund == nil {
+		t.Fail()
+	}
+
+	// Now, we move on to image2
+	// Sign image2 and DO NOT store the entry in rekor
+	defer setenv(t, options.ExperimentalEnv, "0")()
+	img2 := path.Join(regName, "unrelated")
+	imgRef2, _, cleanup := mkimage(t, img2)
+	defer cleanup()
+	must(sign.SignCmd(ro, ko, regOpts, nil, []string{img2}, "", true, "", "", "", false, false, ""), t)
+	must(verify(pubKeyPath, img2, true, nil, ""), t)
+
+	si2, err := ociremote.SignedEntity(imgRef2, remoteOpts)
+	must(err, t)
+	sigs2, err := si2.Signatures()
+	must(err, t)
+	gottenSigs2, err := sigs2.Get()
+	must(err, t)
+	if len(gottenSigs2) != 1 {
+		t.Fatal("there should be one signature")
+	}
+	sigsTag, err := ociremote.SignatureTag(imgRef2)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// At this point, we would mutate the signature to add the bundle annotation
+	// since we don't have a function for it at the moment, mock this by deleting the signature
+	// and pushing a new signature with the additional bundle annotation
+	if err := remote.Delete(sigsTag); err != nil {
+		t.Fatal(err)
+	}
+	mustErr(verify(pubKeyPath, img2, true, nil, ""), t)
+
+	newSig, err := mutate.Signature(gottenSigs2[0], mutate.WithBundle(bund))
+	must(err, t)
+	si2, err = ociremote.SignedEntity(imgRef2, remoteOpts)
+	must(err, t)
+	newImage, err := mutate.AttachSignatureToEntity(si2, newSig)
+	must(err, t)
+	if err := ociremote.WriteSignatures(sigsTag.Repository, newImage); err != nil {
+		t.Fatal(err)
+	}
+
+	// veriyfing image2 now should fail
+	mustErr(verify(pubKeyPath, img2, true, nil, ""), t)
+}