Validate signing round 9 decommitments as curve points

CHANGELOG.md

@@ -33,6 +33,7 @@ belongs to PR #2 (the base BNB hardening integration) unless it is tagged with a
 - **PR #4** — BNB #332 tBTC-relevant hardening backport (stacked on PR #2).
 - **PR #5** — removal of EdDSA and ECDSA resharing protocols (stacked on PR #4).
 - **PR #6** — remaining BNB cryptographic hardening follow-ups (stacked on PR #5).
+- **PR #7** — signing round-9 decommitment validation and related fixes (stacked on PR #6).
 
 ### ⚠️ Compatibility — read before upgrading
 
@@ -277,6 +278,24 @@ rejecting input that an honest caller would previously have produced.
   bytes are made deterministic; large-modulus `sampleYModN` block indexing is fixed; and
   canonical-generator checks were added in `crypto/commitments` and `crypto/paillier`.
   _Provenance: `BNB #332` + `threshold-original`, PR #6._
+- **ECDSA signing round-9 decommitment curve-point validation (PR #7):** decommitted
+  `Uj`/`Tj` coordinates are now validated as canonical curve points (`crypto.NewECPoint`)
+  before any group operation, with failures attributed to the sending party
+  (`ecdsa/signing/round_9.go`). Previously off-curve coordinates went straight into
+  `elliptic.Curve.Add`, which panics for Go's stdlib curves and yields undefined coordinates
+  for btcec — turning a malformed decommitment into a crash or an unattributed `U != T` abort
+  that blamed the honest reporter. Layered on PR #4's `decommitFour` length guard. Honest
+  decommitments are unaffected. _Provenance: `BNB #332`, PR #7._
+- **ECDSA signing round-1 message-range validation (PR #7):** signing `Start()` now rejects a
+  nil, negative, or `>= curve order` hashed message instead of panicking on `Cmp` (nil) or
+  surfacing later as an unattributed finalize verification failure (negative)
+  (`ecdsa/signing/round_1.go`). Honest callers passing a hash in `[0, N)` are unaffected.
+  _Provenance: `threshold-original`, PR #7._
+- **Paillier FactorVerify distinct-generator check (PR #7):** `FactorVerify` rejects equal
+  Pedersen bases (`s == t`), under which the binding degenerates, mirroring the
+  distinct-generator policy already enforced by DLN and MtA proofs
+  (`crypto/paillier/factor_proof.go`). Honest setups use distinct generators.
+  _Provenance: `threshold-original`, PR #7._
 
 ### Added
 

crypto/mta/proofs.go

@@ -98,7 +98,7 @@ func ProveBobWC(ec elliptic.Curve, pk *paillier.PublicKey, NTilde, h1, h2, c1, c
 
 	// 11-12. e'
 	var e *big.Int
-	{ // must use RejectionSample
+	{ // derive the Fiat-Shamir challenge by reducing the hash mod q
 		var eHash *big.Int
 		// X is nil if called by ProveBob (Bob's proof "without check")
 		if X == nil {
@@ -293,7 +293,7 @@ func (pf *ProofBobWC) Verify(ec elliptic.Curve, pk *paillier.PublicKey, NTilde,
 
 	// 1-2. e'
 	var e *big.Int
-	{ // must use RejectionSample
+	{ // derive the Fiat-Shamir challenge by reducing the hash mod q
 		var eHash *big.Int
 		// X is nil if called on a ProveBob (Bob's proof "without check")
 		if X == nil {

crypto/paillier/factor_proof.go

@@ -114,6 +114,13 @@ func (pf FactorProof) FactorVerify(pkN, N, s, t *big.Int, session ...[]byte) (bo
 		}
 	}
 
+	// The Pedersen-style bases s and t must be distinct; with s == t the
+	// binding degenerates. Sibling proofs (dlnproof, MtA range/respondent)
+	// reject equal generators, so mirror that policy here.
+	if s.Cmp(t) == 0 {
+		return false, fmt.Errorf("fac proof verify: generators s and t must be distinct")
+	}
+
 	limit := big.NewInt(1)
 	limit.Lsh(limit, PARAM_L+PARAM_E)
 	limit.Mul(limit, new(big.Int).Sqrt(pkN))

crypto/paillier/factor_proof_test.go

@@ -131,6 +131,18 @@ func TestFactorProofVerifyFail3(t *testing.T) {
 	assert.False(t, res, "proof verify result must be false")
 }
 
+func TestFactorProofVerifyRejectsEqualGenerators(t *testing.T) {
+	facSetUp(t)
+	// A proof built with identical Pedersen bases (s == t) is internally
+	// self-consistent and would otherwise verify, but equal generators make
+	// the commitment binding degenerate. Sibling proofs (dlnproof, MtA
+	// range/respondent) reject equal generators, so FactorVerify must too.
+	proof := privateKey.FactorProof(auxPrime.N, s, s)
+	res, err := proof.FactorVerify(publicKey.N, auxPrime.N, s, s)
+	assert.Error(t, err)
+	assert.False(t, res, "proof with s == t must be rejected")
+}
+
 func TestFactorProofVerifyRejectsNonInvertibleBase(t *testing.T) {
 	facSetUp(t)
 	proof := privateKey.FactorProof(auxPrime.N, s, tt)

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

@@ -10,6 +10,9 @@ import (
 	"errors"
 	"math/big"
 
+	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"
 )
@@ -35,8 +38,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
@@ -47,14 +50,36 @@ func (round *round9) Start() *tss.Error {
 		cj, dj := r7msg.UnmarshalCommitment(), r8msg.UnmarshalDeCommitment()
 		values, ok := decommitFour(commitments.HashCommitDecommit{C: cj, D: dj})
 		if !ok {
-			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

@@ -8,11 +8,16 @@ 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"
 )
 
 func TestDecommitFour(t *testing.T) {
@@ -54,3 +59,149 @@ func TestDecommitFour(t *testing.T) {
 		assert.False(t, ok)
 	})
 }
+
+// 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)
+		}
+	}
+}