EVP_AEAD reused portions of EVP_CIPHER's GCM128_CONTEXT which contains both the
key and intermediate state for each operation. (The legacy OpenSSL EVP_CIPHER
API has no way to store just a key.) Split out a GCM128_KEY and store that
instead.
Change-Id: Ibc550084fa82963d3860346ed26f9cf170dceda5
Reviewed-on: https://boringssl-review.googlesource.com/32004
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>
The check of `r` instead of `rr` was introduced in change
I298400b988e3bd108d01d6a7c8a5b262ddf81feb.
Change-Id: I4376a81c65856f6457b0a11276176bf35e9c647d
Reviewed-on: https://boringssl-review.googlesource.com/31844
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 doesn't really matter, but once less visible symbol.
Change-Id: If4ee8cfe5c9db9d1c05ca74b8c6fee5cf3ea5a9b
Reviewed-on: https://boringssl-review.googlesource.com/31764
Reviewed-by: Adam Langley <agl@google.com>
The assembly files need some includes. Also evp.h has some conflicting
macros. Finally, md5.c's pattern of checking if a function name is
defined needs to switch to checking MD5_ASM.
Change-Id: Ib1987ba6f279144f0505f6951dead53968e05f20
Reviewed-on: https://boringssl-review.googlesource.com/31704
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>
- In base.h, if BORINGSSL_PREFIX is defined, include
boringssl_prefix_symbols.h
- In all .S files, if BORINGSSL_PREFIX is defined, include
boringssl_prefix_symbols_asm.h
- In base.h, BSSL_NAMESPACE_BEGIN and BSSL_NAMESPACE_END are
defined with appropriate values depending on whether
BORINGSSL_PREFIX is defined; these macros are used in place
of 'namespace bssl {' and '}'
- Add util/make_prefix_headers.go, which takes a list of symbols
and auto-generates the header files mentioned above
- In CMakeLists.txt, if BORINGSSL_PREFIX and BORINGSSL_PREFIX_SYMBOLS
are defined, run util/make_prefix_headers.go to generate header
files
- In various CMakeLists.txt files, add "global_target" that all
targets depend on to give us a place to hook logic that must run
before all other targets (in particular, the header file generation
logic)
- Document this in BUILDING.md, including the fact that it is
the caller's responsibility to provide the symbol list and keep it
up to date
- Note that this scheme has not been tested on Windows, and likely
does not work on it; Windows support will need to be added in a
future commit
Change-Id: If66a7157f46b5b66230ef91e15826b910cf979a2
Reviewed-on: https://boringssl-review.googlesource.com/31364
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>
https://boringssl-review.googlesource.com/31085 wasn't right. We already forbid
creating BN_MONT_CTX on negative numbers, which means almost all moduli already
don't work with BN_mod_exp_mont. Only -1 happened to not get rejected, but it
computed the wrong value. Reject it instead.
Update-Note: BN_mod_exp* will no longer work for negative moduli. It already
didn't work for all negative odd moduli other than -1, so rejecting -1 and
negative evens is unlikely to be noticed.
Bug: 71
Change-Id: I7c713d417e2e6512f3e78f402de88540809977e3
Reviewed-on: https://boringssl-review.googlesource.com/31484
Reviewed-by: Adam Langley <agl@google.com>
This often causes confusion since, for various silly reasons (intrinsic
ref-counting, FOO_METHOD, and RSA's cached Montgomery bits), the thread
safety of some functions don't match the usual const/non-const
distinction. Fix const-ness where easy and document it otherwise.
Change-Id: If2037a4874d7580cc79b18ee21f12ae0f47db7fd
Reviewed-on: https://boringssl-review.googlesource.com/31344
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
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>
Change-Id: I2d1671a4f21a602191fd0c9b932244a376ac5713
Reviewed-on: https://boringssl-review.googlesource.com/31104
Reviewed-by: David Benjamin <davidben@google.com>
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>
Historically, OpenSSL's modular exponentiation functions tolerated negative
moduli by ignoring the sign bit. The special case for a modulus of 1 should do
the same. That said, this is ridiculous and the only reason I'm importing this
is BN_abs_is_word(1) is marginally more efficient than BN_is_one() and we
haven't gotten around to enforcing positive moduli yet.
Thanks to Guido Vranken and OSSFuzz for finding this issue and reporting to
OpenSSL.
(Imported from upstream's 235119f015e46a74040b78b10fd6e954f7f07774.)
Change-Id: I526889dfbe2356753aa1e6ecfd3aa3dc3a8cd2b8
Reviewed-on: https://boringssl-review.googlesource.com/31085
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This imports upstream's be4e1f79f631e49c76d02fe4644b52f907c374b2.
Change-Id: If0c4f066ba0ce540beaddd6a3e2540165d949dd2
Reviewed-on: https://boringssl-review.googlesource.com/31024
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>
We currently write a mix of "if (FOO)" and "if(FOO)". While the former looks
more like a usual language, CMake believes everything, even "if" and "else", is
just a really really funny function call (a "command").
We should pick something for consistency. Upstream CMake writes "if(FOO)", so
go with that one.
Change-Id: I67e0eb650a52670110b417312a362c9f161c8721
Reviewed-on: https://boringssl-review.googlesource.com/30807
Reviewed-by: Adam Langley <agl@google.com>
Upstream generalized most of the EVP_CTRL_GCM_* constants to be their general
AEAD API in 1.1.0. Define them for better compatibility with code that targets
OpenSSL 1.1.0.
Change-Id: Ieaed8379eebde3718e3048f6290c21cdeac01efd
Reviewed-on: https://boringssl-review.googlesource.com/30604
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>
This change syncs several assembly files from upstream. The only meanful
additions are more CFI directives.
Change-Id: I6aec50b6fddbea297b79bae22cfd68d5c115220f
Reviewed-on: https://boringssl-review.googlesource.com/30364
Reviewed-by: Adam Langley <agl@google.com>
Setting OPENSSL_NO_ASM skips enabling the “ASM” language in CMake.
However, the FIPS module fundamentally needs to build asm because
delocate works via textual assembly. Thus this combination is currently
broken with CMake.
This change ensures that support for building asm is enabled in CMake
for this combination.
Change-Id: I4516cf3a6f579ee7c72f04ac25d15785926cf125
Reviewed-on: https://boringssl-review.googlesource.com/29884
Reviewed-by: Adam Langley <agl@google.com>
This change adds a function so that an ECDH and the hashing of the
resulting 'x' coordinate can occur inside the FIPS boundary.
Change-Id: If93c20a70dc9dcbca49056f10915d3ce064f641f
Reviewed-on: https://boringssl-review.googlesource.com/30104
Reviewed-by: Adam Langley <agl@google.com>
Otherwise, if the output BIGNUM was previously negative, we'd incorrectly give
a negative result. Thanks to Guide Vranken for reporting this issue!
Fortunately, this does not appear to come up in any existing caller. This isn't
all that surprising as negative numbers never really come up in cryptography.
Were it not for OpenSSL historically designing a calculator API, we'd just
delete the bit altogether. :-(
Bug: chromium:865924
Change-Id: I28fdc986dfaba3e38435b14ebf07453d537cc60a
Reviewed-on: https://boringssl-review.googlesource.com/29944
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>
Previously we used thread-local state objects in rand.c. However, for
applications with large numbers of threads, this can lead to excessive
memory usage.
This change causes us to maintain a mutex-protected pool of state
objects where the size of the pool equals the maximum concurrency of
|RAND_bytes|. This might lead to state objects bouncing between CPUs
more often, but should help the memory usage problem.
Change-Id: Ie83763d3bc139e64ac17bf7e015ad082b2f8a81a
Reviewed-on: https://boringssl-review.googlesource.com/29565
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Previously, delocate.go couldn't handle GOT references and so |stderr|
was a problematic symbol. We can cope with them now, so write FIPS
power-on test and urandom errors to stderr rather than stdout.
Change-Id: If6d7c19ee5f22dcbd74fb01c231500c2e130e6f7
Update-note: resolves internal bug 110102292.
Reviewed-on: https://boringssl-review.googlesource.com/29244
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>
This change adds an AES-GCM AEAD that enforces nonce uniqueness inside
the FIPS module, like we have for TLS 1.2. While TLS 1.3 has not yet
been mentioned in the FIPS 140 IG, we expect it to be in the next ~12
months and so are preparing for that.
Change-Id: I65a7d8196b08dc0033bdde5c844a73059da13d9e
Reviewed-on: https://boringssl-review.googlesource.com/29224
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
cryptography.io gets offended if the library supports some OFB sizes but
not others.
Change-Id: I7fc7b12e7820547a82aae84d9418457389a482fe
Reviewed-on: https://boringssl-review.googlesource.com/29204
Reviewed-by: Adam Langley <agl@google.com>
Although the original value of tmp does not matter, the selects
ultimately do bit operations on the uninitialized values and thus depend
on them behaving like *some* consistent concrete value. The C spec
appears to allow uninitialized values to resolve to trap
representations, which means this isn't quite valid..
(If I'm reading it wrong and the compiler must behave as if there were a
consistent value in there, it's probably fine, but there's no sense in
risking compiler bugs on a subtle corner of things.)
Change-Id: Id4547b0ec702414b387e906c4de55595e6214ddb
Reviewed-on: https://boringssl-review.googlesource.com/29124
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>
When building files separately, omitting this causes some #defines to be
missing.
Change-Id: I235231467d3f51ee0a53325698356aefa72c6a67
Reviewed-on: https://boringssl-review.googlesource.com/28944
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>
In neither OpenSSL nor BoringSSL can this function actually fail, but
OpenSSL makes it return one anyway. Match them for compatibility.
Change-Id: I497437321ad9ccc5da738f06cd5b19c467167575
Reviewed-on: https://boringssl-review.googlesource.com/28784
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>
|alloca| is dangerous and poorly specified, according to any
description of |alloca|. It's also hard for some analysis tools to
reason about.
The code here assumed |alloca| is a macro, which isn't a valid
assumption. Depending on what which headers are included and what
toolchain is being used, |alloca| may or may not be defined as a macro,
and this might change over time if/when toolchains are updated. Or, we
might be doing static analysis and/or dynamic analysis with a different
configuration w.r.t. the availability of |alloca| than production
builds use.
Regardless, the |alloca| code path only kicked in when the inputs are
840 bits or smaller. Since the multi-prime RSA support was removed, for
interesting RSA key sizes the input will be at least 1024 bits and this
code path won't be triggered since powerbufLen will be larger than 3072
bytes in those cases. ECC inversion via Fermat's Little Theorem has its
own constant-time exponentiation so there are no cases where smaller
inputs need to be fast.
The RSAZ code avoids the |OPENSSL_malloc| for 2048-bit RSA keys.
Increasingly the RSAZ code won't be used though, since it will be
skipped over on Broadwell+ CPUs. Generalize the RSAZ stack allocation
to work for non-RSAZ code paths. In order to ensure this doesn't cause
too much stack usage on platforms where RSAZ wasn't already being used,
only do so on x86-64, which already has this large stack size
requirement due to RSAZ.
This change will make it easier to refactor |BN_mod_exp_mont_consttime|
to do that more safely and in a way that's more compatible with various
analysis tools.
This is also a step towards eliminating the |uintptr_t|-based alignment
hack.
Since this change increases the number of times |OPENSSL_free| is
skipped, I've added an explicit |OPENSSL_cleanse| to ensure the
zeroization is done. This should be done regardless of the other changes
here.
Change-Id: I8a161ce2720a26127e85fff7513f394883e50b2e
Reviewed-on: https://boringssl-review.googlesource.com/28584
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>
Thanks to Brian Smith for pointing this out.
Change-Id: I27ae58df0028bc6aa3a11741acb5453369e202cc
Reviewed-on: https://boringssl-review.googlesource.com/28625
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>
cryptography.io wants things exposed out of EVP_get_cipherby* including,
sadly, ECB mode.
Change-Id: I9bac46f8ffad1a79d190cee3b0c0686bf540298e
Reviewed-on: https://boringssl-review.googlesource.com/28464
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>
Rather than have plain-C functions, asm functions, and accelerated
functions, just have accelerated and non-accelerated, where the latter
are either provided by assembly or by C code.
Pertinently, this allows Aarch64 to use hardware accel for the basic
|AES_*| functions.
Change-Id: I0003c0c7a43d85a3eee8c8f37697f61a3070dd40
Reviewed-on: https://boringssl-review.googlesource.com/28385
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>
cryptography.io wants RSA_R_BLOCK_TYPE_IS_NOT_02, only used by the
ancient RSA_padding_check_SSLv23 function. Define it but never emit it.
Additionally, it's rather finicky about RSA_R_TOO_LARGE* errors. We
merged them in BoringSSL because having RSA_R_TOO_LARGE,
RSA_R_TOO_LARGE_FOR_MODULUS, and RSA_R_TOO_LARGE_FOR_KEY_SIZE is a
little silly. But since we don't expect well-behaved code to condition
on error codes anyway, perhaps that wasn't worth it. Split them back
up.
Looking through OpenSSL, there is a vague semantic difference:
RSA_R_DIGEST_TOO_BIG_FOR_RSA_KEY - Specifically emitted if a digest is
too big for PKCS#1 signing with this key.
RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE - You asked me to sign or encrypt a
digest/plaintext, but it's too big for this key.
RSA_R_DATA_TOO_LARGE_FOR_MODULUS - You gave me an RSA ciphertext or
signature and it is not fully reduced modulo N.
-OR-
The padding functions produced something that isn't reduced, but I
believe this is unreachable outside of RSA_NO_PADDING.
RSA_R_DATA_TOO_LARGE - Some low-level padding function was told to copy
a digest/plaintext into some buffer, but the buffer was too small. I
think this is basically unreachable.
-OR-
You asked me to verify a PSS signature, but I didn't need to bother
because the digest/salt parameters you picked were too big.
Update-Note: This depends on cl/196566462.
Change-Id: I2e539e075eff8bfcd52ccde365e975ebcee72567
Reviewed-on: https://boringssl-review.googlesource.com/28547
Reviewed-by: Adam Langley <agl@google.com>
This was reverted a second time because it ended up always setting the
final argument to CRYPTO_gcm128_init to zero, which disabled some
acceleration of GCM on ≥Haswell. With this update, that argument will be
set to 1 if |aes_hw_*| functions are being used.
Probably this will need to be reverted too for some reason. I'm hoping
to fill the entire git short description with “Revert”.
Change-Id: Ib4a06f937d35d95affdc0b63f29f01c4a8c47d03
Reviewed-on: https://boringssl-review.googlesource.com/28484
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>
Build (and carry) issues are now resolved (as far as we know). Let's try
this again...
Measurements on a Skylake VM (so a little noisy).
Before:
Did 3135 RSA 2048 signing operations in 3015866us (1039.5 ops/sec)
Did 89000 RSA 2048 verify (same key) operations in 3007271us (29594.9 ops/sec)
Did 66000 RSA 2048 verify (fresh key) operations in 3014363us (21895.2 ops/sec)
Did 324 RSA 4096 signing operations in 3004364us (107.8 ops/sec)
Did 23126 RSA 4096 verify (same key) operations in 3003398us (7699.9 ops/sec)
Did 21312 RSA 4096 verify (fresh key) operations in 3017043us (7063.9 ops/sec)
Did 31040 ECDH P-256 operations in 3024273us (10263.6 ops/sec)
Did 91000 ECDSA P-256 signing operations in 3019740us (30135.0 ops/sec)
Did 25678 ECDSA P-256 verify operations in 3046975us (8427.4 ops/sec)
After:
Did 3640 RSA 2048 signing operations in 3035845us (1199.0 ops/sec)
Did 129000 RSA 2048 verify (same key) operations in 3003691us (42947.2 ops/sec)
Did 105000 RSA 2048 verify (fresh key) operations in 3029935us (34654.2 ops/sec)
Did 510 RSA 4096 signing operations in 3014096us (169.2 ops/sec)
Did 38000 RSA 4096 verify (same key) operations in 3092814us (12286.5 ops/sec)
Did 34221 RSA 4096 verify (fresh key) operations in 3003817us (11392.5 ops/sec)
Did 38000 ECDH P-256 operations in 3061758us (12411.2 ops/sec)
Did 116000 ECDSA P-256 signing operations in 3001637us (38645.6 ops/sec)
Did 35100 ECDSA P-256 verify operations in 3023872us (11607.6 ops/sec)
Tested with Intel SDE.
Change-Id: Ib27c0d6012d14274e331ab03f958e5a0c8b7e885
Reviewed-on: https://boringssl-review.googlesource.com/28104
Reviewed-by: Adam Langley <agl@google.com>
Broke Aarch64 on the main builders (but not the trybots, somehow.)
Change-Id: I53eb09c99ef42a59628b0506b5ddb125299b554a
Reviewed-on: https://boringssl-review.googlesource.com/28364
Reviewed-by: Adam Langley <agl@google.com>
This also happens to make the AES_[en|de]crypt functions use AES-NI
(where available) on Intel.
Update-Note: this substantially changes how AES-NI is triggered. Worth running bssl speed (on both k8 and ppc), before and after, to confirm that there are no regressions.
Change-Id: I5f22c1975236bbc1633c24ab60d683bca8ddd4c3
Reviewed-on: https://boringssl-review.googlesource.com/28026
Reviewed-by: David Benjamin <davidben@google.com>
gRPC builds on Debian Jessie, which has GCC 4.9.2, and builds with
-Wtype-limits, which makes it warn about code intended for 64-bit
systems when building on 32-bit systems.
We have tried to avoid these issues with Clang previously by guarding
with “sizeof(size_t) > 4”, but this version of GCC isn't smart enough to
figure that out.
Change-Id: I800ceb3891436fa7c81474ede4b8656021568357
Reviewed-on: https://boringssl-review.googlesource.com/28247
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 all new code. There was a request to make this available under
ISC.
Change-Id: Ibabbe6fbf593c2a781aac47a4de7ac378604dbcf
Reviewed-on: https://boringssl-review.googlesource.com/28267
Reviewed-by: Adam Langley <agl@google.com>
This happened to be working only because of lucky -I argument and At the
same time, include digest.h since this file references |EVP_sha1| and
other digest-related functions.
Change-Id: I0095ea8f5ef21f6e63b3dc819932b38178e09693
Reviewed-on: https://boringssl-review.googlesource.com/28244
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>
bcm.c means e_aes.c can no longer be lazy about warning push/pop.
Change-Id: I558041bab3baa00e3adc628fe19486545d0f6be3
Reviewed-on: https://boringssl-review.googlesource.com/28164
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>
Make it clear this is not a pristine full copy of all of Wycheproof as a
library.
Change-Id: I1aa5253a1d7c696e69b2e8d7897924f15303d9ac
Reviewed-on: https://boringssl-review.googlesource.com/28188
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>
The bug, courtesy of Wycheproof, is that AES key wrap requires the input
be at least two blocks, not one. This also matches the OpenSSL behavior
of those two APIs.
Update-Note: AES_wrap_key with in_len = 8 and AES_unwrap_key with
in_len = 16 will no longer work.
Change-Id: I5fc63ebc16920c2f9fd488afe8c544e0647d7507
Reviewed-on: https://boringssl-review.googlesource.com/27925
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: I0674f4e9b15b546237600fb2486c46aac7cb0716
Reviewed-on: https://boringssl-review.googlesource.com/28027
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>
Montgomery multiplication post-conditions in some of code paths were
formally non-constant time. Cache access pattern was result-neutral,
but a little bit asymmetric, which might have produced a signal [if
processor reordered load and stores at run-time].
(Imported from upstream's 774ff8fed67e19d4f5f0df2f59050f2737abab2a.)
Change-Id: I77443fb79242b77e704c34d69f1de9e3162e9538
Reviewed-on: https://boringssl-review.googlesource.com/27987
Reviewed-by: Adam Langley <agl@google.com>
We've never defined this so this code has always been dead.
Change-Id: Ibcc4095bf812c7e1866c5f39968789606f0995ae
Reviewed-on: https://boringssl-review.googlesource.com/28024
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>
Change-Id: Ib2ce220e31a4f808999934197a7f43b8723131e8
Reviewed-on: https://boringssl-review.googlesource.com/27884
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 is slower, but constant-time. It intentionally omits the signed
digit optimization because we cannot be sure the doubling case will be
unreachable for all curves. This is a fallback generic implementation
for curves which we must support for compatibility but which are not
common or important enough to justify curve-specific work.
Before:
Did 814 ECDH P-384 operations in 1085384us (750.0 ops/sec)
Did 1430 ECDSA P-384 signing operations in 1081988us (1321.6 ops/sec)
Did 308 ECDH P-521 operations in 1057741us (291.2 ops/sec)
Did 539 ECDSA P-521 signing operations in 1049797us (513.4 ops/sec)
After:
Did 715 ECDH P-384 operations in 1080161us (661.9 ops/sec)
Did 1188 ECDSA P-384 verify operations in 1069567us (1110.7 ops/sec)
Did 275 ECDH P-521 operations in 1060503us (259.3 ops/sec)
Did 506 ECDSA P-521 signing operations in 1084739us (466.5 ops/sec)
But we're still faster than the old BIGNUM implementation. EC_FELEM
more than paid for both the loss of points_make_affine and this CL.
Bug: 239
Change-Id: I65d71a731aad16b523928ee47618822d503ea704
Reviewed-on: https://boringssl-review.googlesource.com/27708
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>
w=4 appears to be the correct answer for P-224 through P-521. There's
nominally some optimizations in here for 70- and 20-bit primes, but
that's absurd.
Change-Id: Id4ccec779b17e375e9258c1784e46d7d3651c59a
Reviewed-on: https://boringssl-review.googlesource.com/27707
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>
EC_POINT is split into the existing public EC_POINT (where the caller is
sanity-checked about group mismatches) and the low-level EC_RAW_POINT
(which, like EC_FELEM and EC_SCALAR, assume that is your problem and is
a plain old struct). Having both EC_POINT and EC_RAW_POINT is a little
silly, but we're going to want different type signatures for functions
which return void anyway (my plan is to lift a non-BIGNUM
get_affine_coordinates up through the ECDSA and ECDH code), so I think
it's fine.
This wasn't strictly necessary, but wnaf.c is a lot tidier now. Perf is
a wash; once we get up to this layer, it's only 8 entries in the table
so not particularly interesting.
Bug: 239
Change-Id: I8ace749393d359f42649a5bb0734597bb7c07a2e
Reviewed-on: https://boringssl-review.googlesource.com/27706
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>
Replace them with asserts and better justify why each of the internal
cases are not reachable. Also change the loop to count up to bits+1 so
it is obvious there is no memory error. (The previous loop shape made
more sense when ec_compute_wNAF would return a variable length
schedule.)
Change-Id: I9c7df6abac4290b7a3e545e3d4aa1462108e239e
Reviewed-on: https://boringssl-review.googlesource.com/27705
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>
Rather than expose a (potentially) assembly function directly, wrap it
in a C function to make visibility control easier.
Change-Id: I4a2dfeb8999ff021b2e10fbc54850eeadabbefff
Reviewed-on: https://boringssl-review.googlesource.com/27724
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>
This introduces EC_FELEM, which is analogous to EC_SCALAR. It is used
for EC_POINT's representation in the generic EC_METHOD, as well as
random operations on tuned EC_METHODs that still are implemented
genericly.
Unlike EC_SCALAR, EC_FELEM's exact representation is awkwardly specific
to the EC_METHOD, analogous to how the old values were BIGNUMs but may
or may not have been in Montgomery form. This is kind of a nuisance, but
no more than before. (If p224-64.c were easily convertable to Montgomery
form, we could say |EC_FELEM| is always in Montgomery form. If we
exposed the internal add and double implementations in each of the
curves, we could give |EC_POINT| an |EC_METHOD|-specific representation
and |EC_FELEM| is purely a |EC_GFp_mont_method| type. I'll leave this
for later.)
The generic add and doubling formulas are aligned with the formulas
proved in fiat-crypto. Those only applied to a = -3, so I've proved a
generic one in https://github.com/mit-plv/fiat-crypto/pull/356, in case
someone uses a custom curve. The new formulas are verified,
constant-time, and swap a multiply for a square. As expressed in
fiat-crypto they do use more temporaries, but this seems to be fine with
stack-allocated EC_FELEMs. (We can try to help the compiler later,
but benchamrks below suggest this isn't necessary.)
Unlike BIGNUM, EC_FELEM can be stack-allocated. It also captures the
bounds in the type system and, in particular, that the width is correct,
which will make it easier to select a point in constant-time in the
future. (Indeed the old code did not always have the correct width. Its
point formula involved halving and implemented this in variable time and
variable width.)
Before:
Did 77274 ECDH P-256 operations in 10046087us (7692.0 ops/sec)
Did 5959 ECDH P-384 operations in 10031701us (594.0 ops/sec)
Did 10815 ECDSA P-384 signing operations in 10087892us (1072.1 ops/sec)
Did 8976 ECDSA P-384 verify operations in 10071038us (891.3 ops/sec)
Did 2600 ECDH P-521 operations in 10091688us (257.6 ops/sec)
Did 4590 ECDSA P-521 signing operations in 10055195us (456.5 ops/sec)
Did 3811 ECDSA P-521 verify operations in 10003574us (381.0 ops/sec)
After:
Did 77736 ECDH P-256 operations in 10029858us (7750.5 ops/sec) [+0.8%]
Did 7519 ECDH P-384 operations in 10068076us (746.8 ops/sec) [+25.7%]
Did 13335 ECDSA P-384 signing operations in 10029962us (1329.5 ops/sec) [+24.0%]
Did 11021 ECDSA P-384 verify operations in 10088600us (1092.4 ops/sec) [+22.6%]
Did 2912 ECDH P-521 operations in 10001325us (291.2 ops/sec) [+13.0%]
Did 5150 ECDSA P-521 signing operations in 10027462us (513.6 ops/sec) [+12.5%]
Did 4264 ECDSA P-521 verify operations in 10069694us (423.4 ops/sec) [+11.1%]
This more than pays for removing points_make_affine previously and even
speeds up ECDH P-256 slightly. (The point-on-curve check uses the
generic code.)
Next is to push the stack-allocating up to ec_wNAF_mul, followed by a
constant-time single-point multiplication.
Bug: 239
Change-Id: I44a2dff7c52522e491d0f8cffff64c4ab5cd353c
Reviewed-on: https://boringssl-review.googlesource.com/27668
Reviewed-by: Adam Langley <agl@google.com>
This does not appear to actually pull its weight. The purpose of this
logic is to switch some adds to the faster add_mixed in the wNAF code,
at the cost of a rather expensive inversion. This optimization kicks in
for generic curves, so P-384 and P-521:
With:
Did 32130 ECDSA P-384 signing operations in 30077563us (1068.2 ops/sec)
Did 27456 ECDSA P-384 verify operations in 30073086us (913.0 ops/sec)
Did 14122 ECDSA P-521 signing operations in 30077407us (469.5 ops/sec)
Did 11973 ECDSA P-521 verify operations in 30037330us (398.6 ops/sec)
Without:
Did 32445 ECDSA P-384 signing operations in 30069721us (1079.0 ops/sec)
Did 27056 ECDSA P-384 verify operations in 30032303us (900.9 ops/sec)
Did 13905 ECDSA P-521 signing operations in 30000430us (463.5 ops/sec)
Did 11433 ECDSA P-521 verify operations in 30021876us (380.8 ops/sec)
For single-point multiplication, the optimization is not useful. This
makes sense as we only have one table's worth of additions to convert
but still pay for the inversion. For double-point multiplication, it is
slightly useful for P-384 and very useful for P-521. However, the next
change to stack-allocate EC_FELEMs will more than compensate for
removing it. (The immediate goal here is to simplify the EC_FELEM
story.)
Additionally, that this optimization was not useful for single-point
multiplication implies that, should we wish to recover this, a modest
8-entry pre-computed (affine) base point table should have the same
effect or better.
Update-Note: I do not believe anything was calling either of these
functions. (If necessary, we can always add no-op stubs as whether a
point is affine is not visible to external code. It previously kicked in
some optimizations, but those were removed for constant-time needs
anyway.)
Bug: 239
Change-Id: Ic9c51b001c45595cfe592274c7d5d652f4234839
Reviewed-on: https://boringssl-review.googlesource.com/27667
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 753316232243ccbf86b96c1c51ffcb41651d9ad5.)
Just to sync up a bit further.
Change-Id: I805150d0f0c10d68648fae83603b0d46231ae4ec
Reviewed-on: https://boringssl-review.googlesource.com/27685
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>
If the caller asked for the base to be treated as secret, we should
provide that. Allowing unbounded inputs is not compatible with being
constant-time.
Additionally, this aligns with the guidance here:
https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#1-do-not-conditionally-choose-between-constant-and-non-constant-time
Update-Note: BN_mod_exp_mont_consttime and BN_mod_exp_mont now require
inputs be fully reduced. I believe current callers tolerate this.
Additionally, due to a quirk of how certain operations were ordered,
using (publicly) zero exponent tolerated a NULL BN_CTX while other
exponents required non-NULL BN_CTX. Non-NULL BN_CTX is now required
uniformly. This is unlikely to cause problems. Any call site where the
exponent is always zero should just be replaced with BN_value_one().
Change-Id: I7c941953ea05f36dc2754facb9f4cf83a6789c61
Reviewed-on: https://boringssl-review.googlesource.com/27665
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>
As the point may be the output of some private key operation, whether Z
accidentally hit one is secret.
Bug: 239
Change-Id: I7db34cd3b5dd5ca4b96980e8993a9b4eda49eb88
Reviewed-on: https://boringssl-review.googlesource.com/27664
Reviewed-by: Adam Langley <alangley@gmail.com>
We have little-endian BIGNUM functions now.
Change-Id: Iffc46a14e75c6bba2e170b824b1a08c69d2e9d18
Reviewed-on: https://boringssl-review.googlesource.com/27594
Reviewed-by: Adam Langley <alangley@gmail.com>
This is adapted from upstream's
eb7916960bf50f436593abe3d5f2e0592d291017.
This gives a 22% win for ECDSA signing. (Upstream cites 30-40%, but they
are unnecessarily using BN_mod_exp_mont_consttime in their generic path.
The exponent is public. I expect part of their 30-40% is just offsetting
this.)
Did 506000 ECDSA P-256 signing operations in 25044595us (20204.0 ops/sec)
Did 170506 ECDSA P-256 verify operations in 25033567us (6811.1 ops/sec)
Did 618000 ECDSA P-256 signing operations in 25031294us (24689.1 ops/sec)
Did 182240 ECDSA P-256 verify operations in 25006918us (7287.6 ops/sec)
Most of the performance win appears to be from the assembly operations
and not the addition chain. I have a CL to graft the addition chain onto
the C implementation, but it did not show measurable improvement in
ECDSA verify. ECDSA sign gets 2-4% faster, but we're more concerned
about ECDSA verify in the OPENSSL_SMALL builds.
Change-Id: Ide166f98b146c025f7f80ed7906336c16818540a
Reviewed-on: https://boringssl-review.googlesource.com/27593
Reviewed-by: Adam Langley <alangley@gmail.com>
This introduces a hook for the OpenSSL assembly.
Change-Id: I35e0588f0ed5bed375b12f738d16c9f46ceedeea
Reviewed-on: https://boringssl-review.googlesource.com/27592
Reviewed-by: Adam Langley <alangley@gmail.com>
Largely random data, but make it easy to add things in the future.
Change-Id: I30bee790bd9671b4d0327c2244fe5cd1a8954f90
Reviewed-on: https://boringssl-review.googlesource.com/27591
Reviewed-by: Adam Langley <alangley@gmail.com>
This imports the assembly portion of
eb7916960bf50f436593abe3d5f2e0592d291017 from upstream. Note the
OPENSSL_ia32cap_P bits were tweaked to be delocate-compatible. Those
should be reviewed against the original file.
Change-Id: I19eef722225bb7928275e3d93890f80aa2f8734d
Reviewed-on: https://boringssl-review.googlesource.com/27589
Reviewed-by: Adam Langley <alangley@gmail.com>
We were still using the allocating scalar inversion for ECDSA verify
because previously it seemed to be faster. It appears to have flipped
now, though probably was always just a wash.
While I'm here, save a multiplication by swapping the inversion and
Montgomery reduction.
Did 200000 ECDSA P-256 signing operations in 10025749us (19948.6 ops/sec)
Did 66234 ECDSA P-256 verify operations in 10061123us (6583.2 ops/sec)
Did 202000 ECDSA P-256 signing operations in 10020846us (20158.0 ops/sec)
Did 68052 ECDSA P-256 verify operations in 10020592us (6791.2 ops/sec)
The actual motivation is to get rid of the unchecked EC_SCALAR function
and align sign/verify in preparation for the assembly scalar ops.
Change-Id: I1bd3a5719a67966dc8edaa43535a3864b69f76d0
Reviewed-on: https://boringssl-review.googlesource.com/27588
Reviewed-by: Adam Langley <alangley@gmail.com>
No sense in adding impossible error cases we need to handle.
Additionally, tighten them a bit and require strong bounds. (I wasn't
sure what we'd need at first and made them unnecessarily general.)
Change-Id: I21a0afde90a55be2e9a0b8d7288f595252844f5f
Reviewed-on: https://boringssl-review.googlesource.com/27586
Reviewed-by: Adam Langley <alangley@gmail.com>
This is so the *_small functions can assume somewhat more uniform
widths, to simplify their error-handling.
Change-Id: I0420cb237084b253e918c64b0c170a5dfd99ab40
Reviewed-on: https://boringssl-review.googlesource.com/27584
Reviewed-by: Adam Langley <alangley@gmail.com>
The FIPS 186-4 algorithm we use includes a limit which hits a 2^-20
failure probability, assuming my math is right. We've observed roughly
2^-23. This is a little large at scale. (See b/77854769.)
To avoid modifying the FIPS algorithm, retry the whole thing four times
to bring the failure rate down to 2^-80. Along the way, now that I have
the derivation on hand, adjust
https://boringssl-review.googlesource.com/22584 to target the same
failure probability.
Along the way, fix an issue with RSA_generate_key where, if callers
don't check for failure, there may be half a key in there.
Change-Id: I0e1da98413ebd4ffa65fb74c67a58a0e0cd570ff
Reviewed-on: https://boringssl-review.googlesource.com/27288
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>
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>
The first non-zero window (which we can condition on for public
exponents) always multiplies by one. This means we can cut out one
Montgomery multiplication. It also means we never actually need to
initialize r to one, saving another Montgomery multiplication for P-521.
This, in turn, means we don't need the bn_one_to_montgomery optimization
for the public-exponent exponentations, so we can delete
bn_one_to_montgomery_small. (The function does currently promise to
handle p = 0, but this is not actually reachable, so it can just do a
reduction on RR.)
For RSA, where we're not doing many multiplications to begin with,
saving one is noticeable.
Before:
Did 92000 RSA 2048 verify (same key) operations in 3002557us (30640.6 ops/sec)
Did 25165 RSA 4096 verify (same key) operations in 3045046us (8264.2 ops/sec)
After:
Did 100000 RSA 2048 verify (same key) operations in 3002483us (33305.8 ops/sec)
Did 26603 RSA 4096 verify (same key) operations in 3010942us (8835.4 ops/sec)
(Not looking at the fresh key number yet as that still needs to be
fixed.)
Change-Id: I81a025a68d9b0f8eb0f9c6c04ec4eedf0995a345
Reviewed-on: https://boringssl-review.googlesource.com/27286
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>
It's defined to return one in Montgomery form, not a normal one.
(Not that this matters. This function is only used to Fermat's Little
Theorem. Probably it should have been less general, though we'd need to
make new test vectors first.)
Change-Id: Ia8d7588e6a413b25f01280af9aacef0192283771
Reviewed-on: https://boringssl-review.googlesource.com/27285
Reviewed-by: Adam Langley <agl@google.com>
BN_mod_exp_mont is intended to protect the base, but not the exponent.
Accordingly, it shouldn't treat a base of zero as special.
Change-Id: Ib053e8ce65ab1741973a9f9bfeff8c353567439c
Reviewed-on: https://boringssl-review.googlesource.com/27284
Reviewed-by: Adam Langley <agl@google.com>
Our technique to perform the reduction only works for balanced key
sizes. For unbalanced keys, we fall back to variable-time logic.
Instead, fall back earlier to the non-CRT codepath, which is still
secure, just slower. This also aligns with the advice here:
https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#1-do-not-conditionally-choose-between-constant-and-non-constant-time
Update-Note: This is a performance hit (some keys will run 3x slower),
but only for keys with different-sized primes. I believe the Windows
crypto APIs will not accept such keys at all. There are two scenarios to
be concerned with for RSA performance:
1. Performance of reasonably-generated keys. Keys that BoringSSL or
anyone else reasonable generates will all be balanced, so this change
does not affect them.
2. Worst-case performance for DoS purposes. This CL does not change the
worst-case performance for RSA at a given bit size. In fact, it improves
it slightly. A sufficiently unbalanced RSA key is as slow as not doing
CRT at all.
In both cases, this change does not affect performance. The affected
keys are pathologically-generated ones that were not quite pathological
enough.
Bug: 235
Change-Id: Ie298dabb549ab9108fa9374aa86ebffe8b6c6c88
Reviewed-on: https://boringssl-review.googlesource.com/27504
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>
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>
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>
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 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>
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>
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>
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>
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>
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>
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>
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>
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>
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/3161https://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>
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>
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>
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>
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>
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>
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>
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>
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>
