Port BNB cryptographic hardening follow-ups

CHANGELOG.md

@@ -32,6 +32,7 @@ belongs to PR #2 (the base BNB hardening integration) unless it is tagged with a
 - **PR #2** — base BNB hardening integration.
 - **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).
 
 ### ⚠️ Compatibility — read before upgrading
 
@@ -120,13 +121,48 @@ Two new caller obligations are enforced at runtime (see Breaking Changes 1 and 2
   so in practice this is active on the protocol path.
 - **Migration:** Covered by the coordinated upgrade in Breaking Change 1/3.
 
-> Every session / `fullBytesLen` parameter was added as a trailing variadic argument, so the
-> hardening itself changed no exported signatures (verified by diffing exported signatures
-> between base and HEAD); those breaks are runtime/wire, not compile-time. The source/compile
-> breaks in this set come from PR #5's protocol removal, which deleted the exported
+#### 5. Per-proof-system Fiat-Shamir domain tags (PR #6)
+- **What:** DLN, Schnorr, MtA, and Paillier challenges now prepend a per-proof-system domain
+  tag (e.g. `dlnproof|`, `zk|`, `zkv|`, via `fsDomainTag*` / `fsSession*`) to the session
+  before tagged hashing. This further changes every proof transcript relative to Breaking
+  Change 3.
+- **Break type:** Wire/protocol — compounds Breaking Change 3; still a single coordinated
+  upgrade (a PR #6 node and a PR #2–#5 node will not cross-verify).
+- **Motivation:** Distinct domain separation per proof system, so a challenge from one proof
+  type can never be reused in another.
+- **Provenance:** `BNB #252` / `BNB #256` domain-tag design; PR #6.
+- **Migration:** Covered by the coordinated upgrade in Breaking Change 1/3.
+
+#### 6. `ecdsa/signing.PrepareForSigning` returns an error (PR #6)
+- **What:** the exported signature changed from `(wi, bigWs)` to `(wi, bigWs, err)`; it now
+  validates its inputs and returns an error instead of proceeding on malformed data
+  (`ecdsa/signing/prepare.go`).
+- **Break type:** Source/compile — downstream callers must handle the third return value.
+- **Motivation:** Surface invalid signing-preparation inputs instead of producing corrupt
+  signing state.
+- **Provenance:** BNB hardening follow-ups; PR #6. (A code search found no current
+  `threshold-network/keep-core` callers.)
+- **Migration:** Update call sites to handle the returned `error`.
+
+#### 7. Stricter `tss.NewParameters` and `SortPartyIDs` validation (PR #6)
+- **What:** `NewParameters` now panics on a party count below 2, a threshold outside
+  `[1, partyCount)`, a `PartyID` key congruent to 0 mod q, or two `PartyID`s colliding mod q;
+  `SortPartyIDs` panics on duplicate raw party keys (`tss/params.go`, `tss/party_id.go`).
+- **Break type:** Runtime — rejects previously-accepted but invalid/degenerate party sets.
+  Honest setups with ≥2 distinct, non-colliding parties and a valid threshold are unaffected.
+- **Motivation:** Fail fast on malformed party sets that would otherwise corrupt VSS or the
+  protocol.
+- **Provenance:** `threshold-original` / BNB hardening; PR #6.
+- **Migration:** Ensure ceremonies use ≥2 distinct parties, a threshold in `[1, partyCount)`,
+  and non-colliding keys (normal configurations already satisfy this).
+
+> Source/compile breaks in this set: `ecdsa/signing.PrepareForSigning` gained an `error` return
+> (Breaking Change 6, PR #6), and PR #5's protocol removal deleted the exported
 > `tss.ReSharingParameters` / `tss.NewReSharingParameters`, `crypto.ECPoint.EightInvEight`, and
-> `ecdsa/resharing.NewDGRound1Message` API (see Removed). Downstream code using EdDSA, ECDSA
-> resharing, or those symbols must adapt.
+> `ecdsa/resharing.NewDGRound1Message` API (see Removed). Otherwise every session /
+> `fullBytesLen` parameter was added as a trailing variadic argument, so all remaining call
+> sites compile unchanged; those breaks are runtime/wire. Verified by diffing exported
+> signatures between base and HEAD.
 
 ### Removed
 
@@ -227,6 +263,20 @@ rejecting input that an honest caller would previously have produced.
 - **ECDSA signing round-4 nil theta-inverse guard (PR #4):** a non-invertible theta
   (`ModInverse` returning nil) is rejected with a clean error instead of propagating nil
   (`ecdsa/signing/round_4.go`). _Provenance: `BNB #332`, PR #4._
+- **Shared cryptographic input validators (PR #6):** `common/validation.go` adds reusable
+  canonical checks for unknown-order moduli, generators, and Paillier ciphertexts, wired into
+  the proof verifiers and round handlers. _Provenance: `BNB #252`/`BNB #332`, PR #6._
+- **VSS reconstruction input validation (PR #6):** `feldman_vss` rejects malformed
+  reconstruction inputs and out-of-bound parameters before use (`crypto/vss/feldman_vss.go`).
+  _Provenance: `BNB #332`, PR #6._
+- **Idempotent message redelivery (PR #6):** keygen/signing message storage treats an
+  identical redelivery from a party as a no-op while rejecting a content-different replay,
+  preventing duplicate-message state corruption (`tss/message.go`). _Provenance: `threshold-original`, PR #6._
+- **Review follow-up correctness fixes (PR #6):** Schnorr verification accepts unregistered
+  generic curves; `common.GetRandomInt`'s zero-inclusive range is corrected; message wire
+  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._
 
 ### Added
 

common/hash_utils.go

@@ -21,14 +21,31 @@ func LiterallyJustMod(q *big.Int, eHash *big.Int) *big.Int { // e' = eHash
 	return e
 }
 
-// RejectionSample preserves the upstream challenge-reduction function name.
-// This implementation reduces the hash modulo q rather than looping with fresh
-// hash material, so callers must only use it where modular-reduction bias is
-// acceptable for the proof challenge.
-func RejectionSample(q *big.Int, eHash *big.Int) *big.Int {
+// RejectionSample reduces `eHash` modulo q. The name preserves the upstream
+// challenge-derivation function name; this is NOT true rejection sampling
+// (no loop with fresh hash material until a candidate falls in [0, q)).
+//
+// Safe only when q is close to 2^k from below, where k is the hash output
+// width in bits (k = 256 for SHA512_256). For secp256k1 the bias is ≈ 2^-128
+// — negligible for Fiat-Shamir challenges.
+//
+// For q that is NOT close to a power of 2, or for moduli larger than 2^k
+// (e.g. Paillier N ≈ 2^2048), use HashToN / HashToNTagged: those absorb
+// ≥ k + 256 bits of entropy before reduction and bound bias at ≤ 2^-256
+// regardless of q. For applications requiring an unbiased uniform sample
+// over [0, q), implement true rejection sampling at the call site.
+func ModReduceHash(q *big.Int, eHash *big.Int) *big.Int {
 	return LiterallyJustMod(q, eHash)
 }
 
+// RejectionSample is kept for compatibility with older callers.
+//
+// Deprecated: use ModReduceHash. This function is modular reduction, not true
+// rejection sampling.
+func RejectionSample(q *big.Int, eHash *big.Int) *big.Int {
+	return ModReduceHash(q, eHash)
+}
+
 // Return a big.Int between 0 and N
 func HashToN(N *big.Int, in ...*big.Int) *big.Int {
 	bitCnt := N.BitLen()

common/int.go

@@ -105,6 +105,10 @@ func IsInInterval(b *big.Int, bound *big.Int) bool {
 	return b != nil && bound != nil && b.Cmp(bound) < 0 && b.Cmp(zero) >= 0
 }
 
+func IsInIntervalPositive(b *big.Int, bound *big.Int) bool {
+	return b != nil && bound != nil && b.Cmp(bound) < 0 && b.Sign() > 0
+}
+
 func AppendBigIntToBytesSlice(commonBytes []byte, appended *big.Int) []byte {
 	resultBytes := make([]byte, len(commonBytes), len(commonBytes)+len(appended.Bytes()))
 	copy(resultBytes, commonBytes)

common/random.go

@@ -36,13 +36,27 @@ func MustGetRandomInt(bits int) *big.Int {
 }
 
 func GetRandomPositiveInt(lessThan *big.Int) *big.Int {
-	if lessThan == nil || zero.Cmp(lessThan) != -1 {
+	if lessThan == nil || lessThan.Cmp(one) <= 0 {
 		return nil
 	}
 	var try *big.Int
 	for {
 		try = MustGetRandomInt(lessThan.BitLen())
-		if try.Cmp(lessThan) < 0 && try.Cmp(zero) >= 0 {
+		if try.Cmp(lessThan) < 0 && try.Sign() > 0 {
+			break
+		}
+	}
+	return try
+}
+
+func getRandomNonNegativeInt(lessThan *big.Int) *big.Int {
+	if lessThan == nil || lessThan.Sign() <= 0 {
+		return nil
+	}
+	var try *big.Int
+	for {
+		try = MustGetRandomInt(lessThan.BitLen())
+		if try.Cmp(lessThan) < 0 {
 			break
 		}
 	}
@@ -51,11 +65,14 @@ func GetRandomPositiveInt(lessThan *big.Int) *big.Int {
 
 // Sample an integer in range (-limit, limit)
 func GetRandomInt(limit *big.Int) *big.Int {
+	if limit == nil || limit.Sign() <= 0 {
+		return nil
+	}
 	limitMinus1 := new(big.Int).Sub(limit, big.NewInt(1))
 	limitDoubleMinus1 := new(big.Int).Add(limit, limitMinus1)
 	// get an integer in [0, 2*limit-1) and subtract limit-1
 	// to get an integer in [-limit+1, limit-1]
-	i := GetRandomPositiveInt(limitDoubleMinus1)
+	i := getRandomNonNegativeInt(limitDoubleMinus1)
 	i = i.Sub(i, limitMinus1)
 	return i
 }

common/random_test.go

@@ -31,6 +31,18 @@ func TestGetRandomPositiveInt(t *testing.T) {
 	assert.True(t, rndPos.Cmp(big.NewInt(0)) == 1, "rand int should be positive")
 }
 
+func TestGetRandomPositiveIntRejectsNoPositiveRange(t *testing.T) {
+	assert.Nil(t, common.GetRandomPositiveInt(nil))
+	assert.Nil(t, common.GetRandomPositiveInt(big.NewInt(0)))
+	assert.Nil(t, common.GetRandomPositiveInt(big.NewInt(1)))
+}
+
+func TestGetRandomIntPreservesZeroInclusiveRange(t *testing.T) {
+	assert.Nil(t, common.GetRandomInt(nil))
+	assert.Nil(t, common.GetRandomInt(big.NewInt(0)))
+	assert.Zero(t, common.GetRandomInt(big.NewInt(1)).Sign())
+}
+
 func TestGetRandomPositiveRelativelyPrimeInt(t *testing.T) {
 	rnd := common.MustGetRandomInt(randomIntBitLen)
 	rndPosRP := common.GetRandomPositiveRelativelyPrimeInt(rnd)

common/validation.go

@@ -0,0 +1,38 @@
+// 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 common
+
+import "math/big"
+
+const primalityRounds = 30
+
+func IsUsableUnknownOrderModulus(N *big.Int, minBitLen int) bool {
+	return N != nil &&
+		N.Sign() == 1 &&
+		N.Bit(0) == 1 &&
+		N.BitLen() >= minBitLen &&
+		!N.ProbablyPrime(primalityRounds)
+}
+
+func IsCanonicalGenerator(N, v *big.Int) bool {
+	return N != nil &&
+		N.Sign() == 1 &&
+		v != nil &&
+		v.Cmp(one) > 0 &&
+		v.Cmp(N) < 0 &&
+		IsNumberInMultiplicativeGroup(N, v)
+}
+
+func IsCanonicalPaillierCiphertext(c, N *big.Int) bool {
+	if c == nil || N == nil || N.Sign() != 1 {
+		return false
+	}
+	NSquared := new(big.Int).Mul(N, N)
+	return c.Sign() > 0 &&
+		c.Cmp(NSquared) < 0 &&
+		new(big.Int).GCD(nil, nil, c, N).Cmp(one) == 0
+}

crypto/commitments/commitment.go

@@ -53,10 +53,18 @@ func NewHashDeCommitmentFromBytes(marshalled [][]byte) HashDeCommitment {
 }
 
 func (cmt *HashCommitDecommit) Verify() bool {
+	if cmt == nil {
+		return false
+	}
 	C, D := cmt.C, cmt.D
 	if C == nil || D == nil {
 		return false
 	}
+	for _, part := range D {
+		if part == nil {
+			return false
+		}
+	}
 	hash := common.SHA512_256i(D...)
 	return hash.Cmp(C) == 0
 }

crypto/dlnproof/proof.go

@@ -18,7 +18,15 @@ import (
 	"github.com/bnb-chain/tss-lib/common"
 )
 
-const Iterations = 128
+const (
+	Iterations             = 128
+	verifyMinModulusBitLen = 2048
+	fsDomainTagDLNProof    = "tss-lib.threshold.dlnproof"
+)
+
+func fsSessionDLNProof(session []byte) []byte {
+	return append([]byte(fsDomainTagDLNProof+"|"), session...)
+}
 
 type (
 	Proof struct {
@@ -42,7 +50,7 @@ func NewDLNProof(h1, h2, x, p, q, N *big.Int, session ...[]byte) *Proof {
 		alpha[i] = modN.Exp(h1, a[i])
 	}
 	msg := append([]*big.Int{h1, h2, N}, alpha[:]...)
-	c := common.SHA512_256i_TAGGED(Session, msg...)
+	c := common.SHA512_256i_TAGGED(fsSessionDLNProof(Session), msg...)
 	t := [Iterations]*big.Int{}
 	cIBI := new(big.Int)
 	for i := range t {
@@ -58,22 +66,14 @@ func (p *Proof) Verify(h1, h2, N *big.Int, session ...[]byte) bool {
 	if p == nil {
 		return false
 	}
-	if h1 == nil || h2 == nil || N == nil || N.Sign() != 1 {
+	if !common.IsUsableUnknownOrderModulus(N, verifyMinModulusBitLen) {
 		return false
 	}
 	modN := common.ModInt(N)
-	h1_ := new(big.Int).Mod(h1, N)
-	if h1_.Cmp(one) != 1 || h1_.Cmp(N) != -1 {
-		return false
-	}
-	h2_ := new(big.Int).Mod(h2, N)
-	if h2_.Cmp(one) != 1 || h2_.Cmp(N) != -1 {
-		return false
-	}
-	if h1_.Cmp(h2_) == 0 {
+	if !common.IsCanonicalGenerator(N, h1) || !common.IsCanonicalGenerator(N, h2) {
 		return false
 	}
-	if !common.Coprime(h1_, N) || !common.Coprime(h2_, N) {
+	if h1.Cmp(h2) == 0 {
 		return false
 	}
 	for i := range p.T {
@@ -82,12 +82,12 @@ func (p *Proof) Verify(h1, h2, N *big.Int, session ...[]byte) bool {
 		}
 	}
 	for i := range p.Alpha {
-		if p.Alpha[i] == nil || p.Alpha[i].Cmp(one) <= 0 || p.Alpha[i].Cmp(N) >= 0 {
+		if !common.IsCanonicalGenerator(N, p.Alpha[i]) {
 			return false
 		}
 	}
 	msg := append([]*big.Int{h1, h2, N}, p.Alpha[:]...)
-	c := common.SHA512_256i_TAGGED(Session, msg...)
+	c := common.SHA512_256i_TAGGED(fsSessionDLNProof(Session), msg...)
 	cIBI := new(big.Int)
 	for i := 0; i < Iterations; i++ {
 		cI := c.Bit(i)

crypto/ecpoint.go

@@ -83,13 +83,44 @@ func (p *ECPoint) Curve() elliptic.Curve {
 	return p.curve
 }
 
+// SameCurve compares curve domain parameters, not implementation identity. It
+// intentionally accepts distinct elliptic.Curve implementations with identical
+// parameters; callers must still trust the curve implementation they pass in.
+func SameCurve(lhs, rhs elliptic.Curve) bool {
+	if lhs == nil || rhs == nil {
+		return false
+	}
+	lParams, rParams := lhs.Params(), rhs.Params()
+	if lParams == nil || rParams == nil {
+		return false
+	}
+	return sameBigInt(lParams.P, rParams.P) &&
+		sameBigInt(lParams.N, rParams.N) &&
+		sameBigInt(lParams.B, rParams.B) &&
+		sameBigInt(lParams.Gx, rParams.Gx) &&
+		sameBigInt(lParams.Gy, rParams.Gy) &&
+		lParams.BitSize == rParams.BitSize
+}
+
+func sameBigInt(lhs, rhs *big.Int) bool {
+	if lhs == nil || rhs == nil {
+		return lhs == rhs
+	}
+	return lhs.Cmp(rhs) == 0
+}
+
 func (p *ECPoint) Equals(p2 *ECPoint) bool {
 	if p == nil || p2 == nil {
 		return false
 	}
 	return p.X().Cmp(p2.X()) == 0 && p.Y().Cmp(p2.Y()) == 0
 }
 
+// SetCurve mutates the receiver's curve field in place and returns the same
+// pointer. The chained-call style (`p.SetCurve(ec).ScalarMult(k)`) reads as
+// fluent but is a footgun when p is shared — every alias observes the new
+// curve. Callers that need to ensure the curve without mutating a shared point
+// should construct a fresh ECPoint via NewECPoint instead.
 func (p *ECPoint) SetCurve(curve elliptic.Curve) *ECPoint {
 	if p == nil {
 		return nil
@@ -99,7 +130,19 @@ func (p *ECPoint) SetCurve(curve elliptic.Curve) *ECPoint {
 }
 
 func (p *ECPoint) ValidateBasic() bool {
-	return p != nil && p.coords[0] != nil && p.coords[1] != nil && p.IsOnCurve()
+	return p != nil && p.coords[0] != nil && p.coords[1] != nil && p.IsOnCurve() && !p.IsIdentity()
+}
+
+func (p *ECPoint) IsIdentity() bool {
+	if p == nil || p.coords[0] == nil || p.coords[1] == nil {
+		return false
+	}
+	// The supported curves encode usable affine points away from x=0; reject
+	// common identity-like encodings before arithmetic reaches curve methods.
+	if p.coords[0].Sign() != 0 {
+		return false
+	}
+	return p.coords[1].Sign() == 0 || p.coords[1].Cmp(big.NewInt(1)) == 0
 }
 
 func ScalarBaseMult(curve elliptic.Curve, k *big.Int) *ECPoint {