fix issue 854 partially#938
Conversation
|
brainpool bug is inside undutny/elliptic. Library makes incorrect verify calculations for some subset of signatures. |
|
@twiss I think we could merge as it is. At least it fixes dsa test failure. |
| r: r.toArrayLike(Uint8Array), | ||
| s: s.toArrayLike(Uint8Array) | ||
| r: r.toArrayLike(Uint8Array, 'be', 32), | ||
| s: s.toArrayLike(Uint8Array, 'be', 32) |
There was a problem hiding this comment.
Thanks for the PR! 👍 I think these should be q.bitLength() / 32 instead of 32 (the signature size varies by key size).
There was a problem hiding this comment.
P.S. A bit unrelated but the two calls to util.getLeftNBits(hashed, q.bitLength())) could be replaced by hashed.subarray(0, q.bitLength() / 8); q's bit length must be a multiple of 8. Then getLeftNBits can be removed. You don't have to do that in this PR of course, only if you feel like it.
There was a problem hiding this comment.
Thanks for the PR! 👍 I think these should be
q.bitLength() / 32instead of 32 (the signature size varies by key size).
Is there a way to produce failure with the currently implemented code r.toArrayLike(Uint8Array, 'be', 32) ? If yes, it would be wise to add a test specifically for that (with appropriate key size), as this kind of thing would be easy to miss in the future.
There was a problem hiding this comment.
Hmm. I think it would actually still work with this code, as 32 bytes is enough to fit every signature size. Nevertheless, I think we don't have any tests for that. I agree it would be nice to have one.
There was a problem hiding this comment.
Maybe not as crucial to have a test for this if it would not cause a hard failure (and is not a security concern). Thanks for the explanation 👍
chesnokovilya
force-pushed
the
dsa-failure-fix
branch
from
fb0957e to
a3ff853
Compare
|
I will change to |
chesnokovilya
force-pushed
the
dsa-failure-fix
branch
from
a3ff853 to
92f6a91
Compare
3 participants
Solve #854
DSA signature elements are made constant length.