Skip to content

Commit 68b27e8

Browse files
committed
fix: validate 32-byte message requirement in ecdsa.sign and verify when shouldHash=false
When shouldHash=false, the message must be exactly 32 bytes (a hash). Messages longer than 32 bytes caused calculation errors (messages larger than the curve order when converted to bigint) or verification failures because secp256k1.recoverPublicKey() expects exactly 32 bytes. Adds validation in sign() and verify() methods to enforce this requirement and adds tests to verify the fix. Refs: HSM-129 TICKET: HSM-129
1 parent 89ed696 commit 68b27e8

File tree

2 files changed

+73
-4
lines changed
  • modules/sdk-core
    • src/account-lib/mpc/tss/ecdsa
    • test/unit/account-lib/mpc/tss/ecdsa

2 files changed

+73
-4
lines changed

modules/sdk-core/src/account-lib/mpc/tss/ecdsa/ecdsa.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1357,10 +1357,13 @@ export default class Ecdsa {
13571357
* @param {OShare} oShare private omicron share of current participant
13581358
* @param {DShare} dShare delta share received from the other participant
13591359
* @param {Hash} hash hashing algorithm implementing Node`s standard crypto hash interface
1360-
* @param shouldHash if true, we hash the provided buffer before signing
1360+
* @param shouldHash if true, we hash the provided buffer before signing. If false, M must be exactly 32 bytes (a hash).
13611361
* @returns {VAShare}
13621362
*/
13631363
sign(M: Buffer, oShare: OShare, dShare: DShare, hash?: Hash, shouldHash = true): VAShare {
1364+
if (!shouldHash && M.length !== 32) {
1365+
throw new Error(`When shouldHash=false, message must be exactly 32 bytes (a hash), but got ${M.length} bytes`);
1366+
}
13641367
const m = shouldHash ? (hash || createHash('sha256')).update(M).digest() : M;
13651368

13661369
const delta = Ecdsa.curve.scalarAdd(hexToBigInt(oShare.delta), hexToBigInt(dShare.delta));
@@ -1560,10 +1563,15 @@ export default class Ecdsa {
15601563
* @param {Buffer} message
15611564
* @param {Signature } signature
15621565
* @param {Hash} hash hashing algorithm implementing Node`s standard crypto hash interface
1563-
* @param {boolean} shouldHash if true, we hash the provided buffer before verifying
1566+
* @param {boolean} shouldHash if true, we hash the provided buffer before verifying. If false, message must be exactly 32 bytes (a hash).
15641567
* @returns {boolean} True if signature is valid; False otherwise
15651568
*/
15661569
verify(message: Buffer, signature: Signature, hash?: Hash, shouldHash = true): boolean {
1570+
if (!shouldHash && message.length !== 32) {
1571+
throw new Error(
1572+
`When shouldHash=false, message must be exactly 32 bytes (a hash), but got ${message.length} bytes`
1573+
);
1574+
}
15671575
const messageToVerify = shouldHash ? (hash || createHash('sha256')).update(message).digest() : message;
15681576
return Ecdsa.curve.verify(
15691577
messageToVerify,

modules/sdk-core/test/unit/account-lib/mpc/tss/ecdsa/ecdsa.ts

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,32 @@ describe('ecdsa tss', function () {
2626

2727
let signCombine1: SignCombineRT, signCombine2: SignCombineRT;
2828

29+
function signAndVerify(message: Buffer, shouldHash = true): boolean {
30+
const [sign1, sign2] = [
31+
ecdsa.sign(message, signCombine1.oShare, signCombine2.dShare, undefined, shouldHash),
32+
ecdsa.sign(message, signCombine2.oShare, signCombine1.dShare, undefined, shouldHash),
33+
];
34+
35+
const [signWithProofs1, signWithProofs2] = [
36+
ecdsa.generateVAProofs(message, sign1),
37+
ecdsa.generateVAProofs(message, sign2),
38+
];
39+
40+
const [UT1, UT2] = [
41+
ecdsa.verifyVAShares(signWithProofs1, [signWithProofs2]),
42+
ecdsa.verifyVAShares(signWithProofs2, [signWithProofs1]),
43+
];
44+
45+
const [publicUTShares_1, publicUTShares_2] = [UT1, UT2];
46+
const [signature1, signature2] = [
47+
ecdsa.verifyUTShares(UT1, [publicUTShares_2]),
48+
ecdsa.verifyUTShares(UT2, [publicUTShares_1]),
49+
];
50+
51+
const signature = ecdsa.constructSignature([signature1, signature2]);
52+
return ecdsa.verify(message, signature, undefined, shouldHash);
53+
}
54+
2955
before('generate key and sign phase 1 to 4', async function () {
3056
// In some execution environments (e.g. CI), generateNtilde below may take longer at times.
3157
this.timeout('40s');
@@ -158,8 +184,6 @@ describe('ecdsa tss', function () {
158184
});
159185

160186
it('sign phase 5 should succeed', async function () {
161-
// TODO(HSM-129): There is a bug signing unhashed message (although this deviates from DSA spec) if the message is a little long.
162-
// Some discrepancy between Ecdsa.sign and secp256k1.recoverPublicKey on handling the message input.
163187
const message = Buffer.from('GG18 PHASE 5');
164188

165189
// Starting phase 5
@@ -271,4 +295,41 @@ describe('ecdsa tss', function () {
271295
(() => ecdsa.verifyUTShares(UT1, [publicUTShares_2])).should.throw('Sum of all U_i does not match sum of all T_i');
272296
(() => ecdsa.verifyUTShares(UT2, [publicUTShares_1])).should.throw('Sum of all U_i does not match sum of all T_i');
273297
});
298+
299+
describe('shouldHash parameter', function () {
300+
it('should validate message length when shouldHash=false', function () {
301+
const hash32 = Buffer.alloc(32, 0x01);
302+
const shortMessage = Buffer.from('Short');
303+
const longMessage = Buffer.from('This is a longer message that exceeds 32 bytes in length');
304+
305+
(() => ecdsa.sign(hash32, signCombine1.oShare, signCombine2.dShare, undefined, false)).should.not.throw();
306+
307+
(() => ecdsa.sign(shortMessage, signCombine1.oShare, signCombine2.dShare, undefined, false)).should.throw(
308+
/must be exactly 32 bytes/
309+
);
310+
(() => ecdsa.sign(longMessage, signCombine1.oShare, signCombine2.dShare, undefined, false)).should.throw(
311+
/must be exactly 32 bytes/
312+
);
313+
314+
const dummySig = { r: '00', s: '00', recid: 0, y: signCombine1.oShare.y };
315+
(() => ecdsa.verify(shortMessage, dummySig, undefined, false)).should.throw(/must be exactly 32 bytes/);
316+
(() => ecdsa.verify(longMessage, dummySig, undefined, false)).should.throw(/must be exactly 32 bytes/);
317+
});
318+
319+
it('should work correctly with shouldHash=false when message is 32 bytes', async function () {
320+
const { createHash } = require('crypto');
321+
const message = Buffer.from('Any message');
322+
const messageHash = createHash('sha256').update(message).digest();
323+
324+
signAndVerify(messageHash, false).should.equal(true);
325+
});
326+
327+
it('should work correctly with shouldHash=true for any message length', async function () {
328+
const shortMessage = Buffer.from('Short message');
329+
const longMessage = Buffer.from('This is a longer message that exceeds 32 bytes in length');
330+
331+
signAndVerify(shortMessage, true).should.equal(true);
332+
signAndVerify(longMessage, true).should.equal(true);
333+
});
334+
});
274335
});

0 commit comments

Comments
 (0)