Fix #221: ecdsa-modified: fix bias and omission of zero in getBigRandom() #631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this replaces the previously remainder-based limiting of the random number which caused bias toward small numbers and excluded zero altogether by simple filtering as proposed frequently in kjur#221 and because the performance in most cases is actually faster than in the present implementation; also, an adaptation of swiftlang/swift#39143 has been considered but it performed significantly slower for large integers;
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have evaluated the simple filtering approach and the approach from swiftlang/swift#39143 and found out that the former is a lot more performant than (my adaption of) the latter and still effective for fixing issue #221 .
See https://htmlpreview.github.io/?https://github.com/tvogel/jsrsasign/blob/tv/getBigRandom-comparison/src/ecdsamod-new-random.output.html for a comparison of the functions. The histograms also make the problem from #221 evident.
In most cases, the new function is faster than the previous implementation and only rarely slightly slower.
The main advantage for the dismissed alternative
new2would be that it is constant in time (depending on the limit only) but it involves consuming always exactly two big random numbers and two big multiplications.See https://github.com/tvogel/jsrsasign/blob/tv/getBigRandom-comparison/src/ecdsamod-new-random.html for the benchmarking code.