This was changed in draft-ietf-quic-tls-13 to use a codepoint from the
reserved range.
Change-Id: Ia3cda249a3f37bc244d5c8a7765ec34a5708c9ae
Reviewed-on: https://boringssl-review.googlesource.com/29464
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>
Update-Note: SSL_CTX_set_min_proto_version(SSL3_VERSION) now fails.
SSL_OP_NO_SSLv3 is now zero. Internal SSL3-specific "AEAD"s are gone.
Change-Id: I34edb160be40a5eea3e2e0fdea562c6e2adda229
Reviewed-on: https://boringssl-review.googlesource.com/29444
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 full library is a bit much, but this is enough to appease most of
cryptography.io.
Change-Id: I1bb0d83744c4550d5fe23c5c98cfd7e36b17fcc9
Reviewed-on: https://boringssl-review.googlesource.com/29365
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>
Right now we're inconsistent about it. If the OPTIONAL container is
missing, we report an error, but if the container is empty, we happily
return nothing. The latter behavior is more convenient for emulating
OpenSSL's PKCS#7 functions.
These are our own functions, so we have some leeway here. Looking
through callers, they appear to handle this fine.
Update-Note: This is a behavior change.
Change-Id: I1321025a64df3054d380003c90e57d9eb95e610f
Reviewed-on: https://boringssl-review.googlesource.com/29364
Reviewed-by: Adam Langley <agl@google.com>
CBS_asn1_ber_to_der was a little cumbersome to use. While it, in theory,
allowed callers to consistently advance past the element, no caller
actually did so consistently. Instead they would advance if conversion
happened, and not if it was already DER. For the PKCS7_* functions, this
was even caller-exposed.
Change-Id: I658d265df899bace9ba6616cb465f19c9e6c3534
Reviewed-on: https://boringssl-review.googlesource.com/29304
Reviewed-by: Adam Langley <agl@google.com>
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>
The DSA code is deprecated and will, hopefully, be removed in the future.
Nonetheless, this is easy enough to fix. It's the analog of the work we'd
already done for ECDSA.
- Document more clearly that we don't care about the DSA code.
- Use the existing constant-time modular addition function rather than
the ad-hoc code.
- Reduce the digest to satisfy modular operations' invariants. (The
underlying algorithms could accept looser bounds, but we reduce for
simplicity.) There's no particular reason to do this in constant time,
but we have the code for it, so we may as well.
- This additionally adds a missing check that num_bits(q) is a multiple
of 8. We otherwise don't compute the right answer. Verification
already rejected all 160-, 224-, and 256-bit keys, and we only
generate DSA parameters where the length of q matches some hash
function's length, so this is unlikely to cause anyone trouble.
- Use Montgomery reduction to perform the modular multiplication. This
could be optimized to save a couple Montgomery reductions as in ECDSA,
but DSA is deprecated, so I haven't bothered optimizing this.
- The reduction from g^k (mod p) to r = g^k (mod p) (mod q) is left
in variable time, but reversing it would require a discrete log
anyway. (The corresponding ECDSA operation is much easier to make
constant-time due to Hasse's theorem, though that's actually still a
TODO. I need to finish lifting EC_FELEM up the stack.)
Thanks to Keegan Ryan from NCC Group for reporting the modular addition issue
(CVE-2018-0495). The remainder is stuff I noticed along the way.
Update-Note: See the num_bits(q) change.
Change-Id: I4f032b041e2aeb09f9737a39f178c24e6a7fa1cb
Reviewed-on: https://boringssl-review.googlesource.com/29145
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 last libssl struct is now opaque! (Promote the SSL_MAX_* constants
as folks use them pretty frequently.)
Update-Note: SSL_SESSION is now opaque. I believe everything handles
this now.
Bug: 6
Change-Id: I8cd29d16173e4370f3341c0e6f0a56e00ea188e9
Reviewed-on: https://boringssl-review.googlesource.com/28964
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: This tweaks the SSL_shutdown behavior. OpenSSL's original
SSL_shutdown behavior was an incoherent mix of discarding the record and
rejecting it (it would return SSL_ERROR_SYSCALL but retrying the
operation would discard it). SSLeay appears to have intended to discard
it, so we previously "fixed" it actually discard.
However, this behavior is somewhat bizarre and means we skip over
unbounded data, which we typically try to avoid. If you are trying to
cleanly shutdown the TLS portion of your protocol, surely it is at a
point where additional data is a syntax error. I suspect I originally
did not realize that, because the discarded record did not properly
continue the loop, SSL_shutdown would appear as if it rejected the data,
and so it's unlikely anyone was relying on that behavior.
Discussion in https://github.com/openssl/openssl/pull/6340 suggests
(some of) upstream also prefers rejecting.
Change-Id: Icde419049306ed17eb06ce1a7e1ff587901166f3
Reviewed-on: https://boringssl-review.googlesource.com/28864
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>
This change adds server-side support for compressed certificates.
(Although some definitions for client-side support are included in the
headers, there's no code behind them yet.)
Change-Id: I0f98abf0b782b7337ddd014c58e19e6b8cc5a3c2
Reviewed-on: https://boringssl-review.googlesource.com/27964
Reviewed-by: David Benjamin <davidben@google.com>
This matches the OpenSSL 1.1.0 spelling. I'd thought we could hide
SSL_SESSION this pass, but I missed one test that messed with session
IDs!
Bug: 6
Change-Id: I84ea113353eb0eaa2b06b68dec71cb9061c047ca
Reviewed-on: https://boringssl-review.googlesource.com/28866
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@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>
It would be nice to restrict these, limiting the incorrect sizes to a
separate EVP_AEAD, but start by documenting this.
Bug: 34
Change-Id: I09845882f76a53a010355ceefd168d4fc10a0681
Reviewed-on: https://boringssl-review.googlesource.com/28745
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>
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>
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>
PyOpenSSL's tests expect all of the outputs to be distinct. OpenSSL also
tends to prefix the return values with strings like "compiler:", so do
something similar.
Change-Id: Ic411c95a276b477641ebad803ac309b3035c1b13
Reviewed-on: https://boringssl-review.googlesource.com/28544
Reviewed-by: Adam Langley <agl@google.com>
This is so Chromium can verify the session before offering it, rather
than doing it after the handshake (at which point it's too late to punt
the session) as we do today. This should, in turn, allow us to finally
verify certificates off a callback and order it correctly relative to
CertificateRequest in TLS 1.3.
(It will also order "correctly" in TLS 1.2, but this is useless. TLS 1.2
does not bind the CertificateRequest to the certificate at the point the
client needs to act on it.)
Bug: chromium:347402
Change-Id: I0daac2868c97b820aead6c3a7e4dc30d8ba44dc4
Reviewed-on: https://boringssl-review.googlesource.com/28405
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Previously, we'd omitted OpenSSL's OCSP APIs because they depend on a
complex OCSP mechanism and encourage the the unreliable server behavior
that hampers using OCSP stapling to fix revocation today. (OCSP
responses should not be fetched on-demand on a callback. They should be
managed like other server credentials and refreshed eagerly, so
temporary CA outage does not translate to loss of OCSP.)
But most of the APIs are byte-oriented anyway, so they're easy to
support. Intentionally omit the one that takes a bunch of OCSP_RESPIDs.
The callback is benign on the client (an artifact of OpenSSL reading
OCSP and verifying certificates in the wrong order). On the server, it
encourages unreliability, but pyOpenSSL/cryptography.io depends on this.
Dcument that this is only for compatibility with legacy software.
Also tweak a few things for compatilibility. cryptography.io expects
SSL_CTX_set_read_ahead to return something, SSL_get_server_tmp_key's
signature was wrong, and cryptography.io tries to redefine
SSL_get_server_tmp_key if SSL_CTRL_GET_SERVER_TMP_KEY is missing.
Change-Id: I2f99711783456bfb7324e9ad972510be8a95e845
Reviewed-on: https://boringssl-review.googlesource.com/28404
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These were added in OpenSSL 1.1.0.
Change-Id: I261e0e0ccf82544883c4a2ef5c5dc4a651c0c756
Reviewed-on: https://boringssl-review.googlesource.com/28329
Reviewed-by: Adam Langley <agl@google.com>
PyOpenSSL calls this function these days. Tested by roundtripping with
ourselves and also manually confirming our output interoperates with
OpenSSL. (For anyone repeating this experiment, the OpenSSL
command-line tool has a bug and does not correctly output friendlyName
attributes with non-ASCII characters. I'll send them a PR to fix this
shortly.)
Between this and the UTF-8 logic earlier, the theme of this patch series
seems to be "implement in C something I last implemented in
JavaScript"...
Change-Id: I258d563498d82998c6bffc6789efeaba36fe3a5e
Reviewed-on: https://boringssl-review.googlesource.com/28328
Reviewed-by: Adam Langley <agl@google.com>
This is not very useful without PKCS12_create, which a follow-up change
will implement.
Change-Id: I355ccd22a165830911ae189871ab90a6101f42ae
Reviewed-on: https://boringssl-review.googlesource.com/28327
Reviewed-by: Adam Langley <agl@google.com>
This aligns with OpenSSL 1.1.0's behavior, which deviated from OpenSSL
1.0.2. OpenSSL 1.0.2 effectively assumed input passwords were always
Latin-1.
Update-Note: If anyone was using PKCS#12 passwords with non-ASCII
characters, this changes them from being encoding-confused to hopefully
interpretting "correctly". If this breaks anything, we can add a
fallback to PKCS12_get_key_and_certs/PKCS12_parse, but OpenSSL 1.1.0
does not have such behavior. It only implements a fallback in the
command-line tool, not the APIs.
Change-Id: I0aa92db26077b07a40f85b89f4d3e0f6b0d7be87
Reviewed-on: https://boringssl-review.googlesource.com/28326
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: This changes causes BoringSSL to be stricter about handling
Unicode strings:
· Reject code points outside of Unicode
· Reject surrogate values
· Don't allow invalid UTF-8 to pass through when the source claims to
be UTF-8 already.
· Drop byte-order marks.
Previously, for example, a UniversalString could contain a large-valued
code point that would cause the UTF-8 encoder to emit invalid UTF-8.
Change-Id: I94d9db7796b70491b04494be84249907ff8fb46c
Reviewed-on: https://boringssl-review.googlesource.com/28325
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Callers should not mutate these.
Update-Note: I believe I've fixed up everything. If I missed one, the
fix should be straightforward.
Change-Id: Ifbce4961204822f57502a0de33aaa5a2a08b026d
Reviewed-on: https://boringssl-review.googlesource.com/28266
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Update-Note: Enabling TLS 1.3 now enables both draft-23 and draft-28
by default, in preparation for cycling all to draft-28.
Change-Id: I9405f39081f2e5f7049aaae8a9c85399f21df047
Reviewed-on: https://boringssl-review.googlesource.com/28304
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Hopefully this is the last of it before we can hide the struct. We're
missing peer_sha256 accessors, and some test wants to mutate the ticket
in a test client.
Change-Id: I1a30fcc0a1e866d42acbc07a776014c9257f7c86
Reviewed-on: https://boringssl-review.googlesource.com/28268
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>
Some third-party projects include it for some inexplicable reason.
Change-Id: I57c406d77d82a4a9ba6b54519023f2b02f2eb5e2
Reviewed-on: https://boringssl-review.googlesource.com/28225
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>
We forgot to do this in our original implementation on general ecosystem
grounds. It's also mandated starting draft-26.
Just to avoid unnecessary turbulence, since draft-23 is doomed to die
anyway, condition this on our draft-28 implementation. (We don't support
24 through 27.)
We'd actually checked this already on the Go side, but the spec wants a
different alert.
Change-Id: I0014cda03d7129df0b48de077e45f8ae9fd16976
Reviewed-on: https://boringssl-review.googlesource.com/28124
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This is done by adding two new tagged data types to the shim's
transcript: one for the serialized handoff, and another for the
serialized handback.
Then, the handshake driver in |TLSFuzzer| is modified to be able to
drive a handoff+handback sequence in the same way as was done for
testing: by swapping |BIO|s into additional |SSL| objects. (If a
particular transcript does not contain a serialized handoff, this is a
no-op.)
Change-Id: Iab23e4dc27959ffd3d444adc41d40a4274e83653
Reviewed-on: https://boringssl-review.googlesource.com/27204
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>
All CBC ciphers in TLS are broken and insecure. TLS 1.2 introduced
AEAD-based ciphers which avoid their many problems. It also introduced
new CBC ciphers based on HMAC-SHA256 and HMAC-SHA384 that share the same
flaws as the original HMAC-SHA1 ones. These serve no purpose. Old
clients don't support them, they have the highest overhead of all TLS
ciphers, and new clients can use AEADs anyway.
Remove them from libssl. This is the smaller, more easily reverted
portion of the removal. If it survives a week or so, we can unwind a lot
more code elsewhere in libcrypto. This removal will allow us to clear
some indirect calls from crypto/cipher_extra/tls_cbc.c, aligning with
the recommendations here:
https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#2-avoid-indirect-branches-in-constant-time-code
Update-Note: The following cipher suites are removed:
- TLS_RSA_WITH_AES_128_CBC_SHA256
- TLS_RSA_WITH_AES_256_CBC_SHA256
- TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
- TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
- TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
- TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
Change-Id: I7ade0fc1fa2464626560d156659893899aab6f77
Reviewed-on: https://boringssl-review.googlesource.com/27944
Reviewed-by: Adam Langley <agl@google.com>
Chrome needs to support renegotiation at TLS 1.2 + HTTP/1.1, but we're
free to shed the handshake configuration at TLS 1.3 or HTTP/2.
Rather than making config shedding implicitly disable renegotiation,
make the actual shedding dependent on a combination of the two settings.
If config shedding is enabled, but so is renegotiation (including
whether we are a client, etc.), leave the config around. If the
renegotiation setting gets disabled again after the handshake,
re-evaluate and shed the config then.
Bug: 123
Change-Id: Ie833f413b3f15b8f0ede617991e3fef239d4a323
Reviewed-on: https://boringssl-review.googlesource.com/27904
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Matt Braithwaite <mab@google.com>
|SSL_CONFIG| is a container for bits of configuration that are
unneeded after the handshake completes. By default it is retained for
the life of the |SSL|, but it may be shed at the caller's option by
calling SSL_set_shed_handshake_config(). This is incompatible with
renegotiation, and with SSL_clear().
|SSL_CONFIG| is reachable by |ssl->config| and by |hs->config|. The
latter is always non-NULL. To avoid null checks, I've changed the
signature of a number of functions from |SSL*| arguments to
|SSL_HANDSHAKE*| arguments.
When configuration has been shed, setters that touch |SSL_CONFIG|
return an error value if that is possible. Setters that return |void|
do nothing.
Getters that request |SSL_CONFIG| values will fail with an |assert| if
the configuration has been shed. When asserts are compiled out, they
will return an error value.
The aim of this commit is to simplify analysis of split-handshakes by
making it obvious that some bits of state have no effects beyond the
handshake. It also cuts down on memory usage.
Of note: |SSL_CTX| is still reachable after the configuration has been
shed, and a couple things need to be retained only for the sake of
post-handshake hooks. Perhaps these can be fixed in time.
Change-Id: Idf09642e0518945b81a1e9fcd7331cc9cf7cc2d6
Bug: 123
Reviewed-on: https://boringssl-review.googlesource.com/27644
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>
Chromium has some code which reaches into this field for memory
accounting.
This fixes a bug in doc.go where this line-wrapping confuses it. doc.go
needs a bit of a rewrite, but this is a bit better.
Change-Id: Ic9cc2c2fe9329d7bc366ccf91e0c9a92eae08ed2
Reviewed-on: https://boringssl-review.googlesource.com/27764
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>
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 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>
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>
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>
Alessandro Ghedini
David Benjamin
Adam Langley
Steven Valdez
Matthew Braithwaite