Repository: theupdateframework/go-tuf
Pull request #418 (2022-10-14 21:02:05)

fix: fix verification to continue on invalid sigs
Signed-off-by: Asra Ali <asraa@google.com>

Fixes https://github.com/theupdateframework/go-tuf/issues/370

Blocked on https://github.com/theupdateframework/go-tuf/issues/417

As described in the issue, client will currently fail on any single invalid signature rather than counting the number of valid signatures. The fix is that it continues on invalid signatures, not counting them to the threshold of valid signatures.



Please fill in the fields below to submit a pull request.  The more information that is provided, the better.

Fixes #<Issue>

Release Notes: <!-- What comments/remarks should we include in the release notes for this change? -->
* fix!: client will allow invalid signatures on metadata, so long as the number of valid ones reaches the threshold. Previously, the client rejected metadata signed with any invalid signatures.

**Types of changes**:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected). **Please ensure that your PR title** is a [Conventional Commit](https://www.conventionalcommits.org/en/v1.0.0/) breaking change (with a `!`, as in `feat!: change foo`). 

**Description of the changes being introduced by the pull request**:

**Please verify and check that the pull request fulfills the following requirements**:

- [x] Tests have been added for the bug fix or new feature
- [ ] Docs have been added for the bug fix or new feature



Patch:
diff --git a/go.mod b/go.mod
index 717b989..ab6d7ad 100644
--- a/go.mod
+++ b/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
 )
diff --git a/go.sum b/go.sum
index 05aeffa..4acdef4 100644
--- a/go.sum
+++ b/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=
diff --git a/repo.go b/repo.go
index 8b38d50..c6a23de 100644
--- a/repo.go
+++ b/repo.go
@@ -843,7 +843,13 @@ 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 {
@@ -851,15 +857,27 @@ func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signatur
 		}
 		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
diff --git a/verify/verify.go b/verify/verify.go
index f5675a2..e62042e 100644
--- a/verify/verify.go
+++ b/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


Test patch:
diff --git a/client/client_test.go b/client/client_test.go
index 327b137..b9fcbd8 100644
--- a/client/client_test.go
+++ b/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,7 +361,8 @@ 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)
@@ -368,6 +370,29 @@ func (s *ClientSuite) TestInit(c *C) {
 	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).
diff --git a/repo_test.go b/repo_test.go
index 20d57a1..ad69e66 100644
--- a/repo_test.go
+++ b/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)
diff --git a/verify/verify_test.go b/verify/verify_test.go
index afbf79d..191c1ed 100644
--- a/verify/verify_test.go
+++ b/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 {