Validate signing round 9 decommitments as curve points

BNB_HARDENING_INTEGRATION.md

@@ -27,6 +27,7 @@ This is a protocol/wire compatibility break for proof transcripts. Proofs whose
 - `fc38979`, GG20 SSID uniqueness: ported `tss.Parameters.SessionNonce` / `SetSessionNonce`, added `SetSessionNonceBytes` (requires session IDs of at least 16 bytes), and wired ECDSA keygen/signing SSID derivation. Keygen and signing now require a positive `SetSessionNonce` and fail closed if it is not set — the previous zero fallback (keygen) and SHA512_256(messageBytes) fallback (signing) caused two ceremonies with otherwise-identical inputs to derive the same SSID, breaking the session-binding property the proofs rely on.
 - `685c2af`, canonical EC coordinates: ported rejection of EC coordinates outside `[0, P)`.
 - Post-review cleanup: party-specific proof contexts now append fixed-width uint64 party indexes so party 0 does not collapse to the bare SSID. The earlier signing default that derived an SSID nonce from full message bytes has been removed in favour of the fail-closed requirement above; the helpers that produced it (`messageBytes`, `messageSessionNonce`) are gone with it.
+- Post-review cleanup: signing round 9 now validates decommitted `U_j`/`T_j` coordinates as canonical curve points before group addition (previously they reached `elliptic.Curve.Add` raw, which panics on off-curve points for Go's stdlib curves and yields undefined coordinates for btcec) and attributes the failure to the sender; the unattributable `U != T` abort no longer blames the honest reporting party. Signing round 1 also rejects nil and negative messages up front instead of panicking or failing late in finalize.
 
 ## Already Covered Or Superseded
 
@@ -70,6 +71,7 @@ Added or updated focused tests cover:
 - VSS `threshold+1` verification/reconstruction behavior.
 - Non-canonical EC coordinate rejection.
 - ECDSA leading-zero message signing.
+- Signing round 9 rejection of off-curve and identity-summing `U_j`/`T_j` decommitments with sender attribution, culprit-free `U != T` aborts, and round 1 rejection of nil/negative messages.
 
 ## Residual Risks
 

ecdsa/signing/round_1.go

@@ -38,7 +38,10 @@ func (round *round1) Start() *tss.Error {
 	// but considered different blockchain use different hash function we accept the converted big.Int
 	// if this big.Int is not belongs to Zq, the client might not comply with common rule (for ECDSA):
 	// https://github.com/btcsuite/btcd/blob/c26ffa870fd817666a857af1bf6498fabba1ffe3/btcec/signature.go#L263
-	if round.temp.m.Cmp(round.Params().EC().Params().N) >= 0 {
+	// A nil message would otherwise panic on Cmp, and a negative one would only
+	// surface as an unattributed signature-verification failure in finalize.
+	if round.temp.m == nil || round.temp.m.Sign() < 0 ||
+		round.temp.m.Cmp(round.Params().EC().Params().N) >= 0 {
 		return round.WrapError(errors.New("hashed message is not valid"))
 	}
 

ecdsa/signing/round_9.go

@@ -9,6 +9,9 @@ package signing
 import (
 	"errors"
 
+	errors2 "github.com/pkg/errors"
+
+	"github.com/bnb-chain/tss-lib/crypto"
 	"github.com/bnb-chain/tss-lib/crypto/commitments"
 	"github.com/bnb-chain/tss-lib/tss"
 )
@@ -21,8 +24,8 @@ func (round *round9) Start() *tss.Error {
 	round.started = true
 	round.resetOK()
 
-	UX, UY := round.temp.Ui.X(), round.temp.Ui.Y()
-	TX, TY := round.temp.Ti.X(), round.temp.Ti.Y()
+	U := round.temp.Ui
+	T := round.temp.Ti
 	for j, Pj := range round.Parties().IDs() {
 		if j == round.PartyID().Index {
 			continue
@@ -34,14 +37,36 @@ func (round *round9) Start() *tss.Error {
 		cmt := commitments.HashCommitDecommit{C: cj, D: dj}
 		ok, values := cmt.DeCommit()
 		if !ok || len(values) != 4 {
-			return round.WrapError(errors.New("de-commitment for bigVj and bigAj failed"), Pj)
+			return round.WrapError(errors.New("de-commitment for bigUj and bigTj failed"), Pj)
+		}
+		// The decommitted coordinates are adversarial wire data; validate them
+		// as canonical curve points before any group operation. Go's stdlib
+		// curves panic on off-curve inputs to Add, and btcec returns undefined
+		// coordinates, which would have turned a malformed decommitment into a
+		// crash or an unattributed U != T abort.
+		bigUj, err := crypto.NewECPoint(round.Params().EC(), values[0], values[1])
+		if err != nil {
+			return round.WrapError(errors2.Wrapf(err, "NewECPoint(bigUj)"), Pj)
+		}
+		bigTj, err := crypto.NewECPoint(round.Params().EC(), values[2], values[3])
+		if err != nil {
+			return round.WrapError(errors2.Wrapf(err, "NewECPoint(bigTj)"), Pj)
+		}
+		U, err = U.Add(bigUj)
+		if err != nil {
+			return round.WrapError(errors2.Wrapf(err, "U.Add(bigUj)"), Pj)
+		}
+		T, err = T.Add(bigTj)
+		if err != nil {
+			return round.WrapError(errors2.Wrapf(err, "T.Add(bigTj)"), Pj)
 		}
-		UjX, UjY, TjX, TjY := values[0], values[1], values[2], values[3]
-		UX, UY = round.Params().EC().Add(UX, UY, UjX, UjY)
-		TX, TY = round.Params().EC().Add(TX, TY, TjX, TjY)
 	}
-	if UX.Cmp(TX) != 0 || UY.Cmp(TY) != 0 {
-		return round.WrapError(errors.New("U doesn't equal T"), round.PartyID())
+	// A mismatch here proves some party misbehaved in phase 5 but does not
+	// identify which one, so no culprit is attributed. The previous behaviour
+	// blamed the honest reporting party itself, which would misdirect any
+	// orchestration layer that acts on culprits.
+	if !U.Equals(T) {
+		return round.WrapError(errors.New("U doesn't equal T"))
 	}
 
 	r9msg := NewSignRound9Message(round.PartyID(), round.temp.si)

ecdsa/signing/round_9_test.go

@@ -0,0 +1,167 @@
+// Copyright © 2019 Binance
+//
+// This file is part of Binance. The full Binance copyright notice, including
+// terms governing use, modification, and redistribution, is contained in the
+// file LICENSE at the root of the source code distribution tree.
+
+package signing
+
+import (
+	"math/big"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/bnb-chain/tss-lib/common"
+	"github.com/bnb-chain/tss-lib/crypto"
+	"github.com/bnb-chain/tss-lib/crypto/commitments"
+	"github.com/bnb-chain/tss-lib/ecdsa/keygen"
+	"github.com/bnb-chain/tss-lib/tss"
+)
+
+// newRound9ForTest builds a two-party round 9 with this party's contribution
+// fixed to Ui = Ti = G, ready to consume a crafted commit/decommit pair from
+// the peer at index 1.
+func newRound9ForTest(t *testing.T) (*round9, tss.SortedPartyIDs) {
+	t.Helper()
+
+	pIDs := tss.GenerateTestPartyIDs(2)
+	params := tss.NewParameters(tss.S256(), tss.NewPeerContext(pIDs), pIDs[0], len(pIDs), 1)
+	keys := keygen.NewLocalPartySaveData(len(pIDs))
+	data := common.SignatureData{}
+	temp := localTempData{}
+	temp.signRound7Messages = make([]tss.ParsedMessage, len(pIDs))
+	temp.signRound8Messages = make([]tss.ParsedMessage, len(pIDs))
+	temp.signRound9Messages = make([]tss.ParsedMessage, len(pIDs))
+	out := make(chan tss.Message, len(pIDs))
+	end := make(chan common.SignatureData, len(pIDs))
+
+	g := crypto.ScalarBaseMult(params.EC(), big.NewInt(1))
+	temp.Ui = g
+	temp.Ti = g
+	temp.si = big.NewInt(1)
+
+	rnd := &round9{&round8{&round7{&round6{&round5{&round4{&round3{&round2{&round1{
+		&base{params, &keys, &data, &temp, out, end, make([]bool, len(pIDs)), false, 8},
+	}}}}}}}}}
+	return rnd, pIDs
+}
+
+// storePeerDecommitment commits to the four given coordinates as the peer's
+// (Uj, Tj) decommitment for round 9.
+func storePeerDecommitment(rnd *round9, from *tss.PartyID, values ...*big.Int) {
+	cmt := commitments.NewHashCommitment(values...)
+	rnd.temp.signRound7Messages[1] = NewSignRound7Message(from, cmt.C)
+	rnd.temp.signRound8Messages[1] = NewSignRound8Message(from, cmt.D)
+}
+
+// TestRound9_RejectsMalformedDecommitments pins that decommitted U_j/T_j
+// coordinates are validated as canonical curve points before any group
+// operation, with the failure attributed to the sending party. Previously the
+// raw coordinates went straight into elliptic.Curve.Add, which panics on
+// off-curve points for Go's stdlib curves and yields undefined coordinates for
+// btcec — and the resulting U != T abort blamed the honest reporting party.
+func TestRound9_RejectsMalformedDecommitments(t *testing.T) {
+	g2 := crypto.ScalarBaseMult(tss.S256(), big.NewInt(2))
+	gNeg := crypto.ScalarBaseMult(tss.S256(), new(big.Int).Sub(tss.S256().Params().N, big.NewInt(1)))
+
+	tests := []struct {
+		name    string
+		values  func() []*big.Int
+		wantErr string
+	}{
+		{
+			name: "off-curve Uj",
+			values: func() []*big.Int {
+				return []*big.Int{big.NewInt(1), big.NewInt(2), g2.X(), g2.Y()}
+			},
+			wantErr: "NewECPoint(bigUj)",
+		},
+		{
+			name: "off-curve Tj",
+			values: func() []*big.Int {
+				return []*big.Int{g2.X(), g2.Y(), big.NewInt(3), big.NewInt(4)}
+			},
+			wantErr: "NewECPoint(bigTj)",
+		},
+		{
+			// Uj = -G cancels this party's Ui = G; the sum is the point at
+			// infinity, which has no affine encoding and must be rejected.
+			name: "Uj sums to identity",
+			values: func() []*big.Int {
+				return []*big.Int{gNeg.X(), gNeg.Y(), g2.X(), g2.Y()}
+			},
+			wantErr: "U.Add(bigUj)",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rnd, pIDs := newRound9ForTest(t)
+			storePeerDecommitment(rnd, pIDs[1], tt.values()...)
+
+			err := rnd.Start()
+			if assert.NotNil(t, err, "round 9 must reject the malformed decommitment") {
+				assert.Contains(t, err.Error(), tt.wantErr)
+				assert.Equal(t, []*tss.PartyID{pIDs[1]}, err.Culprits(), "the sender must be attributed")
+			}
+		})
+	}
+}
+
+// TestRound9_UTMismatchHasNoCulprit pins that a U != T abort carries no
+// culprit: the mismatch proves some party misbehaved in phase 5 but does not
+// identify which one, and the previous self-attribution would have misdirected
+// orchestration layers that act on culprits.
+func TestRound9_UTMismatchHasNoCulprit(t *testing.T) {
+	rnd, pIDs := newRound9ForTest(t)
+	g2 := crypto.ScalarBaseMult(tss.S256(), big.NewInt(2))
+	g3 := crypto.ScalarBaseMult(tss.S256(), big.NewInt(3))
+	// U = G + 2G = 3G but T = G + 3G = 4G
+	storePeerDecommitment(rnd, pIDs[1], g2.X(), g2.Y(), g3.X(), g3.Y())
+
+	err := rnd.Start()
+	if assert.NotNil(t, err, "round 9 must abort on U != T") {
+		assert.Contains(t, err.Error(), "U doesn't equal T")
+		assert.Empty(t, err.Culprits(), "an unattributable abort must not name a culprit")
+	}
+}
+
+func TestRound9_ConsistentDecommitmentsSucceed(t *testing.T) {
+	rnd, pIDs := newRound9ForTest(t)
+	g2 := crypto.ScalarBaseMult(tss.S256(), big.NewInt(2))
+	// U = G + 2G = T
+	storePeerDecommitment(rnd, pIDs[1], g2.X(), g2.Y(), g2.X(), g2.Y())
+
+	err := rnd.Start()
+	assert.Nil(t, err)
+	assert.NotNil(t, rnd.temp.signRound9Messages[0], "round 9 message must be produced")
+}
+
+// TestSigning_Start_RejectsInvalidMessage pins the round-1 message validity
+// check: a nil message must fail cleanly instead of panicking on Cmp, and a
+// negative message must fail at Start instead of surfacing as an unattributed
+// signature-verification failure in finalize.
+func TestSigning_Start_RejectsInvalidMessage(t *testing.T) {
+	setUp("info")
+	keys, signPIDs, err := keygen.LoadKeygenTestFixturesRandomSet(testThreshold+1, testParticipants)
+	assert.NoError(t, err, "should load keygen fixtures")
+
+	for _, msg := range []*big.Int{nil, big.NewInt(-42)} {
+		p2pCtx := tss.NewPeerContext(signPIDs)
+		outCh := make(chan tss.Message, len(signPIDs))
+		endCh := make(chan common.SignatureData, len(signPIDs))
+
+		params := tss.NewParameters(tss.S256(), p2pCtx, signPIDs[0], len(signPIDs), testThreshold)
+		params.SetSessionNonce(big.NewInt(1))
+
+		P := NewLocalParty(msg, params, keys[0], outCh, endCh, 32).(*LocalParty)
+		tssErr := P.Start()
+		if tssErr == nil {
+			t.Fatalf("Start must return an error for message %v", msg)
+		}
+		if !strings.Contains(tssErr.Error(), "hashed message is not valid") {
+			t.Fatalf("error must reject the message, got: %v", tssErr)
+		}
+	}
+}