Fix timing leak in BN_from_montgomery_word. #5228
Conversation
BN_from_montgomery_word doesn't have a constant memory access pattern. Replace the pointer trick with a constant-time select. There is, of course, still the bn_correct_top leak pervasive in BIGNUM itself. See also https://boringssl-review.googlesource.com/22904 from BoringSSL.
|
Incidentally I have virtually identical modification in pipeline. As for virtual see inline comment is a moment... |
| * and we know at most one subtraction is needed. | ||
| */ | ||
| v = bn_sub_words(rp, ap, np, nl) - carry; | ||
| v = 0 - v; |
There was a problem hiding this comment.
No, promise of comment turned to be false alarm. Sorry about confusion :-) I thought that v and carry were declared int, but they are declared BN_ULONG, and that works.
|
BTW, there will be patches for assembly modules addressing similar problem. Just in case for reference. Original code did provide adequate protection for flush-n-reload and alike, but arguably it could expose minuscule variations when address disambiguation logic interacts with speculative loads. I'm adding labels down to 1.0.2 and if it doesn't cherry-pick I'll file follow-up request. |
dot-asm
added
branch: master
dot-asm
added
the
approval: review pending
|
So if I understand correctly, we don't care about the time leak caused by bn_sub_words() because what happens before isn't constant time either? |
|
No, Montgomery reduction should be constant-time. (That is, in part, the whole point of Montgomery reduction.) That includes This all is here because Montgomery reduction produces an answer in [0, 2M), not [0, M), so there is a conditional subtraction needed. That subtraction needs to be performed without leaking whether it was used or not. (This function still isn't constant-time after this PR because of |
|
I was just a little confused about some things. |
kroeckx
added
approval: done
BN_from_montgomery_word doesn't have a constant memory access pattern. Replace the pointer trick with a constant-time select. There is, of course, still the bn_correct_top leak pervasive in BIGNUM itself. See also https://boringssl-review.googlesource.com/22904 from BoringSSL. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #5228)
BN_from_montgomery_word doesn't have a constant memory access pattern. Replace the pointer trick with a constant-time select. There is, of course, still the bn_correct_top leak pervasive in BIGNUM itself. See also https://boringssl-review.googlesource.com/22904 from BoringSSL. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #5228) (cherry picked from commit f345b1f)
|
Didn't cherry-pick to 1.0.2, so #5240 is submitted. Merged to master and 1.1.0. Thanks! |
Of course, now @davidben is a committer this was perhaps overly enthusiastic! |
|
Haha, no worries. I still need to go figure out how that's all set up anyway. |
Just to double-check, are those still in your queue? |
BN_from_montgomery_worddoesn't have a constant memory access pattern. Replace the pointer trick with a constant-time select. There is, of course, still thebn_correct_topleak pervasive in BIGNUM itself.See also https://boringssl-review.googlesource.com/22904 from BoringSSL.
I removed the manual loop unrolling since it didn't seem to particular matter, and this was much easier to read. One would assume the rest of that function dominates the subtraction here. @dot-asm, thoughts?