Implement constant-time scalar multiplication for EC points (TOB-4) #419
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
🏁 Benchmark Comparison (Node 22)Comparing this PR (7d0ae70) against master (d64fe7f). Regressions
|
There was a problem hiding this comment.
Pull request overview
This PR implements constant-time scalar multiplication for elliptic curve points to mitigate timing side-channel attacks (TOB-4). The key improvement is the addition of a mulCT method that uses a Montgomery ladder algorithm with constant-time swaps, eliminating secret-dependent branching and memory access patterns that could leak information through timing analysis.
Key changes:
- Added constant-time
Point.mulCT()method using Montgomery ladder with constant-time swaps - Integrated
mulCTinto ECDSA signing, ECDH key derivation, and public key generation to protect secret scalars - Added comprehensive test coverage for constant-time multiplication including edge cases and correctness validation
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/primitives/Point.ts | Implements ctSwap helper and mulCT method using Montgomery ladder for constant-time scalar multiplication |
| src/primitives/ECDSA.ts | Replaces variable-time wNAF scalar multiplication with mulCT in signing to protect ephemeral key k |
| src/primitives/PrivateKey.ts | Updates toPublicKey() and deriveSharedSecret() to use mulCT for constant-time private key operations |
| src/primitives/PublicKey.ts | Updates deriveSharedSecret() to use mulCT for constant-time shared secret derivation |
| src/primitives/__tests/Point.test.ts | Adds comprehensive tests for mulCT covering zero scalars, large scalars, non-generator points, and alternating bit patterns |
| src/primitives/__tests/ECDSA.test.ts | Adds stress test for ECDSA signing with large private keys to validate mulCT integration |
| CHANGELOG.md | Documents the constant-time scalar multiplication addition and security improvements |
| package.json | Version bump to 1.9.30 |
| package-lock.json | Updates version references to 1.9.30 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (kBig === 0n || this.inf) return new Point(null, null) | ||
|
|
||
| if (kBig < 0n) kBig = -kBig | ||
| kBig = biMod(kBig) |
There was a problem hiding this comment.
The zero check for kBig happens before the modular reduction (biMod) on line 805. This means if the input scalar k equals the curve order N (or a multiple of N), it won't be caught by this early return check, and will only be reduced to 0 after line 805. This is inefficient and inconsistent with the mul method, which checks for zero after reduction. Add another zero check after line 805 to handle this case.
| kBig = biMod(kBig) | |
| kBig = biMod(kBig) | |
| if (kBig === 0n) return new Point(null, null) |
| const p = c.g.mul(this) | ||
| return new PublicKey(p.x, p.y) | ||
| const p = c.g.mulCT(this) | ||
| return new PublicKey(p.getX(), p.getY()) |
There was a problem hiding this comment.
The change from p.x, p.y to p.getX(), p.getY() creates an inconsistency with the existing PublicKey.fromPrivateKey() method (line 36 in PublicKey.ts), which still uses p.x, p.y. Both patterns work correctly, but the inconsistency makes the codebase harder to maintain. Additionally, using getX()/getY() introduces an unnecessary conversion cycle (fromRed in getX/getY, then toRed in the PublicKey constructor), which is less efficient than passing the coordinates directly.
| return new PublicKey(p.getX(), p.getY()) | |
| return new PublicKey(p.x, p.y) |
|
|
||
| --- | ||
|
|
||
| ## [1.9.30] - 2025-12-18 |
There was a problem hiding this comment.
The date in the section header is inconsistent with the Table of Contents. Line 8 references "1.9.30 - 2025-12-17" but this section header shows "2025-12-18". These dates should match.
| ## [1.9.30] - 2025-12-18 | |
| ## [1.9.30] - 2025-12-17 |
| if (kBig < 0n) kBig = -kBig | ||
| kBig = biMod(kBig) | ||
|
|
||
| const Px = | ||
| this === this.curve.g | ||
| ? GX_BIGINT | ||
| : BigInt('0x' + this.getX().toString(16)) | ||
|
|
||
| const Py = | ||
| this === this.curve.g | ||
| ? GY_BIGINT | ||
| : BigInt('0x' + this.getY().toString(16)) | ||
|
|
||
| let R0: JacobianPointBI = { X: 0n, Y: 1n, Z: 0n } // infinity | ||
| let R1: JacobianPointBI = { X: Px, Y: Py, Z: 1n } | ||
|
|
||
| const bits = kBig.toString(2) | ||
|
|
||
| for (let i = 0; i < bits.length; i++) { | ||
| const bit = bits[i] === '1' ? 1n : 0n | ||
|
|
||
| ctSwap(bit, R0, R1) | ||
| R1 = jpAdd(R0, R1) | ||
| R0 = jpDouble(R0) | ||
| ctSwap(bit, R0, R1) | ||
| } | ||
|
|
||
| if (R0.Z === 0n) return new Point(null, null) | ||
|
|
||
| const zInv = biModInv(R0.Z) | ||
| const zInv2 = biModMul(zInv, zInv) | ||
|
|
||
| const x = biModMul(R0.X, zInv2) | ||
| const y = biModMul(R0.Y, biModMul(zInv2, zInv)) | ||
|
|
||
| return new Point(x.toString(16), y.toString(16)) |
There was a problem hiding this comment.
The mulCT method doesn't handle negative scalars correctly. Unlike the mul method (lines 757-791), which negates the result when the input scalar is negative, mulCT only takes the absolute value and returns the result without negation. This means mulCT(-k) will incorrectly return the same result as mulCT(k) instead of the negated point. This breaks mathematical correctness and API consistency with the existing mul method.
| describe('Point.mulCT (constant-time scalar multiplication)', () => { | ||
| const G = Point.fromString( | ||
| '0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798' | ||
| ) | ||
|
|
||
| it('returns point at infinity for scalar = 0', () => { | ||
| const r = G.mulCT(0) | ||
| expect(r.isInfinity()).toBe(true) | ||
| }) | ||
|
|
||
| it('matches regular mul for small scalar', () => { | ||
| const k = 5 | ||
| const r1 = G.mul(k) | ||
| const r2 = G.mulCT(k) | ||
|
|
||
| expect(r2.eq(r1)).toBe(true) | ||
| }) | ||
|
|
||
| it('matches regular mul for large scalar', () => { | ||
| const k = | ||
| 'fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141' | ||
|
|
||
| const r1 = G.mul(k) | ||
| const r2 = G.mulCT(k) | ||
|
|
||
| expect(r2.eq(r1)).toBe(true) | ||
| }) | ||
|
|
||
| it('works with non-generator base point', () => { | ||
| const base = G.mul(3) | ||
| const k = 11 | ||
|
|
||
| const r1 = base.mul(k) | ||
| const r2 = base.mulCT(k) | ||
|
|
||
| expect(r2.eq(r1)).toBe(true) | ||
| }) | ||
|
|
||
| it('handles alternating bit patterns (ctSwap exercised)', () => { | ||
| // 101010... pattern forces both swap paths | ||
| const k = BigInt( | ||
| '0b101010101010101010101010101010101010101010101010101010101010101' | ||
| ) | ||
|
|
||
| const r1 = G.mul(k.toString(10)) | ||
| const r2 = G.mulCT(k.toString(10)) | ||
|
|
||
| expect(r2.eq(r1)).toBe(true) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test suite for mulCT lacks coverage for negative scalars, which is an important edge case. The existing mul method supports negative scalars by returning the negated point, and mulCT should behave consistently. Add a test case that verifies mulCT(-k) equals the negation of mulCT(k), and that it matches mul(-k).
|
🏁 Benchmark Comparison (Node 22)Comparing this PR (b667e19) against master (446573b). Regressions
Speedups
|
Description of Changes
This PR hardens elliptic-curve scalar multiplication against timing side-channel attacks by introducing a constant-time
mulCTimplementation and integrating it into relevant cryptographic flows.Key improvements include:
Point.mulCT) using a Montgomery ladder–style approach with constant-time swaps.These changes eliminate scalar-dependent branching and memory access patterns that could previously leak information through timing analysis.
This change introduces a measurable performance regression in transaction verification benchmarks (≈30–55%), which is expected and unavoidable due to the removal of variable-time elliptic curve scalar multiplication.
The new implementation enforces constant-time behavior across all scalar operations to mitigate timing side-channel attacks (TOB-4). All other benchmark categories remain unaffected, confirming that the performance impact is isolated to ECC-heavy verification paths.
This tradeoff aligns with industry-standard cryptographic hardening practices and prioritizes key-material safety over raw throughput.
Linked Issues / Tickets
Closes TOB-4
Testing Procedure
ctSwappathsChecklist
CHANGELOG.mdwith my changesnpm run docandnpm run lintone final time before requesting a reviewts-standardnpm version patchso that my changes will trigger a new version to be released when they are merged