Regression in 1.5.0

[achingbrain]

If you sign a piece of data with @noble/ed25519 and make a copy of the signature using TypedArray.set, verification sometimes fails.

Repro:

const ed = require('@noble/ed25519')
const crypto = require('crypto')

async function main () {
  const privateKey = ed.utils.randomPrivateKey()
  const publicKey = await ed.getPublicKey(privateKey)

  while (true) {
    const payload = crypto.randomBytes(100)
    const signature = await ed.sign(payload, privateKey)

    if (!(await ed.verify(signature, payload, publicKey))) {
      throw new Error('Signature verification failed')
    }

    const signatureCopy = Buffer.alloc(signature.byteLength)
    signatureCopy.set(signature, 0) // <-- breaks

    if (!(await ed.verify(signatureCopy, payload, publicKey))) {
      throw new Error('Copied signature verification failed')
    }

    console.info('all ok')
  }
}

main().catch(err => {
  console.error(err)
  process.exit(1)
})

The above code with 1.4.0:

$ node index.js
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
all ok
...forever (probably)

With 1.5.0:

$ node index.js
all ok
all ok  <-- sometimes once or twice, sometimes never
Error: Copied signature verification failed
    at main (/Users/alex/test/ed/index.js:26:13)

Env:

$ node --version
v16.13.0

[achingbrain]

These both also occasionally break in the same way:

const signatureCopy = Buffer.from(signature.buffer, signature.byteOffset, signature.byteLength)

const signatureCopy = Buffer.from(signature)

[achingbrain]

This is a problem because if you're doing things like writing sigs into protobufs as libp2p does, sometimes verification will then fail after you read the sig back out of the protobuf.

[paulmillr]

Strange. Not sure how could this happen.

[paulmillr]

Uint8Array.set() seems to work correctly — only Buffer.alloc changes 31st byte.

[paulmillr]

Buffers are piece of shit. Besides them leaking private info [do a = Buffer.from(privateKey, 'hex') and then allocate a new buffer, and access its property — you'll see privateKey inside: b = Buffer.from('something'); console.log(hex(buffer.buffer).contains(hex(a))], buffers seem to not care about .slice().

Somehow it gets through our checks because buffer instanceof Uint8Array is true.

> a=Buffer.from([1,2,3])
<Buffer 01 02 03>
> b=a.slice()
<Buffer 01 02 03>
> b[1] = 34
34
> b
<Buffer 01 22 03>
> a
<Buffer 01 22 03>

[paulmillr]

For the record: Uint8Array.slice() creates a copy, not a subarray. You cannot reason about anything if it's subarray.

[paulmillr]

@achingbrain 1.5.1 will fix this, I still suggest to drop Buffers, they are dangerous and are not supported in browsers anyway. u8as are supported everywhere.

[achingbrain]

Unfortunately I don't think it's that simple. Buffer.allocUnsafe(n) is faster than new Uint8Array(n) which makes a significant difference in hot code paths in node.js (with the obvious caveat about memory initialisation).

Uint8Arrays also seem to use more memory to represent the same data so they're not perfect, or even a drop-in replacement thanks to the .slice issue you point out.

Anyway thanks for fixing the issue.

[paulmillr]

Buffer.allocUnsafe(n) is faster than new Uint8Array(n)

Do you have any benchmarks? In recent nodes buffers are backed by uint8arrays.

Also if you're cool with using temporary shared buffers (= what Buffer.from / allocUnsafe do), there's always an approach used in noble-hashes:

https://github.com/paulmillr/noble-hashes/blob/17ad99d82bf9f7b7f1397421da119c5ae0c4d73a/src/sha256.ts#L29

You can create shared uint8arrays and be fast.

[achingbrain]

Do you have any benchmarks?

const REPEAT = 100000
const size = 1024

let start = Date.now()

for (let i = 0; i < REPEAT; i++) {
  new Uint8Array(size)
}

console.info(`new Uint8Array(${size})`, Date.now() - start, 'ms')

start = Date.now()

for (let i = 0; i < REPEAT; i++) {
  Buffer.alloc(size)
}

console.info(`Buffer.alloc(${size})`, Date.now() - start, 'ms')

start = Date.now()

for (let i = 0; i < REPEAT; i++) {
  Buffer.allocUnsafe(size)
}

console.info(`Buffer.allocUnsafe(${size})`, Date.now() - start, 'ms')

start = Date.now()

for (let i = 0; i < REPEAT; i++) {
  Buffer.allocUnsafe(size).fill(0)
}

console.info(`Buffer.allocUnsafe(${size}).fill(0)`, Date.now() - start, 'ms')

$ node index.js 
new Uint8Array(1024) 86 ms
Buffer.alloc(1024) 83 ms
Buffer.allocUnsafe(1024) 36 ms
Buffer.allocUnsafe(1024).fill(0) 46 ms