Improve security of signature generation

paulmillr commented 6 days ago

Problem

Transaction signatures use "nonce" / "k" during their construction. The nonce should never be equal between two different messages. Reusing them would allow attacker to recover private key.

Many years ago, nonce was generated using system randomness. On some systems with bad quality of randomness, that lead to breakages.

Today, the nonce is generated from private key and message hash using RFC 6979. Basically hash(private_key, message). However, if some issue would be found in serialization / parsing of those, and during generation of nonce, it would still be possible to recover private keys. The technique is described here: https://github.com/pcaversaccio/ecdsa-nonce-reuse-attack.

Impact

Private key leakage, hackers stealing money from users.

This is not some theoretical issue. This happened in the past. Soon there would be announcement of a new hack related to this.

Solution

Use RFC6979 3.6: additional k' extraEntropy to mix-in 32 byte of random data on every signature. It is standard way of doing this. It has been extensively used by Bitcoin for non-taproot transactions, to decrease signature size.

https://github.com/ethers-io/ethers.js/blob/9e7e7f3e2f2d51019aaa782e6290e079c38332fb/src.ts/crypto/signing-key.ts#L64-L66

secp256k1.sign(msgHash, privateKey) becomes secp256k1.sign(msgHash, privateKey, { extraEntropy: true })

Disadvantages

Signatures (r, s) would become non-deterministic and "new" after every signature.
- They would still be verifiable. This is not a problem for tests because we can specify our own random extraData in tests.

There is no risk for security. If passed-through random is bad, the signature security would be just like today, not worse

paulmillr commented 6 days ago

Viem is on board and implemented the changes today.

ricmoo commented 6 days ago

This is something I would prefer being opt-in, as many applications already in use depend on deterministic signatures.

Is there a way to specify the random source? I would prefer to supply the random source, as some platforms do not have a global source (or the existing source) is not cryptographically secure.

ricmoo commented 6 days ago

(I otherwise fully endorse adding additional entropy to the signing k)

paulmillr commented 6 days ago

secp256k1.sign(msgHash, privateKey, { extraEntropy: extraEntropy }) where extraEntropy could be false, true, or Uint8Array that you pass.

paulmillr commented 6 days ago

This is something I would prefer being opt-in, as many applications already in use depend on deterministic signatures.

Which applications depend on deterministic signatures besides tests?
It makes sense to have it opt-in because it's breaking, however, we should strongly consider making it default in ethers v7. Opt-ins will not really increase security of the whole ecosystem (users don't care and don't know), while opt-out will.

ricmoo commented 5 days ago

I know CLR uses it when computing their semi-ephemeral voting keys.

I also use it for many of my CLI scripts and simple web interfaces. For example, when computing a salt for registering ENS names. The salt only needs to remain “secret” for about 4 blocks, at which time it is revealed. In general, for blinding operations it is a useful technique since the salt can be recovered (by signing a specific message), without the need for a database or any other complex system on the event of a website refresh or a CLI which has no storage in the first place.

I have also seen it used before with generating short-lived shared secrets.

I am absolutely game for making it opt-out after a major version change, but would also prefer to follow suit with whatever Geth/Clef is doing. Have you tried convincing them yet?

paulmillr commented 5 days ago

The cases seem useful! However, you’re advanced user. Most users aren’t. Advanced users can disable it. If we’ll manage to improve security for all non advanced users that would be a huge jump.

I will ask their thoughts on it soon.

ricmoo commented 5 days ago

Oh, I am in no way disagreeing that it is a good idea to add additional entropy.

I just want to make sure we behave like the existing ecosystem (as best as possible) and don’t want to introduce breaking changes in a non-major version.

paulmillr commented 5 days ago

don’t want to introduce breaking changes in a non-major version

agree 100%

paulmillr commented 5 days ago

https://github.com/ethereum/go-ethereum/issues/30773

ethers-io / ethers.js