Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve security of signature generation #4885

Open
paulmillr opened this issue Nov 19, 2024 · 10 comments
Open

Improve security of signature generation #4885

paulmillr opened this issue Nov 19, 2024 · 10 comments
Assignees
Labels
on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6

Comments

@paulmillr
Copy link

paulmillr commented Nov 19, 2024

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.

const sig = secp256k1.sign(getBytesCopy(digest), getBytesCopy(this.#privateKey), {
lowS: true
});

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 paulmillr added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Nov 19, 2024
@paulmillr
Copy link
Author

Viem is on board and implemented the changes today.

@ricmoo
Copy link
Member

ricmoo commented Nov 20, 2024

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
Copy link
Member

ricmoo commented Nov 20, 2024

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

@paulmillr
Copy link
Author

paulmillr commented Nov 20, 2024

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

@paulmillr
Copy link
Author

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
Copy link
Member

ricmoo commented Nov 20, 2024

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?

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. and removed investigate Under investigation and may be a bug. labels Nov 20, 2024
@paulmillr
Copy link
Author

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
Copy link
Member

ricmoo commented Nov 20, 2024

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
Copy link
Author

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

agree 100%

@paulmillr
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

2 participants