fix: fix verification to continue on invalid sigs (theupdateframework…

…#418)

* fix: fix verification to continue on invalid sigs

Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>

client/client_test.go

@@ -2,6 +2,7 @@ package client
 
 import (
 	"bytes"
+	"crypto/ed25519"
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
@@ -351,7 +352,7 @@ func (s *ClientSuite) TestInit(c *C) {
 	_, err = client.Update()
 	c.Assert(err, Equals, ErrNoRootKeys)
 
-	// check Init() returns ErrInvalid when the root's signature is
+	// check Init() returns ErrRoleThreshold when the root's signature is
 	// invalid
 	// modify root and marshal without regenerating signatures
 	root.Version = root.Version + 1
@@ -360,14 +361,38 @@ func (s *ClientSuite) TestInit(c *C) {
 	dataSigned.Signed = rootBytes
 	dataBytes, err := json.Marshal(dataSigned)
 	c.Assert(err, IsNil)
-	c.Assert(client.Init(dataBytes), Equals, verify.ErrInvalid)
+	c.Assert(client.Init(dataBytes), Equals, verify.ErrRoleThreshold{
+		Expected: 1, Actual: 0})
 
 	// check Update() does not return ErrNoRootKeys after initialization
 	c.Assert(client.Init(bytes), IsNil)
 	_, err = client.Update()
 	c.Assert(err, IsNil)
 }
 
+// This is a regression test for https://github.com/theupdateframework/go-tuf/issues/370
+// where a single invalid signature resulted in an early return.
+// Instead, the client should have continued and counted the number
+// of valid signatures, ignoring the incorrect one.
+func (s *ClientSuite) TestExtraRootSignaturesOnInit(c *C) {
+	client := NewClient(MemoryLocalStore(), s.remote)
+	bytes, err := s.readMeta("root.json")
+	c.Assert(err, IsNil)
+	dataSigned := &data.Signed{}
+	c.Assert(json.Unmarshal(bytes, dataSigned), IsNil)
+
+	// check Init() succeeds when an extra invalid signature was
+	// added to the root.
+	dataSigned.Signatures = append(dataSigned.Signatures,
+		data.Signature{
+			KeyID:     dataSigned.Signatures[0].KeyID,
+			Signature: make([]byte, ed25519.SignatureSize),
+		})
+	dataBytes, err := json.Marshal(dataSigned)
+	c.Assert(err, IsNil)
+	c.Assert(client.Init(dataBytes), IsNil)
+}
+
 func (s *ClientSuite) TestFirstUpdate(c *C) {
 	files, err := s.newClient(c).Update()
 	c.Assert(err, IsNil)
@@ -455,6 +480,44 @@ func (s *ClientSuite) TestNewRoot(c *C) {
 	}
 }
 
+// This is a regression test for https://github.com/theupdateframework/go-tuf/issues/370
+// where a single invalid signature resulted in an early return.
+// Instead, the client should have continued and counted the number
+// of valid signatures, ignoring the incorrect one.
+func (s *ClientSuite) TestExtraSignaturesOnRootUpdate(c *C) {
+	client := s.newClient(c)
+
+	// Add an extra root key to update the root to a new version.
+	s.genKey(c, "root")
+	// update metadata
+	c.Assert(s.repo.Sign("targets.json"), IsNil)
+	c.Assert(s.repo.Snapshot(), IsNil)
+	c.Assert(s.repo.Timestamp(), IsNil)
+	c.Assert(s.repo.Commit(), IsNil)
+	s.syncRemote(c)
+
+	// Add an extra signature to the new remote root.
+	bytes, err := s.readMeta("root.json")
+	c.Assert(err, IsNil)
+	dataSigned := &data.Signed{}
+	c.Assert(json.Unmarshal(bytes, dataSigned), IsNil)
+	dataSigned.Signatures = append(dataSigned.Signatures,
+		data.Signature{
+			KeyID:     dataSigned.Signatures[0].KeyID,
+			Signature: make([]byte, ed25519.SignatureSize),
+		})
+	dataBytes, err := json.Marshal(dataSigned)
+	c.Assert(err, IsNil)
+	s.setRemoteMeta("root.json", dataBytes)
+	s.setRemoteMeta("2.root.json", dataBytes)
+
+	// check Update() succeeds when an extra invalid signature was
+	// added to the root.
+	_, err = client.Update()
+	c.Assert(err, IsNil)
+	c.Assert(client.rootVer, Equals, int64(2))
+}
+
 // startTUFRepoServer starts a HTTP server to serve a TUF Repo.
 func startTUFRepoServer(baseDir string, relPath string) (net.Listener, error) {
 	serverDir := filepath.Join(baseDir, relPath)
@@ -517,9 +580,11 @@ func (s *ClientSuite) TestUpdateRoots(c *C) {
 		// Fails updating root from version 1 to version 3 when versions 1 and 3 are expired but version 2 is not expired.
 		{"testdata/Published3Times_keyrotated_latestrootexpired", ErrDecodeFailed{File: "root.json", Err: verify.ErrExpired{}}, map[string]int64{"root": 2, "timestamp": 1, "snapshot": 1, "targets": 1}},
 		// Fails updating root from version 1 to version 2 when old root 1 did not sign off on it (nth root didn't sign off n+1).
-		{"testdata/Published2Times_keyrotated_invalidOldRootSignature", errors.New("tuf: signature verification failed"), map[string]int64{}},
+		// TODO(asraa): This testcase should have revoked the old key!
+		// https://github.com/theupdateframework/go-tuf/issues/417
+		{"testdata/Published2Times_keyrotated_invalidOldRootSignature", nil, map[string]int64{}},
 		// Fails updating root from version 1 to version 2 when the new root 2 did not sign itself (n+1th root didn't sign off n+1)
-		{"testdata/Published2Times_keyrotated_invalidNewRootSignature", errors.New("tuf: signature verification failed"), map[string]int64{}},
+		{"testdata/Published2Times_keyrotated_invalidNewRootSignature", verify.ErrRoleThreshold{Expected: 1, Actual: 0}, map[string]int64{}},
 		// Fails updating root to 2.root.json when the value of the version field inside it is 1 (rollback attack prevention).
 		{"testdata/Published1Time_backwardRootVersion", verify.ErrWrongVersion(verify.ErrWrongVersion{Given: 1, Expected: 2}), map[string]int64{}},
 		// Fails updating root to 2.root.json when the value of the version field inside it is 3 (rollforward attack prevention).

go.mod

@@ -20,6 +20,6 @@ require (
 	github.com/kr/pretty v0.2.1 // indirect
 	github.com/kr/text v0.1.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
+	golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -101,8 +101,9 @@ golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f h1:v4INt8xihDGvnrfjMDVXGxw9wrfxYyCjk0KbXjhR55s=
+golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 h1:JGgROgKl9N8DuW20oFS5gxc+lE67/N3FcwmBPMe7ArY=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=

repo.go

@@ -843,23 +843,41 @@ func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signatur
 		return ErrInvalidRole{role, "no trusted keys for role"}
 	}
 
+	s, err := r.SignedMeta(roleFilename)
+	if err != nil {
+		return err
+	}
+
 	keyInDB := false
+	validSig := false
 	for _, db := range dbs {
 		roleData := db.GetRole(role)
 		if roleData == nil {
 			return ErrInvalidRole{role, "role is not in verifier DB"}
 		}
 		if roleData.ValidKey(signature.KeyID) {
 			keyInDB = true
+
+			verifier, err := db.GetVerifier(signature.KeyID)
+			if err != nil {
+				continue
+			}
+
+			// Now check if this validly signed the metadata.
+			if err := verify.VerifySignature(s.Signed, signature.Signature,
+				verifier); err == nil {
+				validSig = true
+				break
+			}
 		}
 	}
 	if !keyInDB {
+		// This key was not delegated for the role in any delegatee.
 		return verify.ErrInvalidKey
 	}
-
-	s, err := r.SignedMeta(roleFilename)
-	if err != nil {
-		return err
+	if !validSig {
+		// The signature was invalid.
+		return verify.ErrInvalid
 	}
 
 	// Add or update signature.
@@ -872,16 +890,6 @@ func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signatur
 	signatures = append(signatures, signature)
 	s.Signatures = signatures
 
-	// Check signature on signed meta. Ignore threshold errors as this may not be fully
-	// signed.
-	for _, db := range dbs {
-		if err := db.VerifySignatures(s, role); err != nil {
-			if _, ok := err.(verify.ErrRoleThreshold); !ok {
-				return err
-			}
-		}
-	}
-
 	b, err := r.jsonMarshal(s)
 	if err != nil {
 		return err

repo_test.go

@@ -2663,7 +2663,8 @@ func (rs *RepoSuite) TestSnapshotWithInvalidRoot(c *C) {
 	local.SetMeta("root.json", b)
 
 	// Snapshotting should fail.
-	c.Assert(r.Snapshot(), Equals, ErrInsufficientSignatures{"root.json", verify.ErrInvalid})
+	c.Assert(r.Snapshot(), Equals, ErrInsufficientSignatures{
+		"root.json", verify.ErrRoleThreshold{Expected: 1, Actual: 0}})
 
 	// Correctly sign root
 	c.Assert(r.Sign("root.json"), IsNil)

verify/verify.go

@@ -8,6 +8,7 @@ import (
 	"github.com/secure-systems-lab/go-securesystemslib/cjson"
 	"github.com/theupdateframework/go-tuf/data"
 	"github.com/theupdateframework/go-tuf/internal/roles"
+	"github.com/theupdateframework/go-tuf/pkg/keys"
 )
 
 type signedMeta struct {
@@ -16,6 +17,22 @@ type signedMeta struct {
 	Version int64     `json:"version"`
 }
 
+// VerifySignature takes a signed JSON message, a signature, and a
+// verifier and verifies the given signature on the JSON message
+// using the verifier. It returns an error if verification fails.
+func VerifySignature(signed json.RawMessage, sig data.HexBytes,
+	verifier keys.Verifier) error {
+	var decoded map[string]interface{}
+	if err := json.Unmarshal(signed, &decoded); err != nil {
+		return err
+	}
+	msg, err := cjson.EncodeCanonical(decoded)
+	if err != nil {
+		return err
+	}
+	return verifier.Verify(msg, sig)
+}
+
 func (db *DB) VerifyIgnoreExpiredCheck(s *data.Signed, role string, minVersion int64) error {
 	if err := db.VerifySignatures(s, role); err != nil {
 		return err
@@ -80,15 +97,6 @@ func (db *DB) VerifySignatures(s *data.Signed, role string) error {
 		return ErrUnknownRole{role}
 	}
 
-	var decoded map[string]interface{}
-	if err := json.Unmarshal(s.Signed, &decoded); err != nil {
-		return err
-	}
-	msg, err := cjson.EncodeCanonical(decoded)
-	if err != nil {
-		return err
-	}
-
 	// Verify that a threshold of keys signed the data. Since keys can have
 	// multiple key ids, we need to protect against multiple attached
 	// signatures that just differ on the key id.
@@ -103,9 +111,13 @@ func (db *DB) VerifySignatures(s *data.Signed, role string) error {
 			continue
 		}
 
-		if err := verifier.Verify(msg, sig.Signature); err != nil {
-			// FIXME: don't err out on the 1st bad signature.
-			return ErrInvalid
+		if err := VerifySignature(s.Signed, sig.Signature, verifier); err != nil {
+			// If a signature fails verification, don't count it towards the
+			// threshold but also return early and error out immediately.
+			// Note: Because of this, it is impossible to distinguish between
+			// an error of an invalid signature and a threshold not achieved.
+			// Invalid signatures lead to not achieving the threshold.
+			continue
 		}
 
 		// Only consider this key valid if we haven't seen any of it's

verify/verify_test.go

@@ -89,9 +89,12 @@ func (VerifySuite) Test(c *C) {
 			err:  ErrUnknownRole{"foo"},
 		},
 		{
+			// It is impossible to distinguish between an error of an invalid
+			// signature and a threshold not achieved. Invalid signatures lead
+			// to not achieving the threshold.
 			name: "signature wrong length",
 			mut:  func(t *test) { t.s.Signatures[0].Signature = []byte{0} },
-			err:  ErrInvalid,
+			err:  ErrRoleThreshold{1, 0},
 		},
 		{
 			name: "key missing from role",
@@ -101,7 +104,15 @@ func (VerifySuite) Test(c *C) {
 		{
 			name: "invalid signature",
 			mut:  func(t *test) { t.s.Signatures[0].Signature = make([]byte, ed25519.SignatureSize) },
-			err:  ErrInvalid,
+			err:  ErrRoleThreshold{1, 0},
+		},
+		{
+			name: "enough signatures with extra invalid signature",
+			mut: func(t *test) {
+				t.s.Signatures = append(t.s.Signatures, data.Signature{
+					KeyID:     t.s.Signatures[0].KeyID,
+					Signature: make([]byte, ed25519.SignatureSize)})
+			},
 		},
 		{
 			name: "not enough signatures",
@@ -189,7 +200,8 @@ func (VerifySuite) Test(c *C) {
 			},
 		},
 		{
-			name: "invalid ecdsa signature",
+			// The threshold is still achieved.
+			name: "invalid second ecdsa signature",
 			mut: func(t *test) {
 				k, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 				s := ecdsaSigner{k}
@@ -198,7 +210,6 @@ func (VerifySuite) Test(c *C) {
 				t.keys = append(t.keys, s.PublicData())
 				t.roles["root"].KeyIDs = append(t.roles["root"].KeyIDs, s.PublicData().IDs()...)
 			},
-			err: ErrInvalid,
 		},
 	}
 	for _, t := range tests {