Commit Graph

12 Commits

Author SHA1 Message Date
Adam Langley
b2eaeb0b8b Drop some trial-division primes for 1024-bit candidates.
This is helpful at smaller sizes because the benefits of an unlikely hit
by trival-division are smaller.

The full set of kPrimes eliminates about 94.3% of random numbers. The
first quarter eliminates about 93.2% of them. But the little extra power
of the full set seems to be borderline for RSA 3072 and clearly positive
for RSA 4096.

Did 316 RSA 2048 key-gen operations in 30035598us (10.5 ops/sec)
  min: 19423us, median: 80448us, max: 394265us

Change-Id: Iee53f721329674ae7a08fabd85b4f645c24e119d
Reviewed-on: https://boringssl-review.googlesource.com/26944
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
2018-04-05 03:53:01 +00:00
David Benjamin
ba9da449a4 Tolerate a null BN_CTX in BN_primality_test.
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>
2018-04-03 18:13:47 +00:00
Adam Langley
eb7c3008cc Only do 16 iterations to blind the primality test.
With this, in 0.02% of 1024-bit primes (which is what's used with an RSA
2048 generation), we'll leak that we struggled to generate values less
than the prime. I.e. that there's a greater likelihood of zero bits
after the leading 1 bit in the prime.

But this recovers all the speed loss from making key generation
constant-time, and then some.

Did 273 RSA 2048 key-gen operations in 30023223us (9.1 ops/sec)
  min: 23867us, median: 93688us, max: 421466us
Did 66 RSA 3072 key-gen operations in 30041763us (2.2 ops/sec)
  min: 117044us, median: 402095us, max: 1096538us
Did 31 RSA 4096 key-gen operations in 31673405us (1.0 ops/sec)
  min: 245109us, median: 769480us, max: 2659386us

Change-Id: Id82dedde35f5fbb36b278189c0685a13c7824590
Reviewed-on: https://boringssl-review.googlesource.com/26924
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 22:31:36 +00:00
David Benjamin
97ac45e2f7 Change the order of GCD and trial division.
RSA key generation currently does the GCD check before the primality
test, in hopes of discarding things invalid by other means before
running the expensive primality check.

However, GCD is about to get a bit more expensive to clear the timing
leak, and the trial division part of primality testing is quite fast.
Thus, split that portion out via a new bn_is_obviously_composite and
call it before GCD.

Median of 29 RSA keygens: 0m0.252s -> 0m0.207s
(Accuracy beyond 0.1s is questionable.)

Bug: 238
Change-Id: I3999771fb73cca16797cab9332d14c4ebeb02046
Reviewed-on: https://boringssl-review.googlesource.com/26366
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:53:06 +00:00
David Benjamin
56f5eb9ffd Name constant-time functions more consistently.
I'm not sure why I separated "fixed" and "quick_ctx" names. That's
annoying and doesn't generalize well to, say, adding a bn_div_consttime
function for RSA keygen.

Change-Id: I751d52b30e079de2f0d37a952de380fbf2c1e6b7
Reviewed-on: https://boringssl-review.googlesource.com/26364
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>
2018-03-29 23:30:55 +00:00
David Benjamin
e6f46e2563 Blind the range check for finding a Rabin-Miller witness.
Rabin-Miller requires selecting a random number from 2 to |w|-1.
This is done by picking an N-bit number and discarding out-of-range
values. This leaks information about |w|, so apply blinding. Rather than
discard bad values, adjust them to be in range.
Though not uniformly selected, these adjusted values
are still usable as Rabin-Miller checks.

Rabin-Miller is already probabilistic, so we could reach the desired
confidence levels by just suitably increasing the iteration count.
However, to align with FIPS 186-4, we use a more pessimal analysis: we
do not count the non-uniform values towards the iteration count. As a
result, this function is more complex and has more timing risk than
necessary.

We count both total iterations and uniform ones and iterate until we've
reached at least |BN_PRIME_CHECKS_BLINDED| and |iterations|,
respectively.  If the latter is large enough, it will be the limiting
factor with high probability and we won't leak information.

Note this blinding does not impact most calls when picking primes
because composites are rejected early. Only the two secret primes see
extra work.  So while this does make the BNTest.PrimeChecking test take
about 2x longer to run on debug mode, RSA key generation time is fine.

Another, perhaps simpler, option here would have to run
bn_rand_range_words to the full 100 count, select an arbitrary
successful try, and declare failure of the entire keygen process (as we
do already) if all tries failed. I went with the option in this CL
because I happened to come up with it first, and because the failure
probability decreases much faster. Additionally, the option in this CL
does not affect composite numbers, while the alternate would. This gives
a smaller multiplier on our entropy draw. We also continue to use the
"wasted" work for stronger assurance on primality. FIPS' numbers are
remarkably low, considering the increase has negligible cost.

Thanks to Nathan Benjamin for helping me explore the failure rate as the
target count and blinding count change.

Now we're down to the rest of RSA keygen, which will require all the
operations we've traditionally just avoided in constant-time code!

Median of 29 RSA keygens: 0m0.169s -> 0m0.298s
(Accuracy beyond 0.1s is questionable. The runs at subsequent test- and
rename-only CLs were 0m0.217s, 0m0.245s, 0m0.244s, 0m0.247s.)

Bug: 238
Change-Id: Id6406c3020f2585b86946eb17df64ac42f30ebab
Reviewed-on: https://boringssl-review.googlesource.com/25890
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2018-03-29 22:02:24 +00:00
David Benjamin
8eadca50a2 Don't leak |a| in the primality test.
(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>
2018-03-28 01:44:31 +00:00
David Benjamin
9362ed9e14 Use a Barrett reduction variant for trial division.
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>
2018-03-28 01:42:18 +00:00
David Benjamin
232a6be6f1 Make primality testing mostly constant-time.
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>
2018-03-28 01:42:06 +00:00
David Benjamin
f4b708cc1e Add a function which folds BN_MONT_CTX_{new,set} together.
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>
2018-02-02 20:23:25 +00:00
David Benjamin
808f832917 Run the comment converter on libcrypto.
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>
2017-08-18 21:49:04 +00:00
Adam Langley
5c38c05b26 Move bn/ into crypto/fipsmodule/
Change-Id: I68aa4a740ee1c7f2a308a6536f408929f15b694c
Reviewed-on: https://boringssl-review.googlesource.com/15647
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-05-01 22:51:25 +00:00