From 98e3c92b135a2eaad97ea2a7ba98ca5267e13a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Van=20Rompay?= <97546950+cedricvanrompay-datadog@users.noreply.github.com> Date: Fri, 2 Sep 2022 23:04:36 +0200 Subject: [PATCH] fix(verify): Fix a vulnerability in the verification of threshold signatures (due to handling of keys with multiple IDs) (#369) * add test for several signatures same key diff ID * fix verifying threshold signatures * add some comments * rename variables and add comments Co-authored-by: Trishank Karthik Kuppusamy Signed-off-by: Zachary Newman --- client/client_test.go | 135 ++++++++++++++++++++++++++++++++++++++++++ verify/verify.go | 27 ++++++--- 2 files changed, 154 insertions(+), 8 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 2085dcd8..b476440f 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2,6 +2,8 @@ package client import ( "bytes" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -1270,3 +1272,136 @@ func (s *ClientSuite) TestVerifyDigest(c *C) { c.Assert(client.VerifyDigest(hash, "sha256", size, digest), IsNil) } + +type StateLessSuite struct{} + +var _ = Suite(&StateLessSuite{}) + +func (s *StateLessSuite) TestRejectsMultiSignaturesSameKeyDifferentIDs(c *C) { + // In this test Alice and Bob want to create a TUF repo + // where a root key rotation would require both their signatures. + // Alice uses an old version of Go-TUF where each key gets assigned several IDs. + // Bob uses a modern version of Go-TUF that does not produce the same list of IDs for a same key. + // This test checks that the TUF client + // will not accept a root rotation + // signed twice with Alice's key with different key IDs each time. + // This test was failing with https://github.com/theupdateframework/go-tuf/tree/ac7b5d7bce18cca5a84a28b021bd6372f450b35b + // because the signature verification code was assuming that the key IDs used in the metadata + // were the same as the one the TUF library of the client would generate, + // breaking the security of threshold signatures. + + // The attack works just the same if Alice is malicious from the beginning + // and convinces Bob to sign an initial "root.json" + // with additional key IDs for her only key, + // but this scenario show that the vulnerability can even impact situations + // where Alice is not malicious at all, + // she was simply using an old client and an attacker stole her key. + // The purpose of threshold signatures in TUF is precisely + // to make sure that an attacker cannot forge signatures + // if they did not steal a large enough number of keys. + + alice, err := keys.GenerateEd25519Key() + if err != nil { + panic(err) + } + + root := data.NewRoot() + root.Version = 1 + root.Roles["root"] = &data.Role{ + KeyIDs: []string{}, + Threshold: 2, // Note the threshold + } + + // reproduces how IDs were computed in + // https://github.com/theupdateframework/go-tuf/blob/8e84384bebe3/data/types.go#L50 + oldTUFIDs := func(k *data.PublicKey) []string { + bytes, _ := cjson.EncodeCanonical(k) + digest := sha256.Sum256(bytes) + ids := []string{hex.EncodeToString(digest[:])} + + if k.Scheme != "" || len(k.Algorithms) != 0 { + bytes, _ = cjson.EncodeCanonical(&data.PublicKey{ + Type: k.Type, + Value: k.Value, + }) + digest = sha256.Sum256(bytes) + ids = append(ids, hex.EncodeToString(digest[:])) + } + + return ids + } + + // Alice adds her key using an old version of go-tuf + // which will use several IDs + for _, keyID := range oldTUFIDs(alice.PublicData()) { + root.Keys[keyID] = alice.PublicData() + root.Roles["root"].KeyIDs = append(root.Roles["root"].KeyIDs, keyID) + } + + bob, err := keys.GenerateEd25519Key() + if err != nil { + panic(err) + } + + root.AddKey(bob.PublicData()) + root.Roles["root"].KeyIDs = append( + root.Roles["root"].KeyIDs, + bob.PublicData().IDs()..., + ) + + // signer for the other roles, not important + delegatedSigner, _ := keys.GenerateEd25519Key() + root.AddKey(delegatedSigner.PublicData()) + for _, role := range []string{"targets", "snapshot", "timestamp"} { + root.Roles[role] = &data.Role{ + KeyIDs: delegatedSigner.PublicData().IDs(), + Threshold: 1, + } + } + + signedRoot, err := sign.Marshal(root, alice, bob) + c.Assert(err, IsNil) + rootJSON, err := json.Marshal(signedRoot) + c.Assert(err, IsNil) + + // producing evil root using only Alice's key + + evilRoot := root + evilRoot.Version = 2 + + canonical, err := cjson.EncodeCanonical(evilRoot) + c.Assert(err, IsNil) + sig, err := alice.SignMessage(canonical) + c.Assert(err, IsNil) + signedEvilRoot := &data.Signed{ + Signed: canonical, + Signatures: make([]data.Signature, 0), + } + for _, keyID := range oldTUFIDs(alice.PublicData()) { + signedEvilRoot.Signatures = append(signedEvilRoot.Signatures, data.Signature{ + Signature: sig, + KeyID: keyID, + }) + } + evilRootJSON, err := json.Marshal(signedEvilRoot) + c.Assert(err, IsNil) + + // checking that client does not accept root rotation + // to evil root + + localStore := MemoryLocalStore() + err = localStore.SetMeta("root.json", rootJSON) + c.Assert(err, IsNil) + + remoteStore := newFakeRemoteStore() + remoteStore.meta["2.root.json"] = newFakeFile(evilRootJSON) + + client := NewClient(localStore, remoteStore) + + err = client.UpdateRoots() + if err != nil { + c.Assert(err, DeepEquals, verify.ErrRoleThreshold{Expected: 2, Actual: 1}) + } else { + c.Fatalf("client returned no error when updating with evil root") + } +} diff --git a/verify/verify.go b/verify/verify.go index c12aaaf8..f5675a25 100644 --- a/verify/verify.go +++ b/verify/verify.go @@ -92,8 +92,8 @@ func (db *DB) VerifySignatures(s *data.Signed, role string) error { // 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. - seen := make(map[string]struct{}) - valid := 0 + verifiedKeyIDs := make(map[string]struct{}) + numVerifiedKeys := 0 for _, sig := range s.Signatures { if !roleData.ValidKey(sig.KeyID) { continue @@ -104,21 +104,32 @@ func (db *DB) VerifySignatures(s *data.Signed, role string) error { } if err := verifier.Verify(msg, sig.Signature); err != nil { + // FIXME: don't err out on the 1st bad signature. return ErrInvalid } // Only consider this key valid if we haven't seen any of it's // key ids before. - if _, ok := seen[sig.KeyID]; !ok { - for _, id := range verifier.MarshalPublicKey().IDs() { - seen[id] = struct{}{} + // Careful: we must not rely on the key IDs _declared in the file_, + // instead we get to decide what key IDs this key correspond to. + // XXX dangerous; better stop supporting multiple key IDs altogether. + keyIDs := verifier.MarshalPublicKey().IDs() + wasKeySeen := false + for _, keyID := range keyIDs { + if _, present := verifiedKeyIDs[keyID]; present { + wasKeySeen = true + } + } + if !wasKeySeen { + for _, id := range keyIDs { + verifiedKeyIDs[id] = struct{}{} } - valid++ + numVerifiedKeys++ } } - if valid < roleData.Threshold { - return ErrRoleThreshold{roleData.Threshold, valid} + if numVerifiedKeys < roleData.Threshold { + return ErrRoleThreshold{roleData.Threshold, numVerifiedKeys} } return nil