From 11be493f0179800e0f2063833cffa081365e160e Mon Sep 17 00:00:00 2001 From: maclane Date: Tue, 9 Jun 2026 22:29:44 -0400 Subject: [PATCH 1/4] Validate signing round 9 decommitments as curve points Round 9 was the last place where adversarial wire data reached raw elliptic.Curve.Add without on-curve validation: a malformed U_j/T_j decommitment panics Go stdlib curves and yields undefined coordinates on btcec, and the resulting U != T abort blamed the honest reporting party itself. Validate the decommitted coordinates via crypto.NewECPoint, attribute failures to the sender, and report the unattributable U != T mismatch without a culprit. Also reject nil and negative messages in signing round 1: nil previously panicked on Cmp inside the protocol goroutine, and negative values only surfaced as an unattributed verification failure in finalize. Co-Authored-By: Claude Fable 5 --- ecdsa/signing/round_1.go | 5 +- ecdsa/signing/round_9.go | 41 +++++++-- ecdsa/signing/round_9_test.go | 151 ++++++++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 9 deletions(-) diff --git a/ecdsa/signing/round_1.go b/ecdsa/signing/round_1.go index 1b935530..779d7da6 100644 --- a/ecdsa/signing/round_1.go +++ b/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")) } diff --git a/ecdsa/signing/round_9.go b/ecdsa/signing/round_9.go index 935a7104..f56f9ffd 100644 --- a/ecdsa/signing/round_9.go +++ b/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) diff --git a/ecdsa/signing/round_9_test.go b/ecdsa/signing/round_9_test.go index 1a754b8a..a0963956 100644 --- a/ecdsa/signing/round_9_test.go +++ b/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) + } + } +} From d801db6d0f9f787e4c01a43189907edc810a75b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 15 Jun 2026 07:15:24 +0000 Subject: [PATCH 2/4] docs(mta): correct stale RejectionSample challenge comments The block comments above the Fiat-Shamir challenge derivation claimed rejection sampling, but the code uses common.ModReduceHash (modular reduction). Describe the actual behaviour to avoid misleading auditors. --- crypto/mta/proofs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/mta/proofs.go b/crypto/mta/proofs.go index 9c36e25d..6536339d 100644 --- a/crypto/mta/proofs.go +++ b/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 { From ec8015156bdb3a8071443b2d92739de3834f4ee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 15 Jun 2026 07:17:28 +0000 Subject: [PATCH 3/4] fix(paillier): reject equal generators in FactorVerify FactorVerify required s and t to be canonical generators but not distinct. With s == t the Pedersen-style commitment binding degenerates and a self-consistent proof over equal bases verifies as valid. Sibling proofs (dlnproof, MtA range/respondent) already reject equal generators; mirror that policy here as defense-in-depth. Add a regression test that builds a proof with s == t and confirms it is rejected (it verifies as true without the guard). --- crypto/paillier/factor_proof.go | 7 +++++++ crypto/paillier/factor_proof_test.go | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/crypto/paillier/factor_proof.go b/crypto/paillier/factor_proof.go index 57cc8b1e..cafb1c8b 100644 --- a/crypto/paillier/factor_proof.go +++ b/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)) diff --git a/crypto/paillier/factor_proof_test.go b/crypto/paillier/factor_proof_test.go index e8b45ce6..6294a3ea 100644 --- a/crypto/paillier/factor_proof_test.go +++ b/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) From 3af3ee0467bc1f6224e2c328a2a3bcf840ad48e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 15 Jun 2026 09:43:24 +0000 Subject: [PATCH 4/4] docs(changelog): document PR #7 round-9 and proof fixes Record PR #7's non-breaking hardening: signing round-9 decommitment curve-point validation (layered on PR #4's decommitFour guard), round-1 hashed-message range validation, and the Paillier FactorVerify distinct-generator check. Add PR #7 to the composing-PRs list. No new breaking changes. --- CHANGELOG.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b380d59..2e5902eb 100644 --- a/CHANGELOG.md +++ b/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