diff --git a/universalClient/tss/dkls/utils.go b/universalClient/tss/dkls/utils.go index c77c6241..4a42b813 100644 --- a/universalClient/tss/dkls/utils.go +++ b/universalClient/tss/dkls/utils.go @@ -2,6 +2,10 @@ package dkls import ( "crypto/sha256" + "encoding/binary" + "fmt" + + session "go-wrapper/go-dkls/sessions" ) // deriveKeyID derives a key ID bytes from a string key ID. @@ -22,3 +26,83 @@ func encodeParticipantIDs(participants []string) []byte { } return ids } + +// --- Setup decoding ------------------------------------------------------- +// The coordinator supplies the setup blob and the values a follower validates +// separately. DKLS runs on the blob, so these expose what it actually contains +// and let callers bind the two. Both return an error on a malformed blob. + +// SetupMessageHash returns the message hash embedded in a sign setup blob. +// DklsSignSessionFromSetup signs over the setup, not over any hash passed +// alongside it, so callers must confirm the two agree. +func SetupMessageHash(setupData []byte) ([]byte, error) { + if len(setupData) == 0 { + return nil, fmt.Errorf("setupData is required") + } + return session.DklsDecodeMessage(setupData) +} + +// SetupParticipants returns the participant list embedded in a DKLS setup blob, +// in index order. The setup is what actually drives the session, so callers must +// confirm it matches the participants they validated. Otherwise a coordinator +// can present one list for validation and run the session over another. +// +// Party names decode by index and come back empty past the end, which is how the +// list terminates. +func SetupParticipants(setupData []byte) ([]string, error) { + if len(setupData) == 0 { + return nil, fmt.Errorf("setupData is required") + } + var participants []string + for i := 0; ; i++ { + name, err := session.DklsDecodePartyName(setupData, i) + if err != nil { + return nil, fmt.Errorf("failed to decode party name at index %d: %w", i, err) + } + if len(name) == 0 { + break + } + participants = append(participants, string(name)) + } + return participants, nil +} + +// Setup blobs are a tag-length-value list after a fixed header. The wrapper +// exposes decoders for the key ID, message and party names but not the +// threshold, so that one is read here. Values are laid out as: +// +// tag uint16 little endian +// length uint16 little endian, stored as length-1 +// value length bytes +// +// The header is MESSAGE_ID_SIZE(32) + 2 + 2. This mirrors the library's internal +// encoding, so TestSetupThreshold pins it: if the format changes, that test +// fails rather than this silently reading the wrong byte. +const ( + setupHeaderSize = 36 + setupTagThreshold = 1 +) + +// SetupThreshold returns the threshold embedded in a keygen, keyrefresh or +// quorumchange setup blob. Sign setups carry no threshold and return an error. +func SetupThreshold(setupData []byte) (int, error) { + if len(setupData) < setupHeaderSize { + return 0, fmt.Errorf("setup message too short to contain a threshold") + } + for offset := setupHeaderSize; offset+4 <= len(setupData); { + tag := binary.LittleEndian.Uint16(setupData[offset : offset+2]) + length := int(binary.LittleEndian.Uint16(setupData[offset+2:offset+4])) + 1 + valueStart := offset + 4 + if valueStart+length > len(setupData) { + return 0, fmt.Errorf("setup message is malformed: tag %d claims %d bytes past the end", tag, length) + } + if tag == setupTagThreshold { + if length != 1 { + return 0, fmt.Errorf("threshold tag has unexpected length %d", length) + } + return int(setupData[valueStart]), nil + } + offset = valueStart + length + } + return 0, fmt.Errorf("setup message carries no threshold") +} diff --git a/universalClient/tss/dkls/utils_test.go b/universalClient/tss/dkls/utils_test.go index df57610e..29f43285 100644 --- a/universalClient/tss/dkls/utils_test.go +++ b/universalClient/tss/dkls/utils_test.go @@ -1,8 +1,11 @@ package dkls import ( + "bytes" "crypto/sha256" "testing" + + session "go-wrapper/go-dkls/sessions" ) func TestDeriveKeyID(t *testing.T) { @@ -58,3 +61,162 @@ func TestEncodeParticipantIDs(t *testing.T) { }) } } + +// The setup blob is what DKLS actually runs on, so these decoders are what let a +// follower bind it to the values it validated separately. Both must report what +// the blob really contains, and must error rather than guess on a malformed one. + +func TestSetupMessageHash(t *testing.T) { + participantIDs := encodeParticipantIDs([]string{"party1", "party2"}) + keyID := make([]byte, 32) + + legitHash := make([]byte, 32) + copy(legitHash, "legitimate-outbound-hash-32bytes") + attackerHash := make([]byte, 32) + copy(attackerHash, "attacker-chosen-vault-call-digest") + + t.Run("returns the hash embedded in the setup", func(t *testing.T) { + setup, err := session.DklsSignSetupMsgNew(keyID, nil, legitHash, participantIDs) + if err != nil { + t.Fatalf("failed to build sign setup: %v", err) + } + got, err := SetupMessageHash(setup) + if err != nil { + t.Fatalf("SetupMessageHash() error = %v", err) + } + if !bytes.Equal(got, legitHash) { + t.Errorf("SetupMessageHash() = %x, want %x", got, legitHash) + } + }) + + // A substituted setup must report the hash it really signs, which is what + // makes the mismatch detectable. + t.Run("substituted setup reports the attacker hash", func(t *testing.T) { + setup, err := session.DklsSignSetupMsgNew(keyID, nil, attackerHash, participantIDs) + if err != nil { + t.Fatalf("failed to build sign setup: %v", err) + } + got, err := SetupMessageHash(setup) + if err != nil { + t.Fatalf("SetupMessageHash() error = %v", err) + } + if bytes.Equal(got, legitHash) { + t.Fatal("substituted setup must not report the legitimate hash") + } + if !bytes.Equal(got, attackerHash) { + t.Errorf("SetupMessageHash() = %x, want %x", got, attackerHash) + } + }) + + t.Run("errors on empty and malformed setup", func(t *testing.T) { + if _, err := SetupMessageHash(nil); err == nil { + t.Error("SetupMessageHash(nil) should error") + } + if _, err := SetupMessageHash([]byte("not-a-dkls-setup")); err == nil { + t.Error("SetupMessageHash(malformed) should error") + } + }) +} + +func TestSetupParticipants(t *testing.T) { + t.Run("returns the participants in index order", func(t *testing.T) { + want := []string{"alice", "bob", "carol"} + setup, err := session.DklsKeygenSetupMsgNew(2, nil, encodeParticipantIDs(want)) + if err != nil { + t.Fatalf("failed to build keygen setup: %v", err) + } + got, err := SetupParticipants(setup) + if err != nil { + t.Fatalf("SetupParticipants() error = %v", err) + } + if len(got) != len(want) { + t.Fatalf("SetupParticipants() = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("participant %d = %q, want %q", i, got[i], want[i]) + } + } + }) + + // Enumeration terminates on the first empty name rather than an error, which + // is the contract this relies on to find the end of the list. + t.Run("terminates at the end of a two party list", func(t *testing.T) { + want := []string{"first", "second"} + setup, err := session.DklsKeygenSetupMsgNew(2, nil, encodeParticipantIDs(want)) + if err != nil { + t.Fatalf("failed to build keygen setup: %v", err) + } + got, err := SetupParticipants(setup) + if err != nil { + t.Fatalf("SetupParticipants() error = %v", err) + } + if len(got) != 2 || got[0] != "first" || got[1] != "second" { + t.Errorf("SetupParticipants() = %v, want %v", got, want) + } + }) + + t.Run("errors on empty and malformed setup", func(t *testing.T) { + if _, err := SetupParticipants(nil); err == nil { + t.Error("SetupParticipants(nil) should error") + } + if _, err := SetupParticipants([]byte("not-a-dkls-setup")); err == nil { + t.Error("SetupParticipants(malformed) should error") + } + }) +} + +// Pins the setup TLV layout this package parses directly. If the library +// changes its encoding, this fails loudly instead of SetupThreshold silently +// reading the wrong byte. +func TestSetupThreshold(t *testing.T) { + participants := []string{"alice", "bob", "carol"} + + t.Run("reads the embedded keygen threshold", func(t *testing.T) { + for _, want := range []int{2, 3} { + setup, err := session.DklsKeygenSetupMsgNew(want, nil, encodeParticipantIDs(participants)) + if err != nil { + t.Fatalf("failed to build keygen setup with threshold %d: %v", want, err) + } + got, err := SetupThreshold(setup) + if err != nil { + t.Fatalf("SetupThreshold() error = %v", err) + } + if got != want { + t.Errorf("SetupThreshold() = %d, want %d", got, want) + } + } + }) + + // A downgraded setup must report the weaker threshold it really carries, + // which is what makes the mismatch detectable. + t.Run("downgraded setup reports the weaker threshold", func(t *testing.T) { + setup, err := session.DklsKeygenSetupMsgNew(2, nil, encodeParticipantIDs(participants)) + if err != nil { + t.Fatalf("failed to build keygen setup: %v", err) + } + got, err := SetupThreshold(setup) + if err != nil { + t.Fatalf("SetupThreshold() error = %v", err) + } + if got == 3 { + t.Fatal("downgraded setup must not report the expected threshold") + } + if got != 2 { + t.Errorf("SetupThreshold() = %d, want 2", got) + } + }) + + t.Run("errors on short, malformed and thresholdless setups", func(t *testing.T) { + if _, err := SetupThreshold(nil); err == nil { + t.Error("SetupThreshold(nil) should error") + } + if _, err := SetupThreshold([]byte("too-short")); err == nil { + t.Error("SetupThreshold(short) should error") + } + // Header present but no tags at all. + if _, err := SetupThreshold(make([]byte, setupHeaderSize)); err == nil { + t.Error("SetupThreshold(no tags) should error") + } + }) +} diff --git a/universalClient/tss/sessionmanager/sessionmanager.go b/universalClient/tss/sessionmanager/sessionmanager.go index c4816226..47db05ec 100644 --- a/universalClient/tss/sessionmanager/sessionmanager.go +++ b/universalClient/tss/sessionmanager/sessionmanager.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "math/big" + "slices" "sync" "time" @@ -196,6 +197,18 @@ func (sm *SessionManager) handleSetupMessage(ctx context.Context, senderPeerID s } } + // 6c. Everything validated above came from message fields, but the DKLS + // session runs on msg.Payload, and the two arrive unbound. Require the setup + // blob to carry exactly what we approved, before the ACK, so no shares are + // ever produced for a setup we did not verify. + if err := verifySetupMatchesValidated(msg, event.Type); err != nil { + sm.logger.Error().Err(err). + Str("event_id", msg.EventID). + Str("coordinator", senderPeerID). + Msg("setup message does not match the validated request - rejecting") + return err + } + // 7. Create session based on protocol type session, err := sm.createSession(ctx, event, msg) if err != nil { @@ -976,6 +989,79 @@ func (sm *SessionManager) verifyOutboundSigningRequest(ctx context.Context, even return nil } +// verifySetupMatchesValidated requires the coordinator's DKLS setup blob to +// carry exactly the values the follower validated from the message fields. +// DKLS runs on the blob, so without this the validated values are decorative: +// a coordinator can present legitimate ones for checking and embed different +// ones in Payload. +// +// Participants are checked for every protocol. Sign types additionally bind the +// signing hash; key-lifecycle types additionally bind the threshold, which the +// session constructors accept but ignore, so the embedded value is what the +// protocol actually runs with. +func verifySetupMatchesValidated(msg *coordinator.Message, eventType string) error { + if err := setupBindsParticipants(msg.Payload, msg.Participants); err != nil { + return err + } + if eventType == store.EventTypeSignOutbound || eventType == store.EventTypeSignFundMigrate { + if msg.UnsignedSigningReq == nil { + return fmt.Errorf("sign setup has no signing request to bind against") + } + return setupBindsHash(msg.Payload, msg.UnsignedSigningReq.SigningHash) + } + return setupBindsThreshold(msg.Payload, msg.Participants) +} + +// setupBindsThreshold requires the setup blob to embed the threshold the +// follower derives from the validated participants. Without it a coordinator can +// embed a lower one and elicit help producing a weaker key than was agreed. +func setupBindsThreshold(setupData []byte, validated []string) error { + expected := coordinator.CalculateThreshold(len(validated)) + embedded, err := dkls.SetupThreshold(setupData) + if err != nil { + return fmt.Errorf("cannot decode setup message threshold: %w", err) + } + if embedded != expected { + return fmt.Errorf("setup message threshold %d does not match expected %d for %d participants", + embedded, expected, len(validated)) + } + return nil +} + +// setupBindsHash requires the setup blob to embed exactly the verified hash. +func setupBindsHash(setupData, verifiedHash []byte) error { + if len(verifiedHash) == 0 { + return fmt.Errorf("no verified signing hash to bind setup message to") + } + embedded, err := dkls.SetupMessageHash(setupData) + if err != nil { + return fmt.Errorf("cannot decode setup message to check signing hash: %w", err) + } + if !bytes.Equal(embedded, verifiedHash) { + return fmt.Errorf("setup message signs hash %s but verified hash is %s", + hex.EncodeToString(embedded), hex.EncodeToString(verifiedHash)) + } + return nil +} + +// setupBindsParticipants requires the setup blob to embed exactly the validated +// participants, in the same order. Index order is part of the protocol, so a +// reorder is as consequential as a substitution. +func setupBindsParticipants(setupData []byte, validated []string) error { + if len(validated) == 0 { + return fmt.Errorf("no validated participants to bind setup message to") + } + embedded, err := dkls.SetupParticipants(setupData) + if err != nil { + return fmt.Errorf("cannot decode setup message participants: %w", err) + } + if !slices.Equal(embedded, validated) { + return fmt.Errorf("setup message participants %v do not match validated participants %v", + embedded, validated) + } + return nil +} + // maxNonceGap bounds how far above the ceiling base a coordinator may assign. // An honest coordinator starts at the pending nonce and increments at most // coordinator.PerChainCap times per poll, so pending+PerChainCap is the true diff --git a/universalClient/tss/sessionmanager/sessionmanager_test.go b/universalClient/tss/sessionmanager/sessionmanager_test.go index 0a76ab6d..73f0dd2a 100644 --- a/universalClient/tss/sessionmanager/sessionmanager_test.go +++ b/universalClient/tss/sessionmanager/sessionmanager_test.go @@ -9,10 +9,13 @@ import ( "fmt" "math/big" "reflect" + "strings" "testing" "time" "unsafe" + session "go-wrapper/go-dkls/sessions" + "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -467,18 +470,17 @@ func TestSessionManager_Integration(t *testing.T) { Type: "setup", EventID: event.EventID, Participants: []string{"validator1", "validator2", "validator3"}, - Payload: []byte("invalid setup data"), // Will fail when creating session + Payload: []byte("invalid setup data"), // rejected before a session is created } - // This will fail at session creation or GetLatestBlockNum, but validation should pass + // Rejected at the setup-binding check (the payload is not a decodable DKLS + // setup), or earlier at GetLatestBlockNum. Either way validation must not + // let an unbound payload reach session creation. err := sm.HandleIncomingMessage(ctx, "peer1", &msg) - // We expect an error because we can't create a real DKLS session with invalid data - // or because GetLatestBlockNum fails assert.Error(t, err) - // Error should be about session creation, DKLS library, or no endpoints assert.True(t, - containsAny(err.Error(), []string{"failed to create session", "DKLS", "dkls", "session", "no endpoints"}), - "error should be about session creation or endpoints, got: %s", err.Error()) + containsAny(err.Error(), []string{"failed to create session", "DKLS", "dkls", "session", "setup message", "no endpoints"}), + "error should be about setup binding, session creation or endpoints, got: %s", err.Error()) } func TestVerifySigningRequest_OutboundDisabled(t *testing.T) { @@ -1453,3 +1455,213 @@ func TestNonceBounds(t *testing.T) { require.Error(t, err) }) } + +// A follower verifies UnsignedSigningReq.SigningHash, but DKLS signs the hash +// embedded in Message.Payload, and the two arrive unbound. Without this check a +// coordinator can present a legitimate hash for verification and embed an +// attacker-chosen one in the setup, harvesting honest shares over it. +func TestSetupBindsHash(t *testing.T) { + participantIDs := []byte("party1\x00party2") + keyID := make([]byte, 32) + + legitHash := make([]byte, 32) + copy(legitHash, "legitimate-outbound-hash-32bytes") + attackerHash := make([]byte, 32) + copy(attackerHash, "attacker-chosen-vault-call-digest") + + legitSetup, err := session.DklsSignSetupMsgNew(keyID, nil, legitHash, participantIDs) + require.NoError(t, err) + attackerSetup, err := session.DklsSignSetupMsgNew(keyID, nil, attackerHash, participantIDs) + require.NoError(t, err) + + t.Run("accepts setup that signs the verified hash", func(t *testing.T) { + require.NoError(t, setupBindsHash(legitSetup, legitHash)) + }) + + // The reported attack. + t.Run("rejects setup embedding a different hash", func(t *testing.T) { + err := setupBindsHash(attackerSetup, legitHash) + require.Error(t, err) + assert.Contains(t, err.Error(), "setup message signs hash") + }) + + t.Run("rejects undecodable setup", func(t *testing.T) { + require.Error(t, setupBindsHash([]byte("not-a-dkls-setup"), legitHash)) + require.Error(t, setupBindsHash(nil, legitHash)) + }) + + t.Run("rejects missing verified hash", func(t *testing.T) { + err := setupBindsHash(legitSetup, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "no verified signing hash") + }) +} + +// Keygen, keyrefresh and quorumchange have the same split as the sign path: we +// validate msg.Participants, but the session runs on the list embedded in +// Payload. The threshold cannot be bound this way, see verifySetupBindsParticipants. +func TestSetupBindsParticipants(t *testing.T) { + validated := []string{"validator1", "validator2", "validator3"} + encode := func(ids []string) []byte { + return []byte(strings.Join(ids, "\x00")) + } + + legitSetup, err := session.DklsKeygenSetupMsgNew(2, nil, encode(validated)) + require.NoError(t, err) + + t.Run("accepts setup with the validated participants", func(t *testing.T) { + require.NoError(t, setupBindsParticipants(legitSetup, validated)) + }) + + t.Run("rejects setup with a substituted participant", func(t *testing.T) { + swapped, err := session.DklsKeygenSetupMsgNew(2, nil, + encode([]string{"validator1", "validator2", "attacker"})) + require.NoError(t, err) + err = setupBindsParticipants(swapped, validated) + require.Error(t, err) + assert.Contains(t, err.Error(), "do not match validated participants") + }) + + t.Run("rejects setup with a dropped participant", func(t *testing.T) { + fewer, err := session.DklsKeygenSetupMsgNew(2, nil, + encode([]string{"validator1", "validator2"})) + require.NoError(t, err) + require.Error(t, setupBindsParticipants(fewer, validated)) + }) + + // Index order is part of the protocol, so a reorder is as consequential as + // a substitution. + t.Run("rejects reordered participants", func(t *testing.T) { + reordered, err := session.DklsKeygenSetupMsgNew(2, nil, + encode([]string{"validator3", "validator2", "validator1"})) + require.NoError(t, err) + require.Error(t, setupBindsParticipants(reordered, validated)) + }) + + t.Run("rejects undecodable setup and missing validated list", func(t *testing.T) { + require.Error(t, setupBindsParticipants([]byte("not-a-setup"), validated)) + require.Error(t, setupBindsParticipants(nil, validated)) + require.Error(t, setupBindsParticipants(legitSetup, nil)) + }) +} + +// The regression the finding asks for: a Payload whose embedded hash differs +// from the verified SigningHash must refuse session creation and produce no +// shares. Driven through handleSetupMessage so it covers the wiring, not just +// the comparison helper. +func TestHandleSetupMessage_RejectsPayloadHashMismatch(t *testing.T) { + _, coord, evtStore, keyshareMgr, _, testDB := setupTestSessionManager(t) + ctx := context.Background() + + // Sign-eligible validators are the ACTIVE ones in the fixture. + participants := []string{"validator1", "validator2"} + participantIDs := []byte(strings.Join(participants, "\x00")) + keyID := make([]byte, 32) + + legitHash := make([]byte, 32) + copy(legitHash, "legitimate-outbound-hash-32bytes") + attackerHash := make([]byte, 32) + copy(attackerHash, "attacker-chosen-vault-call-digest") + + newRecordingSM := func() (*SessionManager, *int) { + sends := 0 + sm := NewSessionManager( + evtStore, coord, keyshareMgr, nil, nil, + func(context.Context, string, []byte) error { sends++; return nil }, + "validator1", 3*time.Minute, 30*time.Second, 60, zerolog.Nop(), nil, + ) + return sm, &sends + } + + newEvent := func(t *testing.T, id string) { + t.Helper() + require.NoError(t, testDB.Create(&store.Event{ + EventID: id, BlockHeight: 100, + Type: store.EventTypeSignOutbound, + Status: store.StatusConfirmed, + EventData: []byte(`{"destination_chain":"eip155:11155111"}`), + }).Error) + } + + t.Run("substituted payload hash is refused, no session, no shares", func(t *testing.T) { + newEvent(t, "sign-mismatch") + sm, sends := newRecordingSM() + + // Coordinator shows the legitimate hash but ships a setup over its own. + attackerSetup, err := session.DklsSignSetupMsgNew(keyID, nil, attackerHash, participantIDs) + require.NoError(t, err) + + err = sm.HandleIncomingMessage(ctx, "peer1", &coordinator.Message{ + Type: coordinator.MessageTypeSetup, + EventID: "sign-mismatch", + Participants: participants, + Payload: attackerSetup, + UnsignedSigningReq: &common.UnsignedSigningReq{SigningHash: legitHash, Nonce: 1}, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "setup message signs hash") + + sm.mu.RLock() + sessionCount := len(sm.sessions) + sm.mu.RUnlock() + assert.Zero(t, sessionCount, "no session may be created for a mismatched setup") + assert.Zero(t, *sends, "no ACK or share may be emitted for a mismatched setup") + }) + + // Positive control: with the same wiring, a setup over the verified hash must + // get past the binding check, so the rejection above is the binding and not + // some earlier validation failing. + t.Run("matching payload hash passes the binding check", func(t *testing.T) { + newEvent(t, "sign-match") + sm, _ := newRecordingSM() + + legitSetup, err := session.DklsSignSetupMsgNew(keyID, nil, legitHash, participantIDs) + require.NoError(t, err) + + err = sm.HandleIncomingMessage(ctx, "peer1", &coordinator.Message{ + Type: coordinator.MessageTypeSetup, + EventID: "sign-match", + Participants: participants, + Payload: legitSetup, + UnsignedSigningReq: &common.UnsignedSigningReq{SigningHash: legitHash, Nonce: 1}, + }) + + if err != nil { + assert.NotContains(t, err.Error(), "setup message signs hash", + "matching setup must not be rejected by the hash binding") + assert.NotContains(t, err.Error(), "do not match validated participants", + "matching setup must not be rejected by the participant binding") + } + }) +} + +// The session constructors accept a threshold and ignore it, so the setup blob's +// embedded threshold is what the protocol runs with. A coordinator embedding a +// lower one would elicit help producing a weaker key than the participants +// agreed to, which is worse than a bad signature since it persists. +func TestSetupBindsThreshold(t *testing.T) { + validated := []string{"validator1", "validator2", "validator3"} + expected := coordinator.CalculateThreshold(len(validated)) + encode := func(ids []string) []byte { return []byte(strings.Join(ids, "\x00")) } + + t.Run("accepts the expected threshold", func(t *testing.T) { + setup, err := session.DklsKeygenSetupMsgNew(expected, nil, encode(validated)) + require.NoError(t, err) + require.NoError(t, setupBindsThreshold(setup, validated)) + }) + + t.Run("rejects a downgraded threshold", func(t *testing.T) { + require.Greater(t, expected, 1, "fixture must allow a strictly lower threshold") + downgraded, err := session.DklsKeygenSetupMsgNew(expected-1, nil, encode(validated)) + require.NoError(t, err) + err = setupBindsThreshold(downgraded, validated) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not match expected") + }) + + t.Run("rejects undecodable setup", func(t *testing.T) { + require.Error(t, setupBindsThreshold([]byte("not-a-setup"), validated)) + require.Error(t, setupBindsThreshold(nil, validated)) + }) +}