paulmillr / noble-curves

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

Wrong signature is generated #119

Closed mahdiyari closed 4 months ago

mahdiyari commented 4 months ago

This issue is weird as it happens randomly. The public key retrieved from the signature doesn't match the original public key. And it happens randomly. I did write the following snippet to reproduce the issue.

import { secp256k1 } from '@noble/curves/secp256k1'

const key = '25cc05e7e365b876f9db30d8a02961fef516ded513c6163d5a18fa6fa9e98c41'
const pub = secp256k1.getPublicKey(key, true)
const pubhex = Buffer.from(pub).toString('hex')
console.log('PUB:' , Buffer.from(pub).toString('hex'))
const sign = (message) => {
  let rs
  let rss
  let rv = {}
  let attempts = 0
  do {
    const options = {
      extraEntropy: true,
      lowS: true
    }
    rv = secp256k1.sign(message, key, options)
    rss = rv.r.toString(16) + rv.s.toString(16)
    rs = Buffer.from(rss)
  } while (!isCanonicalSignature(rs))
  return {rss, r: rv.recovery}
}

const isCanonicalSignature = (signature) => {
  return (
    !(signature[0] & 0x80) &&
    !(signature[0] === 0 && !(signature[1] & 0x80)) &&
    !(signature[32] & 0x80) &&
    !(signature[32] === 0 && !(signature[33] & 0x80))
  )
}

const msg = Buffer.from('c8f31ef5055fce1af5e398c35401f282792da00c330b029779d2e62bb02a5085', 'hex')

for (let i = 0; i < 1000; i++) {
  const sig = sign(msg)
  const r = BigInt('0x' + sig.rss.slice(0, 64))
  const s = BigInt('0x' + sig.rss.slice(64, 128))
  const temp = new secp256k1.Signature(r, s, sig.r)
  const pubkey = temp.recoverPublicKey(msg).toHex(1)
  if (pubkey !== pubhex) {
    console.log(sig)
    console.log(temp, r, s)
    console.log(pubkey)
    break
  } 
}

The for at the end breaks whenever the public keys are not matched. You can log i too and it happens very early. Tested on node v20, v18, v16

Sometimes it will throw Error: Cannot find square root

Am I doing something wrong?

mahdiyari commented 4 months ago

This issue originally happened to me using noble-secp256k1 so I used this library instead as part of debugging.

paulmillr commented 4 months ago

Am I doing something wrong?

yes: you are using buffer instead of uint8array

paulmillr commented 4 months ago

why are you using rv.r.toString(16)? we have perfectly fine toCompact and toDER methods

why are you using isCanonicalSignature if we have lowS option in verify

why are you passing R as third recovery argument to Signature constructor?

mahdiyari commented 4 months ago

why are you passing R as third recovery argument to Signature constructor?

that's not R, that's recovery. Bad naming. I did remove Buffer and used hex string and still the same issue.

paulmillr commented 4 months ago

yeah you are still using tostring(16)

mahdiyari commented 4 months ago

I did merge r + s together then I separate them later. The code doesn't seem wrong to me as the values before merge and after separation were identical. The code works but it fails sometimes. That's why it got me.

I can't reproduce the issue without using Buffer and not using tostring(16).

Thanks. Much appreciate it.

paulmillr commented 4 months ago

tostring 16 creates unpadded hex, so 000123 would become 0123, kudos

mahdiyari commented 4 months ago

tostring 16 creates unpadded hex, so 000123 would become 0123, kudos

It all makes sense now. Thanks again. :pray: