fix(verify): backport "Fix a vulnerability in the verification of thr…

…eshold 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>

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")
+	}
+}

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