Skip to content

Commit

Permalink
Cherry pick vulnerability PRs to release-1.5 (#1486)
Browse files Browse the repository at this point in the history 
* Merge pull request from GHSA-ccxc-vr6p-4858

* Make sure signature in Rekor bundle matches signature being verified

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* 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 <nathan@chainguard.dev>

* Add e2e test

Signed-off-by: Nathan Smith <nathan@chainguard.dev>

Co-authored-by: Priya Wadhwa <priya@chainguard.dev>

* fix lint (#1484)

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* Manual merge conflict resolution in e2e_test.

Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>

Co-authored-by: Nathan Smith <12156185+nsmith5@users.noreply.github.com>
Co-authored-by: Dan Lorenc <lorenc.d@gmail.com>
  • Loading branch information
@priyawadhwa @nsmith5 @dlorenc
3 people committed Feb 18, 2022
1 parent 52164f2 commit c09e04a
Show file tree
Hide file tree
Showing 6 changed files with 226 additions and 6 deletions.
17 changes: 13 additions & 4 deletions cmd/cosign/cli/verify/verify.go
View file
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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
Expand Down
9 changes: 8 additions & 1 deletion cmd/cosign/cli/verify/verify_attestation.go
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}
Expand Down
67 changes: 67 additions & 0 deletions pkg/cosign/verify.go
View file
Expand Up @@ -585,6 +585,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
}

pub, err := GetRekorPub(ctx)
if err != nil {
return false, errors.Wrap(err, "retrieving rekor public key")
Expand Down Expand Up @@ -630,6 +634,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
Expand Down Expand Up @@ -691,6 +720,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 {
Expand Down
38 changes: 38 additions & 0 deletions pkg/cosign/verify_test.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
})
}
}
9 changes: 8 additions & 1 deletion pkg/sget/sget.go
View file
Expand Up @@ -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")
}

Expand Down
92 changes: 92 additions & 0 deletions test/e2e_test.go
View file
Expand Up @@ -56,6 +56,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"
Expand Down Expand Up @@ -1106,3 +1107,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(ctx, 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(ctx, 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)
}

0 comments on commit c09e04a

Please sign in to comment.