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

Update taproot signatures to use a deterministic nonce #41

Closed
wants to merge 5 commits into from

Conversation

hoffmabc
Copy link

The Problem

We were using sign_schnorr_no_aux_rand() for Taproot signatures, which means we weren't providing ANY auxiliary randomness in the nonce generation. This is problematic because it reduces the security of the signature scheme - proper nonce randomization is crucial for Schnorr signature security to prevent potential nonce-reuse attacks.

The Solution

We're modifying the signing process to use proper auxiliary randomness through the standard sign_schnorr() method instead of sign_schnorr_no_aux_rand(). This ensures that:

  1. Each signature gets fresh randomness in its nonce generation
  2. The signatures maintain their full security properties
  3. We follow best practices for Schnorr signature generation

Impact

  • Improves signature security by using proper randomization
  • Prevents potential nonce-reuse vulnerabilities
  • Aligns with recommended practices for Schnorr signatures

This is a security-critical change that ensures our Taproot signatures are generated with appropriate randomness in their nonces.

@hoffmabc
Copy link
Author

cc: @0xfinetuned

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some auxiliary randomness makes sense to me, as long as all attempts to make a signature over the same message with the same private key results in the same signature.

The other functions and changes I don't quite understand.

src/util.rs Outdated Show resolved Hide resolved
src/sign.rs Outdated Show resolved Hide resolved
src/sign.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/sign.rs Outdated Show resolved Hide resolved
src/sign.rs Outdated Show resolved Hide resolved
src/verify.rs Outdated Show resolved Hide resolved
src/verify.rs Outdated Show resolved Hide resolved
src/verify.rs Outdated Show resolved Hide resolved
@apoelstra
Copy link
Member

Adding some auxiliary randomness makes sense to me, as long as all attempts to make a signature over the same message with the same private key results in the same signature.

If this is your goal, then adding auxiliary randomness provides no value. The value of the aux randomness is to undermine potential sidechannel attacks which sign the same message repeatedly to obtain higher-resolution timing data about that signature.

It would be better then to continue to use no_aux_randomness to make it clear to a reader that this is a design goal.

@hoffmabc
Copy link
Author

The main point of this PR is to mitigate the side channel attack @apoelstra is mentioning so it conflicts with the idea of having deterministic signatures creating the same output over and over. We'd need something like RFC 6979 to accomplish that I think.

@0xfinetuned
Copy link

Adding some auxiliary randomness makes sense to me, as long as all attempts to make a signature over the same message with the same private key results in the same signature.

@raphjaph is there a reason for requiring all signatures be the same with same message and same private key? if so we can pass the aux data to the function to make it deterministic.

@apoelstra
Copy link
Member

I am also curious under what conditions the signatures need to be deterministic.

If you always need the same signature given same key/message then don't use any aux randomness (as is currently the case). If this is only true within a particular session for example, then you should pass a session ID as the aux randomness. If it's true only for each particular peer, use a peer ID, etc.

But this is just generic advice. I don't know the actual requirements of this crate.

@raphjaph
Copy link
Collaborator

Just looking at Bitcoin Core and it seems to have the facilities to add auxillary randomness but at the moment it's not used (it passes in an empty vec): https://github.com/bitcoin/bitcoin/blob/da10e0bab4a3e98868dd663af02c43b1dc8b7f4a/src/script/sign.cpp#L88

Another javascript BIP322 library also doesn't seem to use aux_randomness. But if you drill into the dependencies namely ecpair and tiny-secp256k1 they have facilities to add that randomness.

I got some of my test vectors from both of these to make sure I produce the correct signatures and that only works if the signatures are deterministic. However, I would like to follow best practices and add the option to add aux_randomness, so here is what I propose.

We make the private functions create_message_signature_taproot and create_message_signature_p2wpkh public and add an Option<[u8; 32]> to the function signature of the former so that you can add any source of randomness you'd like. I think this would also allow us to eliminate the two extra function you wrote. I can't edit this PR unfortunately so I'll open a new PR with the proposed changes.

@raphjaph
Copy link
Collaborator

Here's the PR: #42

Let me know what you think.

@0xfinetuned
Copy link

just checked the PR. that will work for us 👍

@raphjaph
Copy link
Collaborator

Ok, good to know, I will close this one then.

@raphjaph raphjaph closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants