Fix bugs in bn.halveM and bn.montpowermod #441
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
`montpowermod` was incorrectly assuming that the base value was already
modulo N. This went undetected for so long because the `montMul`
function is VERY robust against unusually-large bases. As long as the
base was within ~8 bits of the modulus, the algorithm would perform
correctly. It was also partly masked by the second issue in halveM.
The halveM function did not handle halving 0 or 1 correctly. It would
pop the last limb from the array, returning a `bn` in a weird state.
Most other `bn` operations could recover from it, but the check that
`montpowermod` performs on the modulus is particularly sensitive to
the bug. On my machine this issue was related to the montpowermod,
so I have included both bugs together in the same commit. See below.
I suspect that there is another issue in the montpowermod code, it
doesn't call `normalize()` after `RP.add(NN)` and `NP.add(RR)`, while
the `halveM()` call assumes its input is normalized. I haven't actually
encountered erroneous output, though.
------
On the windows build node v20.10.0, the jit version of halveM was
behaving differently from the interpreted version (i.e. while step
debugging, and while the jit was cold), for the specific
input `new bn(2000).powermod(800, 19)`, evaluating halveM on line:
```js
while (RT.greaterEquals(1)) {
RT.halveM();
```
This would cause the extended Euclidean algorithm check to fail
reverting to the slower (correct) powermod code.
Powermod bug was introduced in commit 2f591b4
HalveM bug was introduced in commit c08108f
Closes bitwiseshiftleft#419
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.
montpowermodwas incorrectly assuming that the base value was already modulo N. This went undetected for so long because themontMulfunction is VERY robust against unusually-large bases. As long as the base was within ~8 bits of the modulus, the algorithm would perform correctly. It was also partly masked by the second issue in halveM.The halveM function did not handle halving 0 or 1 correctly. It would pop the last limb from the array, returning a
bnin a weird state. Most otherbnoperations could recover from it, but the check thatmontpowermodperforms on the modulus is particularly sensitive to the bug. On my machine this issue was related to the montpowermod, so I have included both bugs together in the same commit. See below.I suspect that there is another issue in the montpowermod code, it doesn't call
normalize()afterRP.add(NN)andNP.add(RR), while thehalveM()call assumes its input is normalized. I haven't actually encountered erroneous output, though.On the windows build node v20.10.0, the JIT version of
halveMwas behaving differently from the interpreted version (i.e. while step debugging, and when the JIT was cold), for the specific inputnew bn(2000).powermod(800, 19), evaluatinghalveMon line 370:The interpreted version would cause the extended Euclidean algorithm check to fail reverting to the slower (correct) powermod code.
Powermod bug was introduced in commit 2f591b4
HalveM bug was introduced in commit c08108f
Closes #419
Related to #172