Commit Graph

5293 Commits

Author SHA1 Message Date
David Benjamin
bb2e1e1eea No-op comment to kick the bots.
Trigger some builds to see if the new kernel took.

Change-Id: Ib06c67b5da315ac46a757602abbf76626f46b279
2018-04-09 19:38:45 -04:00
David Benjamin
628b3c7f2f Don't write out a bad OID
If we don't have OID data for an object then we should fail if we
are asked to encode the ASN.1 for that OID.

(Imported from upstream's f3f8e72f494b36d05e0d04fe418f92b692fbb261.)

Change-Id: I3c3d3a3b236bca374fde3c0d02504140f2992602
Reviewed-on: https://boringssl-review.googlesource.com/27065
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>
2018-04-05 23:56:01 +00:00
David Benjamin
dcd862c1cc No-op commit to kick the bots.
This is to confirm what kernel the bots are running, now that we've got
uname -a in there.

Change-Id: I8e940c6b1c1f2fc971da3bbcf28f0bc4f543841e
2018-04-05 17:13:18 -04:00
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
Steven Valdez
861f384d7b Implement TLS 1.3 draft28.
Change-Id: I7298c878bd2c8187dbd25903e397e8f0c2575aa4
Reviewed-on: https://boringssl-review.googlesource.com/26846
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:36:11 +00:00
David Benjamin
eda47f5d98 Make generic point arithmetic slightly less variable-time.
The generic code special-cases affine points, but this leaks
information. (Of course, the generic code also doesn't have a
constant-time multiply and other problems, but one thing at a time.)

The optimization in point doubling is not useful. Point multiplication
more-or-less never doubles an affine point. The optimization in point
addition *is* useful because the wNAF code converts the tables to
affine. Accordingly, align with the P-256 code which adds a 'mixed'
parameter.

(I haven't aligned the formally-verified point formulas themselves yet;
initial testing suggests that the large number of temporaries take a
perf hit with BIGNUM. I'll check the results in EC_FELEM, which will be
stack-allocated, to see if we still need to help the compiler out.)

Strangly, it actually got a bit faster with this change. I'm guessing
because now it doesn't need to bother with unnecessary comparisons and
maybe was kinder to the branch predictor?

Before:
Did 2201 ECDH P-384 operations in 3068341us (717.3 ops/sec)
Did 4092 ECDSA P-384 signing operations in 3076981us (1329.9 ops/sec)
Did 3503 ECDSA P-384 verify operations in 3024753us (1158.1 ops/sec)
Did 992 ECDH P-521 operations in 3017884us (328.7 ops/sec)
Did 1798 ECDSA P-521 signing operations in 3059000us (587.8 ops/sec)
Did 1581 ECDSA P-521 verify operations in 3033142us (521.2 ops/sec)

After:
Did 2310 ECDH P-384 operations in 3092648us (746.9 ops/sec)
Did 4080 ECDSA P-384 signing operations in 3044588us (1340.1 ops/sec)
Did 3520 ECDSA P-384 verify operations in 3056070us (1151.8 ops/sec)
Did 992 ECDH P-521 operations in 3012779us (329.3 ops/sec)
Did 1792 ECDSA P-521 signing operations in 3019459us (593.5 ops/sec)
Did 1600 ECDSA P-521 verify operations in 3047749us (525.0 ops/sec)

Bug: 239
Change-Id: If5d13825fc98e4c58bdd1580cf0245bf7ce93a82
Reviewed-on: https://boringssl-review.googlesource.com/27004
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>
2018-04-04 21:33:22 +00:00
Matthew Braithwaite
56986f905f Hand back ECDHE split handshakes after the first server message.
This changes the contract for split handshakes such that on the
receiving side, the connection is to be driven until it returns
|SSL_ERROR_HANDBACK|, rather than until SSL_do_handshake() returns
success.

Change-Id: Idd1ebfbd943d88474d7c934f4c0ae757ff3c0f37
Reviewed-on: https://boringssl-review.googlesource.com/26864
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2018-04-04 17:58:15 +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
David Benjamin
7a62ab1938 Clarify BN_prime_checks is only for random candidates.
The relevant result (Damgård, Landrock, and Pomerance, Average Case
Error Estimates for the Strong Probably Prime Test) is only applicable
for randomly selected candidates. It relies on there being very few odd
composites with many false witnesses.

(If testing an adversarially-selected composite, false witnesses are
bounded by ϕ(n)/4 for n != 9, so one needs about 40 iterations for a
2^-80 false positive rate.)

Change-Id: I2a063dac5f9042dcb9e6affee8d2ae575f2238a9
Reviewed-on: https://boringssl-review.googlesource.com/26972
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:29:56 +00:00
David Benjamin
5b05988add Implement field_{mul,sqr} in p224-64.c with p224_felems.
This is in preparation for representing field elements with
stack-allocated types in the generic code. While there is likely little
benefit in threading all the turned field arithmetic through all the
generic code, and the P-224 logic, in particular, does not have a tight
enough abstraction for this, the current implementations depend on
BN_div, which is not compatible with stack-allocating things and avoiding
malloc.

This also speeds things up slightly, now that benchmarks cover point
validation.

Before:
Did 82786 ECDH P-224 operations in 10024326us (8258.5 ops/sec)
After:
Did 89991 ECDH P-224 operations in 10012429us (8987.9 ops/sec)

Change-Id: I468483b49f5dc69187aebd62834365ce5caab795
Reviewed-on: https://boringssl-review.googlesource.com/26971
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:27:45 +00:00
David Benjamin
c81ecf3436 Add test coverage for the a != -3 case.
Alas, it is reachable by way of the legacy custom curves API. Add a
basic test to ensure those codepaths work.

Change-Id: If631110045a664001133a0d07fdac4c67971a15f
Reviewed-on: https://boringssl-review.googlesource.com/26970
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:25:08 +00:00
David Benjamin
88b1a37e88 Include EC_POINT_oct2point in ECDH benchmarks.
This includes a point validation, which figures into the overall cost of
an ECDH operation. If, say, point validation is slow because it uses
generic code, we'd like it to show up in benchmarks.

(Later I'd like to replace this mess with a simple byte-oriented ECDH
API. When that happens, I'll update the benchmark accordingly.)

Change-Id: If8c33542d4b40572aac0a71ea2f658e7bc501f4b
Reviewed-on: https://boringssl-review.googlesource.com/26969
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:24:02 +00:00
David Benjamin
04018c5929 Remove EC_LOOSE_SCALAR.
ECDSA converts digests to scalars by taking the leftmost n bits, where n
is the number of bits in the group order. This does not necessarily
produce a fully-reduced scalar.

Montgomery multiplication actually tolerates this slightly looser bound,
so we did not bother with the conditional subtraction. However, this
subtraction is free compared to the multiplication, inversion, and base
point multiplication. Simplify things by keeping it fully-reduced.

Change-Id: If49dffefccc21510f40418dc52ea4da7e3ff198f
Reviewed-on: https://boringssl-review.googlesource.com/26968
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:22:58 +00:00
David Benjamin
9c1f8b4ac7 Add tests for large digests.
ECDSA's logic for converting digests to scalars sometimes produces
slightly unreduced values. Test these cases.

Change-Id: I67a5078db684ee82c286f41e71b13b57c3ee707b
Reviewed-on: https://boringssl-review.googlesource.com/26967
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:18:23 +00:00
David Benjamin
2257e8f3bf Use bn_rshift_words for the ECDSA bit-shift.
May as well use it. Also avoid an overflow with digest_len if someone
asks to sign a truly enormous digest.

Change-Id: Ia0a53007a496f9c7cadd44b1020ec2774b310936
Reviewed-on: https://boringssl-review.googlesource.com/26966
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:17:39 +00:00
David Benjamin
0645c05f5e Test the bit-shifting case in ECDSA.
For non-custom curves, this only comes up with P-521 and, even then,
only with excessively large hashes. Still, we should have test coverage
for this.

Change-Id: Id17a6f47d59d6dd4a43a93857fd3df490f9fa965
Reviewed-on: https://boringssl-review.googlesource.com/26965
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:14:27 +00:00
David Benjamin
cbe77925f4 Extract the single-subtraction reduction into a helper function.
We do this in four different places, with the same long comment, and I'm
about to add yet another one.

Change-Id: If28e3f87ea71020d9b07b92e8947f3848473d99d
Reviewed-on: https://boringssl-review.googlesource.com/26964
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:13:45 +00:00
David Benjamin
25f3d84f4c Rewrite BN_rand without an extra malloc.
RSA keygen uses this to pick primes. May as well avoid bouncing on
malloc. (The BIGNUM internally allocates, of course, but that allocation
will be absorbed by BN_CTX in RSA keygen.)

Change-Id: Ie2243a6e48b9c55f777153cbf67ba5c06688c2f1
Reviewed-on: https://boringssl-review.googlesource.com/26887
Reviewed-by: Adam Langley <agl@google.com>
2018-04-02 18:07:12 +00:00
David Benjamin
85c2cd8a45 Fix up AUTHORITY_INFO_ACCESS/ACCESS_DESCRIPTION's deleter.
AUTHORITY_INFO_ACCESS is a STACK_OF(ACCESS_DESCRIPTION), so we want to
add a deleter for ACCESS_DESCRIPTION, at which point
AUTHORITY_INFO_ACCESS's deleter will show up for free.

Change-Id: Id9efb74093868c39a893de67dd26f1fc15379252
Reviewed-on: https://boringssl-review.googlesource.com/26973
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-04-02 17:07:46 +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
Adam Langley
a0f1c8e3b1 Add RSA key generation to speed.cc
On a Skylake machine, the improvements to make RSA key generation
constant-time did slow things down a bit:

Before:

Did 217 RSA 2048 key-gen operations in 30231344us (7.2 ops/sec)
  min: 17154us, median: 117284us, max: 518336us
Did 70 RSA 3072 key-gen operations in 30188611us (2.3 ops/sec)
  min: 57759us, median: 348873us, max: 1760351us
Did 27 RSA 4096 key-gen operations in 30264235us (0.9 ops/sec)
  min: 202096us, median: 980160us, max: 4282915us

After:

Did 186 RSA 2048 key-gen operations in 30021173us (6.2 ops/sec)
  min: 74850us, median: 147650us, max: 407031us
Did 54 RSA 3072 key-gen operations in 30111667us (1.8 ops/sec)
  min: 292050us, median: 483786us, max: 1294105us
Did 18 RSA 4096 key-gen operations in 30662495us (0.6 ops/sec)
  min: 902547us, median: 1446689us, max: 3660302us

Change-Id: I52a96bb41bab759aa7ef6239bdfa533707a9eb3c
Reviewed-on: https://boringssl-review.googlesource.com/26904
Commit-Queue: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-30 20:53:35 +00:00
David Benjamin
5833dd807e Limit the public exponent in RSA_generate_key_ex.
Windows CryptoAPI and Go bound public exponents at 2^32-1, so don't
generate keys which would violate that.

https://github.com/golang/go/issues/3161
https://msdn.microsoft.com/en-us/library/aa387685(VS.85).aspx

BoringSSL itself also enforces a 33-bit limit.

I don't currently have plans to take much advantage of it, but the
modular inverse step and one of the GCDs in RSA key generation are
helped by small public exponents[0]. In case someone feels inspired
later, get this limit enforced now. Use 32-bits as that's a more
convenient limit, and there's no requirement to produce e=2^32+1 keys.
(Is there still a requirement to accept them?)

[0] This isn't too bad, but it's only worth it if it produces simpler or
smaller code. RSA keygen is not performance-critical.

1. Make bn_mod_u16_consttime work for uint32_t. It only barely doesn't
   work. Maybe only accept 3 and 65537 and pre-compute, maybe call into
   bn_div_rem_words and friends, maybe just tighten the bound a hair
   longer.
2. Implement bn_div_u32_consttime by incorporating 32-bit chunks much
   like bn_mod_u32_consttime.
3. Perform one normal Euclidean algorithm iteration rather than using the
   binary version. u, v, B, and D are now single words, while A and C
   are full-width.
4. Continue with binary Euclidean algorithm (u and v are still secret),
   taking advantage of most values being small.

Update-Note: RSA_generate_key_ex will no longer generate keys with
   public exponents larger than 2^32-1. Everyone uses 65537, save some
   folks who use 3, so this shouldn't matter.

Change-Id: I0d28a29a30d9ff73bff282e34dd98e2b64c35c79
Reviewed-on: https://boringssl-review.googlesource.com/26365
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:54:18 +00:00
David Benjamin
c1c6eeb5e2 Check d is mostly-reduced in RSA_check_key.
We don't check it is fully reduced because different implementations use
Carmichael vs Euler totients, but if d exceeds n, something is wrong.
Note the fixed-width BIGNUM changes already fail operations with
oversized d.

Update-Note: Some blatantly invalid RSA private keys will be rejected at
    RSA_check_key time. Note that most of those keys already are not
    usable with BoringSSL anyway. This CL moves the failure from
    sign/decrypt to RSA_check_key.

Change-Id: I468dbba74a148aa58c5994cc27f549e7ae1486a2
Reviewed-on: https://boringssl-review.googlesource.com/26374
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:54:10 +00:00
David Benjamin
cba958f406 Make RSA_check_key constant-time and more meaningful.
Rather than recompute values the same as in key generation, where
possible, we check differently. In particular, most RSA values are
modular inverses of some value. Check each of them by multiplying and
using our naive constant-time division function.

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

Bug: 238
Change-Id: Iaca19f12c045457013def844a17bf502ed09136e
Reviewed-on: https://boringssl-review.googlesource.com/26373
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:54:00 +00:00
David Benjamin
c4e4757b63 Make RSA key generation constant-time.
This leaves RSA_check_key, which will be fixed in subsequent commits.

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

Bug: 238
Change-Id: I325f23fcc59302e68570908e5427b65471b799f6
Reviewed-on: https://boringssl-review.googlesource.com/26371
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:53:52 +00:00
David Benjamin
a44dae7fd3 Add a constant-time generic modular inverse function.
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>
2018-03-30 19:53:44 +00:00
David Benjamin
1044553d6d Add new GCD and related primitives.
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>
2018-03-30 19:53:36 +00:00
David Benjamin
23af438ccd Compute p - q in constant time.
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>
2018-03-30 19:53:28 +00:00
David Benjamin
8d9ee7d1fe Replace rsa_greater_than_pow2 with BN_cmp.
It costs us a malloc, but it's one less function to test and implement
in constant time, now that BN_cmp and BIGNUM are okay.

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

Bug: 238
Change-Id: Ic56f92f0dcf04da1f542290a7e8cdab8036699ed
Reviewed-on: https://boringssl-review.googlesource.com/26367
Reviewed-by: Adam Langley <alangley@gmail.com>
2018-03-30 19:53:18 +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
40729e374d Revert "Update SDE to 8.16.0."
This reverts commit 21ef155063. Doesn't
look like I succeeded in uploading that. Will sort that out later.

Change-Id: Ic5395abe46b2b99aaffd254afcd97157518c8ba8
Reviewed-on: https://boringssl-review.googlesource.com/26886
Reviewed-by: David Benjamin <davidben@google.com>
2018-03-30 17:59:40 +00:00
David Benjamin
21ef155063 Update SDE to 8.16.0.
Change-Id: If6891b9338352d4f1dc9f902d4e32ca358764675
Reviewed-on: https://boringssl-review.googlesource.com/26885
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>
2018-03-30 17:52:56 +00:00
David Benjamin
365e48c104 Update tools.
Change-Id: Ibbba2e2fa81b5f8b3a25ebadc50297f50dcb610e
Reviewed-on: https://boringssl-review.googlesource.com/26884
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>
2018-03-30 17:51:56 +00:00
Adam Langley
1902d818ac Tighten and test name-checking functions.
This change follows up from e759a9cd with more extensive changes and
tests:

If a name checking function (like |X509_VERIFY_PARAM_set1_host|) fails,
it now poisons the |X509_VERIFY_PARAM| so that all verifications will
fail. This is because we have observed that some callers are not
checking the return value of these functions.

Using a length of zero for a hostname to mean |strlen| is now an error.
It also an error for email addresses and IP addresses now, and doesn't
end up trying to call |strlen| on a (binary) IP address.

Setting an email address with embedded NULs now fails. So does trying to
configure an empty hostname or email with (NULL, 0).

|X509_check_*| functions in BoringSSL don't accept zero lengths (unlike
OpenSSL). It's now tested that such calls always fail.

Change-Id: I4484176f2aae74e502a09081c7e912c85e8d090b
Update-Note: several behaviour changes. See change description.
Reviewed-on: https://boringssl-review.googlesource.com/26764
Reviewed-by: David Benjamin <davidben@google.com>
2018-03-30 16:50:11 +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
50418afb7f Add some EC base point multiplication test vectors.
Probably worth having actual test vectors for these, rather than
checking our code against itself. Additionally, small negative numbers
have, in the past been valuable test vectors (see long comment in
point_add from OpenSSL's ecp_nistp521.c).

Change-Id: Ia5aa8a80eb5b6d0089c3601c5fec2364e699794d
Reviewed-on: https://boringssl-review.googlesource.com/26848
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>
2018-03-27 23:33:24 +00:00
David Benjamin
718c88c961 Fix a bug in p224-64.c.
p224_felem_neg does not produce an output within the tight bounds
suitable for p224_felem_contract. This was found by inspection of the
code.

This only affects the final y-coordinate output of arbitrary-point
multiplication, so it is a no-op for ECDH and ECDSA.

Change-Id: I1d929458d1f21d02cd8e745d2f0f7040a6bb0627
Reviewed-on: https://boringssl-review.googlesource.com/26847
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-27 18:03:14 +00:00
Adam Langley
72bc2328b1 Note licenses for support code in the top-level LICENSE file.
This keeps some scripts happy.

Change-Id: I79be4f3d014b72fbe3f0793759ad2b42329a550c
Reviewed-on: https://boringssl-review.googlesource.com/26824
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-27 17:03:47 +00:00
David Benjamin
2e16f6ba81 Add a test for CRYPTO_memcmp.
This test is written in honor of CVE-2018-0733.

Change-Id: I8a41f917b08496870037f745f19bdcdb65b3d623
Reviewed-on: https://boringssl-review.googlesource.com/26845
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-27 16:22:47 +00:00
David Benjamin
2a19a17ca7 Limit ASN.1 constructed types recursive definition depth
Constructed types with a recursive definition could eventually exceed
the stack given malicious input with excessive recursion. Therefore we
limit the stack depth.

CVE-2018-0739

Credit to OSSFuzz for finding this issue.

(Imported from upstream's 9310d45087ae546e27e61ddf8f6367f29848220d.)

BoringSSL does not contain any such structures, but import this anyway
with a test.

Change-Id: I0e84578ea795134f25dae2ac8b565f3c26ef3204
Reviewed-on: https://boringssl-review.googlesource.com/26844
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-27 15:40:37 +00:00
David Benjamin
0970d397c4 Make various BIGNUM comparisons constant-time.
Primality testing checks for small words in random places.

Median of 29 RSA keygens: 0m0.811s -> 0m0.521s
(Accuracy beyond 0.1s is questionable, and this "speed up" is certainly
noise.)

Bug: 238
Change-Id: Ie5efab7291302a42ac6e283d25da0c094d8577e7
Reviewed-on: https://boringssl-review.googlesource.com/25885
Reviewed-by: Adam Langley <agl@google.com>
2018-03-26 18:53:53 +00:00
David Benjamin
ad066861dd Add bn_usub_fixed.
There are a number of random subtractions in RSA key generation. Add a
fixed-width version.

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

Bug: 238
Change-Id: I9fa0771b95a438fd7d2635fd77a332146ccc96d9
Reviewed-on: https://boringssl-review.googlesource.com/25884
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2018-03-26 18:53:43 +00:00
Adam Langley
d89d65ba12 Add utility program for emitting P-256 x86-64 table.
No semantic change: the table is the same as before, but now with less
magic.

Change-Id: I351c2446e9765f25b7dfb901c9e98f12099a325c
Reviewed-on: https://boringssl-review.googlesource.com/26744
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-26 16:28:42 +00:00
David Benjamin
6ebef73213 Add bssl::UniquePtr<AUTHORITY_INFO_ACCESS>
Change-Id: I8a0c1196bd455a9193c411764f26e662f5b98649
Reviewed-on: https://boringssl-review.googlesource.com/26804
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>
2018-03-26 15:36:33 +00:00
David Benjamin
5fca613918 Fix typo in point_add.
Rather than writing the answer into the output, it wrote it into some
awkwardly-named temporaries. Thanks to Daniel Hirche for reporting this
issue!

Bug: chromium:825273
Change-Id: I5def4be045cd1925453c9873218e5449bf25e3f5
Reviewed-on: https://boringssl-review.googlesource.com/26785
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>
2018-03-23 21:12:29 +00:00
David Benjamin
6291af4e52 Add -DOPENSSL_SMALL to CMake.
Adding preprocessor flags requires a lot of typing in the CMake
command-line (-DCMAKE_C_FLAGS=-DOPENSSL_SMALL
-DCMAKE_CXX_FLAGS=-DOPENSSL_SMALL).

Change-Id: Ieafc4155d656306c1f22746f780faa5c1d3e27be
Reviewed-on: https://boringssl-review.googlesource.com/26784
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>
2018-03-23 21:07:48 +00:00