Imported from upstream's a78324d95bd4568ce2c3b34bfa1d6f14cddf92ef. I
think the "regression" part of that change is some tweak to BN_usub and
I guess the bn_*_words was to compensate for it, but we may as well
import it. Apparently the loop instruction is terrible.
Before:
Did 39871000 bn_add_words operations in 1000002us (39870920.3 ops/sec)
Did 38621750 bn_sub_words operations in 1000001us (38621711.4 ops/sec)
After:
Did 64012000 bn_add_words operations in 1000007us (64011551.9 ops/sec)
Did 81792250 bn_sub_words operations in 1000002us (81792086.4 ops/sec)
loop sets no flags (even doing the comparison to zero without ZF) while
dec sets all flags but CF, so Andres and I are assuming that because
this prevents Intel from microcoding it to dec/jnz, they otherwise can't
be bothered to add more circuitry since every compiler has internalized
by now to never use loop.
Change-Id: I3927cd1c7b707841bbe9963e3d4afd7ba9bd9b36
Reviewed-on: https://boringssl-review.googlesource.com/23344
Reviewed-by: Adam Langley <agl@google.com>
These allow precomputation of k, but bypass our nonce hardening and also
make it harder to excise BIGNUM. As a bonus, ECDSATest.SignTestVectors
is now actually covering the k^-1 and r computations.
Change-Id: I4c71dae162874a88a182387ac43999be9559ddd7
Reviewed-on: https://boringssl-review.googlesource.com/23074
Reviewed-by: Adam Langley <agl@google.com>
wpa_supplicant appear to be using these.
Change-Id: I1f220cae69162901bcd9452e8daf67379c5e276c
Reviewed-on: https://boringssl-review.googlesource.com/23324
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>
After much procrastinating, we finally moved Chromium to the new stuff.
We can now delete this. This is a breaking change for
SSL_PRIVATE_KEY_METHOD consumers, but it should be trivial (remove some
unused fields in the struct). I've bumped BORINGSSL_API_VERSION to ease
any multi-sided changes that may be needed.
Change-Id: I9fe562590ad938bcb4fcf9af0fadeff1d48745fb
Reviewed-on: https://boringssl-review.googlesource.com/23224
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
(Imported from upstream's c29f83c05f3a3c5641c5ddf054789a29d2163bf3.)
ext was being leaked. Upstream also did some stuff around *x which
wasn't strictly necessary (usually OpenSSL only provides basic
exception safety, not strong exception safety), but ah well.
Change-Id: I52d230990b05501b4cee6deee8dcacba4a926c18
Reviewed-on: https://boringssl-review.googlesource.com/23204
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>
mem.h for |OPENSSL_cleanse| and bn/internal.h for things like
|bn_less_than_words| and |bn_correct_top|.
Change-Id: I3c447a565dd9e4f18fb2ff5d59f80564b4df8cea
Reviewed-on: https://boringssl-review.googlesource.com/23164
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I82f92019dccfaf927f7180a5af53c9ffae111861
Reviewed-on: https://boringssl-review.googlesource.com/23145
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>
Change-Id: Id12ab478b6ba441fb1b6f4c2f9479384fc3fbdb6
Reviewed-on: https://boringssl-review.googlesource.com/23144
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
|EC_POINT_mul| is almost exclusively used with reduced scalars, with
this exception. This comes from consumers following NIST SP 800-56A
section 5.6.2.3.2. (Though all our curves have cofactor one, so this
check isn't useful.)
Add a test for this so we don't accidentally break it.
Change-Id: I42492db38a1ea03acec4febdd7945c8a3933530a
Reviewed-on: https://boringssl-review.googlesource.com/23084
Reviewed-by: Adam Langley <agl@google.com>
We were only running a random subset of TLS 1.3 tests with variants and
let a lot of bugs through as a result.
- HelloRetryRequest-EmptyCookie wasn't actually testing what we were
trying to test.
- The second HelloRetryRequest detection needs tweaks in draft-22.
- The empty HelloRetryRequest logic can't be based on non-empty
extensions in draft-22.
- We weren't sending ChangeCipherSpec correctly in HRR or testing it
right.
- Rework how runner reads ChangeCipherSpec by setting a flag which
affects the next readRecord. This cuts down a lot of cases and works
correctly if the client didn't send early data. (In that case, we
don't flush CCS until EndOfEarlyData and runner deadlocks waiting for
the ChangeCipherSpec to arrive.)
Change-Id: I559c96ea3a8b350067e391941231713c6edb2f78
Reviewed-on: https://boringssl-review.googlesource.com/23125
Reviewed-by: Steven Valdez <svaldez@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>
Change-Id: I9da9734625d1d9d2c783830d8b4aecd34f51acc6
Reviewed-on: https://boringssl-review.googlesource.com/23124
Reviewed-by: Steven Valdez <svaldez@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>
Error paths must always have OPENSSL_PUT_ERROR.
Change-Id: I0ed8c8288484a4ea69ec58317064ad3cd90ddd64
Reviewed-on: https://boringssl-review.googlesource.com/23104
Reviewed-by: Steven Valdez <svaldez@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>
I still need to revive the original CL, but right now I'm interested in
giving every EC_GROUP an order_mont and having different ownership of
that field between built-in and custom groups is kind of a nuisance. If
I'm going to do that anyway, better to avoid computing the entire
EC_GROUP in one go.
I'm using some manual locking rather than CRYPTO_once here so that it
behaves well in the face of malloc errors. Not that we especially care,
but it was easy to do.
This speeds up our ECDH benchmark a bit which otherwise must construct the
EC_GROUP each time (matching real world usage).
Before:
Did 7619 ECDH P-224 operations in 1003190us (7594.8 ops/sec)
Did 7518 ECDH P-256 operations in 1060844us (7086.8 ops/sec)
Did 572 ECDH P-384 operations in 1055878us (541.7 ops/sec)
Did 264 ECDH P-521 operations in 1062375us (248.5 ops/sec)
After:
Did 8415 ECDH P-224 operations in 1066695us (7888.9 ops/sec)
Did 7952 ECDH P-256 operations in 1022819us (7774.6 ops/sec)
Did 572 ECDH P-384 operations in 1055817us (541.8 ops/sec)
Did 264 ECDH P-521 operations in 1060008us (249.1 ops/sec)
Bug: 20
Change-Id: I7446cd0a69a840551dcc2dfabadde8ee1e3ff3e2
Reviewed-on: https://boringssl-review.googlesource.com/23073
Reviewed-by: Adam Langley <agl@google.com>
Later code will take advantage of these invariants. Enforcing them on
custom curves avoids making them go through a custom codepath.
Change-Id: I23cee72a90c2e4846b41e03e6be26bc3abeb4a45
Reviewed-on: https://boringssl-review.googlesource.com/23072
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>
bn_from_montgomery_in_place is actually constant-time. It is, of course,
only used by non-constant-time BIGNUM callers, but that will soon be
fixed.
Change-Id: I2b2c9943dc3b8d6a4b5b19a5bc4fa9ebad532bac
Reviewed-on: https://boringssl-review.googlesource.com/23069
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>
Also replace a pointless call to bn_mul_words with a memset.
Change-Id: Ief30ddab0e84864561b73fe2776bd0477931cf7f
Reviewed-on: https://boringssl-review.googlesource.com/23066
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>
BN_from_montgomery_word doesn't have a constant memory access pattern.
Replace the pointer trick with constant_time_select_w. There is, of
course, still the bn_correct_top leak pervasive in BIGNUM itself.
I wasn't able to measure a performance on RSA operations before or after
this change, but the benchmarks would vary wildly run to run. But one
would assume the logic here is nothing compared to the actual reduction.
Change-Id: Ide761fde3a091a93679f0a803a287aa5d0d4600d
Reviewed-on: https://boringssl-review.googlesource.com/22904
Reviewed-by: Adam Langley <agl@google.com>
We don't currently have test coverage for the order_mont bits (or lack
thereof) for custom curves.
Change-Id: I865d547c783226a5a3d3d203e10b0e59bad36984
Reviewed-on: https://boringssl-review.googlesource.com/23064
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>
Change-Id: Ic859f19edff281334bd6975dd3c3b2931c901021
Reviewed-on: https://boringssl-review.googlesource.com/23044
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>
The current PR says the sender only skips it during the handshake. Add a
test that we got this right.
Change-Id: Ib27eb942f11d955b8a24e32321efe474037f5254
Reviewed-on: https://boringssl-review.googlesource.com/23024
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
See https://github.com/tlswg/tls13-spec/pull/1083. We misread the
original text spec, but it turns out the original spec text required
senders have version-specific maximum send fragments. The PR fixes this
off-by-one issue. Align with the new spec text uniformly.
This is a wire format change for our existing drafts *only if* records
have padding. We don't currently send padding, so this is fine. Unpadded
records continue to be capped at 2^14 bytes of plaintext (or 2^14+1
bytes of TLSInnerPlaintext structure).
Change-Id: I01017cfd13162504bb163dd59afd74aff0896cc4
Reviewed-on: https://boringssl-review.googlesource.com/23004
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>
Change-Id: I1a0f264cbfa0eb5d4adac96d0fc24fa342f2b6a3
Reviewed-on: https://boringssl-review.googlesource.com/22946
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>
Because the handshake returns early, it should query SSL_in_early_data.
Change-Id: I64d4c0e8de753832207d5c198c50d660f87afac6
Reviewed-on: https://boringssl-review.googlesource.com/22945
Reviewed-by: Steven Valdez <svaldez@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>
Change-Id: Ie5055d6d1d33690f27cdd978a0aa696307880579
Reviewed-on: https://boringssl-review.googlesource.com/22964
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This introduces a wire change to Experiment2/Experiment3 over 0RTT, however
as there is never going to be a 0RTT deployment with Experiment2/Experiment3,
this is valid.
Change-Id: Id541d195cbc4bbb3df7680ae2a02b53bb8ae3eab
Reviewed-on: https://boringssl-review.googlesource.com/22744
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I76a87ebf2f8be731d6da2381710c1caa60298f6e
Reviewed-on: https://boringssl-review.googlesource.com/22924
Reviewed-by: Steven Valdez <svaldez@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>
This was primarily for my own understanding, but this should hopefully
also be clearer and more amenable to using unsigned indices later.
Change-Id: I09cc3d55de0f7d9284d3b3168d8b0446274b2ab7
Reviewed-on: https://boringssl-review.googlesource.com/22889
Reviewed-by: Adam Langley <agl@google.com>
Normal shifts do the trick just fine and are less likely to tempt the
compiler into inserting a jump.
Change-Id: Iaa1da1b6f986fd447694fcde8f3525efb9eeaf11
Reviewed-on: https://boringssl-review.googlesource.com/22888
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I482093000ee2e4ba371c78b4f7f8e8b121e71640
Reviewed-on: https://boringssl-review.googlesource.com/22886
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We capitalize things Go-style.
Change-Id: Id002efb8a85e4e1886164421bba059d9ca425964
Reviewed-on: https://boringssl-review.googlesource.com/22885
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BN_generate_dsa_nonce will never generate a zero value of k.
Change-Id: I06964b815bc82aa678ffbc80664f9d788cf3851d
Reviewed-on: https://boringssl-review.googlesource.com/22884
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>
Even without strict-aliasing, C does not allow casting pointers to types
that don't match their alignment. After this change, UBSan is happy with
our code at default settings but for the negative left shift language
bug.
Note: architectures without unaligned loads do not generate the same
code for memcpy and pointer casts. But even ARMv6 can perform unaligned
loads and stores (ARMv5 couldn't), so we should be okay here.
Before:
Did 11086000 AES-128-GCM (16 bytes) seal operations in 5000391us (2217026.6 ops/sec): 35.5 MB/s
Did 370000 AES-128-GCM (1350 bytes) seal operations in 5005208us (73923.0 ops/sec): 99.8 MB/s
Did 63000 AES-128-GCM (8192 bytes) seal operations in 5029958us (12525.0 ops/sec): 102.6 MB/s
Did 9894000 AES-256-GCM (16 bytes) seal operations in 5000017us (1978793.3 ops/sec): 31.7 MB/s
Did 316000 AES-256-GCM (1350 bytes) seal operations in 5005564us (63129.7 ops/sec): 85.2 MB/s
Did 54000 AES-256-GCM (8192 bytes) seal operations in 5054156us (10684.3 ops/sec): 87.5 MB/s
After:
Did 11026000 AES-128-GCM (16 bytes) seal operations in 5000197us (2205113.1 ops/sec): 35.3 MB/s
Did 370000 AES-128-GCM (1350 bytes) seal operations in 5005781us (73914.5 ops/sec): 99.8 MB/s
Did 63000 AES-128-GCM (8192 bytes) seal operations in 5032695us (12518.1 ops/sec): 102.5 MB/s
Did 9831750 AES-256-GCM (16 bytes) seal operations in 5000010us (1966346.1 ops/sec): 31.5 MB/s
Did 316000 AES-256-GCM (1350 bytes) seal operations in 5005702us (63128.0 ops/sec): 85.2 MB/s
Did 54000 AES-256-GCM (8192 bytes) seal operations in 5053642us (10685.4 ops/sec): 87.5 MB/s
(Tested with the no-asm builds; most of this code isn't reachable
otherwise.)
Change-Id: I025c365d26491abed0116b0de3b7612159e52297
Reviewed-on: https://boringssl-review.googlesource.com/22804
Reviewed-by: Adam Langley <agl@google.com>
This avoids upsetting the C compiler. UBSan is offended by the alignment
violations in those functions. The business with offset is also
undefined behavior (pointer arithmetic is supposed to stay within a
single object).
There is a small performance cost, however:
Before:
Did 6636000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000475us (1327073.9 ops/sec): 21.2 MB/s
Did 832000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5003481us (166284.2 ops/sec): 224.5 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5026933us (30833.9 ops/sec): 252.6 MB/s
After:
Did 6508000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000160us (1301558.4 ops/sec): 20.8 MB/s
Did 831000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5002865us (166104.8 ops/sec): 224.2 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5013204us (30918.4 ops/sec): 253.3 MB/s
(Tested with the no-asm build which disables the custom stitched mode
assembly and ends up using this one.)
Change-Id: I76d74183f1e04ad3726463a8871ee64be04ce674
Reviewed-on: https://boringssl-review.googlesource.com/22784
Reviewed-by: Adam Langley <agl@google.com>
These functions don't appear to do any stack manipulation thus all they
need are start/end directives in order for the correct CFI tables to be
emitted.
Change-Id: I4c94a9446030d363fa4bcb7c8975c689df3d21dc
Reviewed-on: https://boringssl-review.googlesource.com/22765
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>
Change-Id: Id70cfc78c8d103117d4c2195206b023a5d51edc3
Reviewed-on: https://boringssl-review.googlesource.com/22764
Commit-Queue: Adam Langley <agl@google.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>
It's not clear if it's a feature or bug, but binutils-2.29[.1]
interprets 'adr' instruction with Thumb2 code reference differently,
in a way that affects calculation of addresses of constants' tables.
(Imported from upstream's b82acc3c1a7f304c9df31841753a0fa76b5b3cda.)
Change-Id: Ia0f5233a9fcfaf18b9d1164bf1c88217c0cbb60d
Reviewed-on: https://boringssl-review.googlesource.com/22724
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>
