https://boringssl-review.googlesource.com/10520 and then later
https://boringssl-review.googlesource.com/25285 made BN_MONT_CTX_set
constant-time, which is necessary for RSA's mont_p and mont_q. However,
due to a typo in the benchmark, they did not correctly measure.
Split BN_MONT_CTX creation into a constant-time and variable-time one.
The constant-time one uses our current algorithm and the latter restores
the original BN_mod codepath.
Should we wish to avoid BN_mod, I have an alternate version lying
around:
First, BN_set_bit + bn_mod_lshift1_consttime as now to count up to 2*R.
Next, observe that 2*R = BN_to_montgomery(2) and R*R =
BN_to_montgomery(R) = BN_to_montgomery(2^r_bits) Also observe that
BN_mod_mul_montgomery only needs n0, not RR. Split the core of
BN_mod_exp_mont into its own function so the caller handles conversion.
Raise 2*R to the r_bits power to get 2^r_bits*R = R*R.
The advantage of that algorithm is that it is still constant-time, so we
only need one BN_MONT_CTX_new. Additionally, it avoids BN_mod which is
otherwise (almost, but the remaining links should be easy to cut) out of
the critical path for correctness. One less operation to worry about.
The disadvantage is that it is gives a 25% (RSA-2048) or 32% (RSA-4096)
slower RSA verification speed. I went with the BN_mod one for the time
being.
Before:
Did 9204 RSA 2048 signing operations in 10052053us (915.6 ops/sec)
Did 326000 RSA 2048 verify (same key) operations in 10028823us (32506.3 ops/sec)
Did 50830 RSA 2048 verify (fresh key) operations in 10033794us (5065.9 ops/sec)
Did 1269 RSA 4096 signing operations in 10019204us (126.7 ops/sec)
Did 88435 RSA 4096 verify (same key) operations in 10031129us (8816.1 ops/sec)
Did 14552 RSA 4096 verify (fresh key) operations in 10053411us (1447.5 ops/sec)
After:
Did 9150 RSA 2048 signing operations in 10022831us (912.9 ops/sec)
Did 322000 RSA 2048 verify (same key) operations in 10028604us (32108.2 ops/sec)
Did 289000 RSA 2048 verify (fresh key) operations in 10017205us (28850.4 ops/sec)
Did 1270 RSA 4096 signing operations in 10072950us (126.1 ops/sec)
Did 87480 RSA 4096 verify (same key) operations in 10036328us (8716.3 ops/sec)
Did 80730 RSA 4096 verify (fresh key) operations in 10073614us (8014.0 ops/sec)
Change-Id: Ie8916d1634ccf8513ceda458fa302f09f3e93c07
Reviewed-on: https://boringssl-review.googlesource.com/27287
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This used to work, but I broke it on accident in the recent rewrite.
Change-Id: I06ab5e06eb0c0a6b67ecc97919654e386f3c2198
Reviewed-on: https://boringssl-review.googlesource.com/26984
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This uses the full binary GCD algorithm, where all four of A, B, C, and
D must be retained. (BN_mod_inverse_odd implements the odd number
version which only needs A and C.) It is patterned after the version
in the Handbook of Applied Cryptography, but tweaked so the coefficients
are non-negative and bounded.
Median of 29 RSA keygens: 0m0.225s -> 0m0.220s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I6dc13524ea7c8ac1072592857880ddf141d87526
Reviewed-on: https://boringssl-review.googlesource.com/26370
Reviewed-by: Adam Langley <alangley@gmail.com>
RSA key generation requires computing a GCD (p-1 and q-1 are relatively
prime with e) and an LCM (the Carmichael totient). I haven't made BN_gcd
itself constant-time here to save having to implement
bn_lshift_secret_shift, since the two necessary operations can be served
by bn_rshift_secret_shift, already added for Rabin-Miller. However, the
guts of BN_gcd are replaced. Otherwise, the new functions are only
connected to tests for now, they'll be used in subsequent CLs.
To support LCM, there is also now a constant-time division function.
This does not replace BN_div because bn_div_consttime is some 40x slower
than BN_div. That penalty is fine for RSA keygen because that operation
is not bottlenecked on division, so we prefer simplicity over
performance.
Median of 29 RSA keygens: 0m0.212s -> 0m0.225s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: Idbfbfa6e7f5a3b8782ce227fa130417b3702cf97
Reviewed-on: https://boringssl-review.googlesource.com/26369
Reviewed-by: Adam Langley <alangley@gmail.com>
Expose the constant-time abs_sub functions from the fixed Karatsuba code
in BIGNUM form for RSA to call into. RSA key generation involves
checking if |p - q| is above some lower bound.
BN_sub internally branches on which of p or q is bigger. For any given
iteration, this is not secret---one of p or q is necessarily the larger,
and whether we happened to pick the larger or smaller first is
irrelevant. Accordingly, there is no need to perform the p/q swap at the
end in constant-time.
However, this stage of the algorithm picks p first, sticks with it, and
then computes |p - q| for various q candidates. The distribution of
comparisons leaks information about p. The leak is unlikely to be
problematic, but plug it anyway.
Median of 29 RSA keygens: 0m0.210s -> 0m0.212s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I024b4e51b364f5ca2bcb419a0393e7be13249aec
Reviewed-on: https://boringssl-review.googlesource.com/26368
Reviewed-by: Adam Langley <alangley@gmail.com>
(This is actually slightly silly as |a|'s probability distribution falls
off exponentially, but it's easy enough to do right.)
Instead, we run the loop to the end. This is still performant because we
can, as before, return early on composite numbers. Only two calls
actually run to the end. Moreover, running to the end has comparable
cost to BN_mod_exp_mont_consttime.
Median time goes from 0.140s to 0.231s. That cost some, but we're still
faster than the original implementation.
We're down to one more leak, which is that the BN_rand_range_ex call
does not hide |w1|. That one may only be solved probabilistically...
Median of 29 RSA keygens: 0m0.123s -> 0m0.145s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I4847cb0053118c572d2dd5f855388b5199fa6ce2
Reviewed-on: https://boringssl-review.googlesource.com/25888
Reviewed-by: Adam Langley <agl@google.com>
Compilers use a variant of Barrett reduction to divide by constants,
which conveniently also avoids problematic operations on the secret
numerator. Implement the variant as described here:
http://ridiculousfish.com/blog/posts/labor-of-division-episode-i.html
Repurpose this to implement a constant-time BN_mod_word replacement.
It's even much faster! I've gone ahead and replaced the other
BN_mod_word calls on the primes table.
That should give plenty of budget for the other changes. (I am assuming
that a regression is okay, as RSA keygen is not performance-sensitive,
but that I should avoid anything too dramatic.)
Proof of correctness: https://github.com/davidben/fiat-crypto/blob/barrett/src/Arithmetic/BarrettReduction/RidiculousFish.v
Median of 29 RSA keygens: 0m0.621s -> 0m0.123s
(Accuracy beyond 0.1s is questionable, though this particular
improvement is quite solid.)
Bug: 238
Change-Id: I67fa36ffe522365b13feb503c687b20d91e72932
Reviewed-on: https://boringssl-review.googlesource.com/25887
Reviewed-by: Adam Langley <agl@google.com>
The extra details in Enhanced Rabin-Miller are only used in
RSA_check_key_fips, on the public RSA modulus, which the static linker
will drop in most of our consumers anyway. Implement normal Rabin-Miller
for RSA keygen and use Montgomery reduction so it runs in constant-time.
Note that we only need to avoid leaking information about the input if
it's a large prime. If the number ends up composite, or we find it in
our table of small primes, we can return immediately.
The leaks not addressed by this CL are:
- The difficulty of selecting |b| leaks information about |w|.
- The distribution of whether step 4.4 runs leaks information about w.
- We leak |a| (the largest power of two which divides w) everywhere.
- BN_mod_word in the trial division is not constant-time.
These will be resolved in follow-up changes.
Median of 29 RSA keygens: 0m0.521 -> 0m0.621s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I0cf0ff22079732a0a3ababfe352bb4327e95b879
Reviewed-on: https://boringssl-review.googlesource.com/25886
Reviewed-by: Adam Langley <agl@google.com>
These are composite numbers whose composite witnesses aren't in the
first however many prime numbers, so deterministically checking small
numbers may not work.
We don't check composite witnesses deterministically but these are
probably decent tests. (Not sure how else to find composites with
scarce witnesses, but these seemed decent candidates.)
Change-Id: I23dcb7ba603a64c1f7d1e9a16942e7c29c76da51
Reviewed-on: https://boringssl-review.googlesource.com/26645
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These were randomly generated.
Change-Id: I532afdaf469e6c80e518dae3a75547ff7cb0948f
Reviewed-on: https://boringssl-review.googlesource.com/26065
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This allows a BIGNUM consumer to avoid messing around with bn->d and
bn->top/width.
Bug: 232
Change-Id: I134cf412fef24eb404ff66c84831b4591d921a17
Reviewed-on: https://boringssl-review.googlesource.com/25484
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is a bit easier to read than BN_less_than_consttime when we must do
>= or <=, about as much work to compute, and lots of code calls BN_cmp
on secret data. This also, by extension, makes BN_cmp_word
constant-time.
BN_equal_consttime is probably a little more efficient and is perfectly
readable, so leave that one around.
Change-Id: Id2e07fe312f01cb6fd10a1306dcbf6397990cf13
Reviewed-on: https://boringssl-review.googlesource.com/25444
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This has no behavior change, but it has a semantic one. This CL is an
assertion that all BIGNUM functions tolerate non-minimal BIGNUMs now.
Specifically:
- Functions that do not touch top/width are assumed to not care.
- Functions that do touch top/width will be changed by this CL. These
should be checked in review that they tolerate non-minimal BIGNUMs.
Subsequent CLs will start adjusting the widths that BIGNUM functions
output, to fix timing leaks.
Bug: 232
Change-Id: I3a2b41b071f2174452f8d3801bce5c78947bb8f7
Reviewed-on: https://boringssl-review.googlesource.com/25257
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These actually work as-is, but BN_bn2hex allocates more memory than
necessary, and we may as well skip the unnecessary words where we can.
Also add a test for this.
Bug: 232
Change-Id: Ie271fe9f3901d00dd5c3d7d63c1776de81a10ec7
Reviewed-on: https://boringssl-review.googlesource.com/25304
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Test this by re-running bn_tests.txt tests a lot. For the most part,
this was done by scattering bn_minimal_width or bn_correct_top calls as
needed. We'll incrementally tease apart the functions that need to act
on non-minimal BIGNUMs in constant-time.
BN_sqr was switched to call bn_correct_top at the end, rather than
sample bn_minimal_width, in anticipation of later splitting it into
BN_sqr (for calculators) and BN_sqr_fixed (for BN_mod_mul_montgomery).
BN_div_word also uses bn_correct_top because it calls BN_lshift so
officially shouldn't rely on BN_lshift returning something
minimal-width, though I expect we'd want to split off a BN_lshift_fixed
than change that anyway?
The shifts sample bn_minimal_width rather than bn_correct_top because
they all seem to try to be very clever around the bit width. If we need
constant-time versions of them, we can adjust them later.
Bug: 232
Change-Id: Ie17b39034a713542dbe906cf8954c0c5483c7db7
Reviewed-on: https://boringssl-review.googlesource.com/25255
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These empty states aren't any use to either caller or implementor.
Change-Id: If0b748afeeb79e4a1386182e61c5b5ecf838de62
Reviewed-on: https://boringssl-review.googlesource.com/25254
Reviewed-by: Adam Langley <agl@google.com>
Checking the excess words for zero doesn't need to be in constant time,
but it's free. BN_bn2bin_padded is a little silly as read_word_padded
only exists to work around bn->top being minimal. Once non-minimal
BIGNUMs are turned on and the RSA code works right, we can simplify
BN_bn2bin_padded.
Bug: 232
Change-Id: Ib81e30ca1e5a8ea90ab3278bf4ded219bac481ac
Reviewed-on: https://boringssl-review.googlesource.com/25253
Reviewed-by: Adam Langley <agl@google.com>
Give a non-minimal modulus, there are two possible values of R we might
pick: 2^(BN_BITS2 * width) or 2^(BN_BITS2 * bn_minimal_width).
Potentially secret moduli would make the former attractive and things
might even work, but our only secret moduli (RSA) have public bit
widths. It's more cases to test and the usual BIGNUM invariant is that
widths do not affect numerical output.
Thus, settle on minimizing mont->N for now. With the top explicitly made
minimal, computing |lgBigR| is also a little simpler.
This CL also abstracts out the < R check in the RSA code, and implements
it in a width-agnostic way.
Bug: 232
Change-Id: I354643df30530db7866bb7820e34241d7614f3c2
Reviewed-on: https://boringssl-review.googlesource.com/25250
Reviewed-by: Adam Langley <agl@google.com>
This is actually a bit more complicated (the mismatching widths cases
will never actually happen in RSA), but it's easier to think about and
removes more width-sensitive logic.
Bug: 232
Change-Id: I85fe6e706be1f7d14ffaf587958e930f47f85b3c
Reviewed-on: https://boringssl-review.googlesource.com/25246
Reviewed-by: Adam Langley <agl@google.com>
This makes it easier going to and from non-minimal BIGNUMs and words
without worrying about the widths which are ultimately to become less
friendly.
Bug: 232
Change-Id: Ia57cb29164c560b600573c27b112ad9375a86aad
Reviewed-on: https://boringssl-review.googlesource.com/25245
Reviewed-by: Adam Langley <agl@google.com>
Thanks to Andres Erbsen for extremely helpful suggestions on how finally
plug this long-standing hole!
OpenSSL BIGNUMs are currently minimal-width, which means they cannot be
constant-time. We'll need to either excise BIGNUM from RSA and EC or
somehow fix BIGNUM. EC_SCALAR and later EC_FELEM work will excise it
from EC, but RSA's BIGNUMs are more transparent. Teaching BIGNUM to
handle non-minimal word widths is probably simpler.
The main constraint is BIGNUM's large "calculator" API surface. One
could, in theory, do arbitrary math on RSA components, which means all
public functions must tolerate non-minimal inputs. This is also useful
for EC; https://boringssl-review.googlesource.com/c/boringssl/+/24445 is
silly.
As a first step, fix comparison-type functions that were assuming
minimal BIGNUMs. I've also added bn_resize_words, but it is testing-only
until the rest of the library is fixed.
bn->top is now a loose upper bound we carry around. It does not affect
numerical results, only performance and secrecy. This is a departure
from the original meaning, and compiler help in auditing everything is
nice, so the final change in this series will rename bn->top to
bn->width. Thus these new functions are named per "width", not "top".
Looking further ahead, how are output BIGNUM widths determined? There's
three notions of correctness here:
1. Do I compute the right answer for all widths?
2. Do I handle secret data in constant time?
3. Does my memory usage not balloon absurdly?
For (1), a BIGNUM function must give the same answer for all input
widths. BN_mod_add_quick may assume |a| < |m|, but |a| may still be
wider than |m| by way of leading zeres. The simplest approach is to
write code in a width-agnostic way and rely on functions to accept all
widths. Where functions need to look at bn->d, we'll a few helper
functions to smooth over funny widths.
For (2), (1) is little cumbersome. Consider constant-time modular
addition. A sane type system would guarantee input widths match. But C
is weak here, and bifurcating the internals is a lot of work. Thus, at
least for now, I do not propose we move RSA's internal computation out
of BIGNUM. (EC_SCALAR/EC_FELEM are valuable for EC because we get to
stack-allocate, curves were already specialized, and EC only has two
types with many operations on those types. None of these apply to RSA.
We've got numbers mod n, mod p, mod q, and their corresponding
exponents, each of which is used for basically one operation.)
Instead, constant-time BIGNUM functions will output non-minimal widths.
This is trivial for BN_bin2bn or modular arithmetic. But for BN_mul,
constant-time[*] would dictate r->top = a->top + b->top. A calculator
repeatedly multiplying by one would then run out of memory. Those we'll
split into a private BN_mul_fixed for crypto, leaving BN_mul for
calculators. BN_mul is just BN_mul_fixed followed by bn_correct_top.
[*] BN_mul is not constant-time for other reasons, but that will be
fixed separately.
Bug: 232
Change-Id: Ide2258ae8c09a9a41bb71d6777908d1c27917069
Reviewed-on: https://boringssl-review.googlesource.com/25244
Reviewed-by: Adam Langley <agl@google.com>
(The BN_num_bits_word implementation was originally written by Andy
Polyakov for OpenSSL. See also
https://github.com/openssl/openssl/pull/5154.)
BN_num_bits, by way of BN_num_bits_word, currently leaks the
most-significant word of its argument via branching and memory access
pattern.
BN_num_bits is called on RSA prime factors in various places. These have
public bit lengths, but all bits beyond the high bit are secret. This
fully resolves those cases.
There are a few places where BN_num_bits is called on an input where
the bit length is also secret. The two left in BoringSSL are:
- BN_mod_exp_mont_consttime calls it on the RSA private exponent.
- The timing "fix" to add the order to k in DSA.
This does *not* fully resolve those cases as we still only look at the
top word. Today, that is guaranteed to be non-zero, but only because of
the long-standing bn_correct_top timing leak. Once that is fixed (I hope
to have patches soon), a constant-time BN_num_bits on such inputs must
count bits on each word.
Instead, those cases should not call BN_num_bits at all. The former uses
the bit width to pick windows, but it should be using the maximum bit
width. The next patch will fix this. The latter is the same "fix" we
excised from ECDSA in a838f9dc7e. That
should be excised from DSA after the bn_correct_top bug is fixed.
Thanks to Dinghao Wu, Danfeng Zhang, Shuai Wang, Pei Wang, and Xiao Liu
for reporting this issue.
Change-Id: Idc3da518cc5ec18bd8688b95f959b15300a57c14
Reviewed-on: https://boringssl-review.googlesource.com/25184
Reviewed-by: Adam Langley <agl@google.com>
These can be used to invert values in ECDSA. Unlike their BIGNUM
counterparts, the caller is responsible for taking values in and out of
Montgomery domain. This will save some work later on in the ECDSA
computation.
Change-Id: Ib7292900a0fdeedce6cb3e9a9123c94863659043
Reviewed-on: https://boringssl-review.googlesource.com/23071
Reviewed-by: Adam Langley <agl@google.com>
These use the square and multiply functions added earlier.
Change-Id: I723834f9a227a9983b752504a2d7ce0223c43d24
Reviewed-on: https://boringssl-review.googlesource.com/23070
Reviewed-by: Adam Langley <agl@google.com>
As part of excising BIGNUM from EC scalars, we will need a "words"
version of BN_mod_mul_montgomery. That, in turn, requires BN_sqr and
BN_mul for cases where we don't have bn_mul_mont.
BN_sqr and BN_mul have a lot of logic in there, with the most complex
cases being not even remotely constant time. Fortunately, those only
apply to RSA-sized numbers, not EC-sized numbers. (With the exception, I
believe, of 32-bit P-521 which just barely exceeds the cutoff.) Imposing
a limit also makes it easier to stack-allocate temporaries (BN_CTX
serves a similar purpose in BIGNUM).
Extract bn_mul_small and bn_sqr_small and test them as part of
bn_tests.txt. Later changes will build on these.
If we end up reusing these functions for RSA in the future (though that
would require tending to the egregiously non-constant-time code in the
no-asm build), we probably want to extract a version where there is an
explicit tmp parameter as in bn_sqr_normal rather than the stack bits.
Change-Id: If414981eefe12d6664ab2f5e991a359534aa7532
Reviewed-on: https://boringssl-review.googlesource.com/23068
Reviewed-by: Adam Langley <agl@google.com>
This rewrites the internals with a "words" variant that can avoid
bn_correct_top. It still ultimately calls bn_correct_top as the calling
convention is sadly still BIGNUM, but we can lift that calling
convention out incrementally.
Performance seems to be comparable, if not faster.
Before:
Did 85000 ECDSA P-256 signing operations in 5030401us (16897.3 ops/sec)
Did 34278 ECDSA P-256 verify operations in 5048029us (6790.4 ops/sec)
After:
Did 85000 ECDSA P-256 signing operations in 5021057us (16928.7 ops/sec)
Did 34086 ECDSA P-256 verify operations in 5010416us (6803.0 ops/sec)
Change-Id: I1159746dfcc00726dc3f28396076a354556e6e7d
Reviewed-on: https://boringssl-review.googlesource.com/23065
Reviewed-by: Adam Langley <agl@google.com>
Those EXPECTs should be ASSERTs to ensure bn is not null.
Change-Id: Icb54c242ffbde5f8eaa67f19f214c9eef13705ea
Reviewed-on: https://boringssl-review.googlesource.com/22366
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I've left EVP_set_buggy_rsa_parser as a no-op stub for now, but it
shouldn't need to last very long. (Just waiting for a CL to land in a
consumer.)
Bug: chromium:735616
Change-Id: I6426588f84dd0803661a79c6636a0414f4e98855
Reviewed-on: https://boringssl-review.googlesource.com/22124
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
crypto/{asn1,x509,x509v3,pem} were skipped as they are still OpenSSL
style.
Change-Id: I3cd9a60e1cb483a981aca325041f3fbce294247c
Reviewed-on: https://boringssl-review.googlesource.com/19504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
