Merge pull request from GHSA-ccxc-vr6p-4858

* Make sure signature in Rekor bundle matches signature being verified

Signed-off-by: Priya Wadhwa <[email protected]>

* Only print "fulcio verified" when using Fulcio

Currently we tell users that we've verified against the Fulcio root
trust when verifying but this isn't always true. This work ensures we
only say this when we actually use the Fulcio root cert for
verification.

Signed-off-by: Nathan Smith <[email protected]>

* Add e2e test

Signed-off-by: Nathan Smith <[email protected]>

Co-authored-by: Priya Wadhwa <[email protected]>

cmd/cosign/cli/verify/verify.go

@@ -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

cmd/cosign/cli/verify/verify_attestation.go

@@ -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")
 	}

pkg/cosign/verify.go

@@ -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 {

pkg/cosign/verify_test.go

@@ -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)
+			}
+		})
+	}
+}

pkg/sget/sget.go

@@ -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")
 	}
 

test/e2e_test.go

@@ -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)
+}