paulmillr / noble-curves

Audited & minimal JS implementation of elliptic curve cryptography.
https://paulmillr.com/noble
MIT License
664 stars 62 forks source link

Signature::fromHex wrong in 1.3.0 #124

Closed randombit closed 7 months ago

randombit commented 7 months ago

I think https://github.com/paulmillr/noble-curves/commit/2f1460a4d7f31e5a61db9606266f5b9a5c659c9d introduced a bug into Signature::fromHex, in this part of the change:

-       const z1 = bytesToNumberBE(hex.slice(0, half));
-       const z2 = bytesToNumberBE(hex.slice(half));
+       const z1 = bytesToNumberBE(value.slice(0, half));
+       const z2 = bytesToNumberBE(value.slice(half));

Here half is hex.length / 2, aka the number of bytes encoded in hex. This is the same as the full length of value, so the slices are wrong.

I repro'd using this test

    should(`this work`, () => {
      const pk = 'a7623a93cdb56c4d23d99c14216afaab3dfd6d4f9eb3db23d038280b6d5cb2caaee2a19dd92c9df7001dede23bf036bc0f33982dfb41e8fa9b8e96b5dc3e83d55ca4dd146c7eb2e8b6859cb5a5db815db86810b8d12cee1588b5dbf34a4dc9a5';

        const sig = 'b89e13a212c830586eaa9ad53946cd968718ebecc27eda849d9232673dcd4f440e8b5df39bf14a88048c15e16cbcaabe';
        const msg = '68656C6C6F';

        deepStrictEqual(bls.verifyShortSignature(sig, msg, pk), true);
    });

This is a valid signature (at least according to both zkcrypto/bls12_381 and MIRACL) but the public key is rejected as not being in the subgroup.

This seems to fix it

-        const z1 = bytesToNumberBE(value.slice(0, half));
-        const z2 = bytesToNumberBE(value.slice(half));
+        const z1 = bytesToNumberBE(value.slice(0, half/2));
+        const z2 = bytesToNumberBE(value.slice(half/2));
heliuchuan commented 7 months ago

i also encountered this

paulmillr commented 7 months ago

a7623... has length 191 , which seems invalid

randombit commented 7 months ago

Sorry something must have gone wrong when I pasted into the issue, corrected now

paulmillr commented 6 months ago

1.4.0 is out