diff --git a/client/client_test.go b/client/client_test.go index 327b1371..b9fcbd81 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/go.mod b/go.mod index 717b9895..ab6d7ada 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 05aeffa5..4acdef41 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 8b38d508..c6a23dee 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/repo_test.go b/repo_test.go index 20d57a10..ad69e664 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.go b/verify/verify.go index f5675a25..e62042ee 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 diff --git a/verify/verify_test.go b/verify/verify_test.go index afbf79d8..191c1ed0 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 {