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

BIP 340 improvements #893

Merged
merged 30 commits into from
Apr 30, 2020
Merged

BIP 340 improvements #893

merged 30 commits into from
Apr 30, 2020

Conversation

sipa
Copy link
Member

@sipa sipa commented Feb 24, 2020

This makes a number of changes to BIP 340:

  • The tie-breaker for public keys with implicit Y coordinate is changed from square to even. This improves signing speed, and makes integration with existing key generation easier. This also has implications for BIP 341.
  • The nonce generation function is improved to take certain failure scenarios into account (precomputed public key, fault injection attacks, power analysis).
  • Recommendations around using of signing-time randomness and verification are strengthened as these reduce vulnerabilities against the above attacks significantly.
  • The tags are updated to make sure accidental use of earlier draft code breaks consistently.
  • Various contributed improvements.

This is rebased on top of #892 to avoid conflicts.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK mod nit (EDIT)


When an RNG is available at signing time, up to 32 bytes of its output should be included in ''a''. The result is then called a ''synthetic nonce''. Doing so may improve protection against [https://moderncrypto.org/mail-archive/curves/2017/000925.html fault injection attacks and side-channel attacks]. Therefore, '''synthetic nonces are recommended in settings where these attacks are a concern''' - in particular on offline signing devices. Adding more than 32 bytes serves no security purpose. Note that while this means the resulting nonce is not deterministic, its normal security properties do not depend on the quality of the RNG, and in fact using a completely broken RNG is still secure.
The auxiliary random data should be set to fresh randomness generated at signing time, resulting in what is called a ''synthetic nonce''. If no randomness is available, a simple counter can be used as well, or even nothing at all. Using any non-repeating value increases protection against [https://moderncrypto.org/mail-archive/curves/2017/000925.html fault injection attacks]. Using unpredictable randomness additionally increases protection against other side-channel attacks, and is '''recommended whenever available'''. Note that while this means the resulting nonce is not deterministic, the randomness is only supplemental to security. The normal security properties (excluding side-channel attacks) do not depend on the quality of the signing-time RNG.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps s/should be set to fresh randomness/should be set to 32 bytes of fresh randomness/ to provide a clear recommendation.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is resolved

@jonasnick
Copy link
Contributor

With this PR the test vectors would be out of sync. Would be better to include the updates from sipa#196 (still WIP at the moment).

 * Recommend a byte length for aux random data
 * Clarify that with signature verification by default at the end of the signing algorithm, using public keys from untrusted sources is not an issue.  
 *  A few editorial nits
@sipa sipa changed the title BIP 340 improvements [WIP, dontmerge] BIP 340 improvements Feb 25, 2020
@sipa
Copy link
Member Author

sipa commented Feb 25, 2020

I'm marking this as WIP until those things are resolved, but leaving this open for exposure.

As an alternative to generating keys randomly, it is also possible and safe to repurpose existing key generation algorithms for ECDSA in a compatible way. The secret keys constructed by such an algorithm can be used as ''sk'' directly. The public keys constructed by such an algorithm (assuming they use the 33-byte compressed encoding) need to be converted by dropping the first byte. Specifically, [[bip-0032.mediawiki|BIP32]] and schemes built on top of it remain usable.

==== Default Signing ====

Input:
* The secret key ''sk'': a 32-byte array
* The message ''m'': a 32-byte array
* Auxiliary random data ''a'': a byte array of length 0 to 32 (inclusive)

The algorithm ''Sign(sk, m)'' is defined as:
Copy link

Choose a reason for hiding this comment

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

missing a as an argument.

As an alternative to generating keys randomly, it is also possible and safe to repurpose existing key generation algorithms for ECDSA in a compatible way. The secret keys constructed by such an algorithm can be used as ''sk'' directly. The public keys constructed by such an algorithm (assuming they use the 33-byte compressed encoding) need to be converted by dropping the first byte. Specifically, [[bip-0032.mediawiki|BIP32]] and schemes built on top of it remain usable.

==== Default Signing ====

Input:
* The secret key ''sk'': a 32-byte array
* The message ''m'': a 32-byte array
* Auxiliary random data ''a'': a byte array of length 0 to 32 (inclusive)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we fix a to be 32 bytes (or alternatively 0 or 32 bytes). This would make the test vectors much simpler because otherwise we will want to have a test vector with an a that is in between. Making the libsecp compatible with variable len a is unnecessarily complicated as the caller would need to provide a byte array for a that also encodes the length. So either we make a (0 or 32 bytes) or we need to design the test vectors such that only one of them has an in-between a and skip that one it in libsecp.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is resolved

sipa and others added 4 commits March 10, 2020 06:28
Update reference code and test vectors
BIP-0341: Avoid decompressing the output public key in script spends
and make reference code and pseudocode more consistent with each other
This is better for playing around with the code. Now these
these exceptions can really be raised when the verification
during signing fails.
jonasnick and others added 5 commits March 27, 2020 15:14
@sipa sipa changed the title [WIP, dontmerge] BIP 340 improvements BIP 340 improvements Apr 10, 2020
@sipa
Copy link
Member Author

sipa commented Apr 10, 2020

@luke-jr This is ready for merge.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Didn't we promise on the mailing list to provide better rationale for aux_rand (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-March/017711.html) and expanding on how to make up for losing the ability to spot check (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-March/017667.html)? Should we do this here or in a separate PR @sipa?

@sipa
Copy link
Member Author

sipa commented Apr 12, 2020

@jonasnick Sure, but I think those can be done independently. I mostly want to get the even/odd tiebreaker stuff into the published BIP.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Okay, let's get the BIP up to date first.

ACK cf2937c

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK cf2937c

@ajtowns
Copy link
Contributor

ajtowns commented Apr 18, 2020

This PR has an un-squashed "fixup!" commit, as well as a bunch of merges from PRs against sipa's tree... Seems a bit clunky?

Otherwise, ACK cf2937c

@luke-jr luke-jr merged commit 187fabb into bitcoin:master Apr 30, 2020
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.

8 participants