Skip to content

Conversation

@dcousens
Copy link
Contributor

This pull request changes deterministicGenerateK to strictly adhere to the RFC rather than just step around the edge cases.

Namely it avoids the following flaw in the current implementation (we were not looping):

Please note that when k is generated from T, the result of bits2int is compared to q, not reduced modulo q. If the value is not between 1 and q-1, the process loops.
Performing a simple modular reduction would induce biases that would be detrimental to signature security.

This is not a critical security update, no security advisory is necessary.
Probability of bias is close to 1 in 2^128 and was known previously.

@dcousens
Copy link
Contributor Author

Waiting on @jprichardson to release ecurve 0.10.0.

@dcousens dcousens changed the title ecdsa RFC 6979 amendments Stricter ecdsa RFC 6979 adherence Jun 25, 2014
The previous impl. was in breach of the following section:

> Please note that when k is generated from T, the result of bits2int is
> compared to q, not reduced modulo q. If the value is not between 1 and
> q-1, the process loops.
> Performing a simple modular reduction would induce biases that would be
> detrimental to signature security.
@jprichardson
Copy link
Member

Should be good now :p

@dcousens
Copy link
Contributor Author

Cheers

@jprichardson
Copy link
Member

I reviewed the RFC 6979 changes. Looks good to me.

@dcousens
Copy link
Contributor Author

Thanks @jprichardson for the comments in IRC, included in above commits.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling bdb0fe8 on dcousens:rfc6979fix into d93623e on bitcoinjs:master.

@kyledrake
Copy link
Contributor

+1

kyledrake added a commit that referenced this pull request Jun 25, 2014
Stricter ecdsa RFC 6979 adherence
@kyledrake kyledrake merged commit 0198477 into bitcoinjs:master Jun 25, 2014
@dcousens dcousens deleted the rfc6979fix branch June 27, 2014 03:10
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