fix(verify): backport "Fix a vulnerability in the verification of threshold si… (#375)
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 <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Zachary Newman <z@znewman.net>
Signed-off-by: Zachary Newman <z@znewman.net>
Co-authored-by: Cédric Van Rompay <97546950+cedricvanrompay-datadog@users.noreply.github.com>
Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
diff --git a/client/client_test.go b/client/client_test.go
index 2085dcd..b476440 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -2,6 +2,8 @@
import (
"bytes"
+ "crypto/sha256"
+ "encoding/hex"
"encoding/json"
"errors"
"fmt"
@@ -1270,3 +1272,136 @@
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 c12aaaf..f5675a2 100644
--- a/verify/verify.go
+++ b/verify/verify.go
@@ -92,8 +92,8 @@
// 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 @@
}
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
