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>
Chrome uses the platform certificate verifier and thus cannot reliably
expect PSS signatures to work in all configurations. Add an API for the
consumer to inform BoringSSL of this ability. We will then adjust our
advertisements accordingly.
Note that, because TLS 1.2 does not have the signature_algorithms_cert
extension, turning off TLS 1.3 and using this API will stop advertising
RSA-PSS. I believe this is the correct behavior given the semantics of
that code point.
The tests check the various combinations here, as well as checking that
the peer never sends signature_algorithms_cert identical to
signature_algorithms.
Bug: 229
Change-Id: I8c33a93efdc9252097e3899425b49548fc42a93a
Reviewed-on: https://boringssl-review.googlesource.com/27488
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 reflects the change to add the key type into the constant. The old
constants are left around for now as legacy aliases and will be removed
later.
Change-Id: I67f1b50c01fbe0ebf4a2e9e89d3e7d5ed5f5a9d7
Reviewed-on: https://boringssl-review.googlesource.com/27486
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>
Update-Note: I believe everything relying on this overload has since
been updated.
Change-Id: I7facf59cde56098e5e3c79470293b67abb715f4c
Reviewed-on: https://boringssl-review.googlesource.com/27485
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>
Conscrypt need this function right now. They ought to be fixed up to not
need this but, in the meantime, this API is also provided by OpenSSL and
will clear one most consumer reaching into SSL_SESSION.
Bumping the API since Conscrypt often involves multi-sided stuff.
Change-Id: I665ca6b6a17ef479133c29c23fc639f278128c69
Reviewed-on: https://boringssl-review.googlesource.com/27405
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>
OpenSSL 1.1.0 renamed that. Also clang-format wanted to smush it all
onto one line.
Change-Id: Icdaa0eefc503c4aab1b309ccb34625f5e811c537
Reviewed-on: https://boringssl-review.googlesource.com/27404
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>
Change-Id: I7298c878bd2c8187dbd25903e397e8f0c2575aa4
Reviewed-on: https://boringssl-review.googlesource.com/26846
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This changes the contract for split handshakes such that on the
receiving side, the connection is to be driven until it returns
|SSL_ERROR_HANDBACK|, rather than until SSL_do_handshake() returns
success.
Change-Id: Idd1ebfbd943d88474d7c934f4c0ae757ff3c0f37
Reviewed-on: https://boringssl-review.googlesource.com/26864
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The relevant result (Damgård, Landrock, and Pomerance, Average Case
Error Estimates for the Strong Probably Prime Test) is only applicable
for randomly selected candidates. It relies on there being very few odd
composites with many false witnesses.
(If testing an adversarially-selected composite, false witnesses are
bounded by ϕ(n)/4 for n != 9, so one needs about 40 iterations for a
2^-80 false positive rate.)
Change-Id: I2a063dac5f9042dcb9e6affee8d2ae575f2238a9
Reviewed-on: https://boringssl-review.googlesource.com/26972
Reviewed-by: Adam Langley <agl@google.com>
AUTHORITY_INFO_ACCESS is a STACK_OF(ACCESS_DESCRIPTION), so we want to
add a deleter for ACCESS_DESCRIPTION, at which point
AUTHORITY_INFO_ACCESS's deleter will show up for free.
Change-Id: Id9efb74093868c39a893de67dd26f1fc15379252
Reviewed-on: https://boringssl-review.googlesource.com/26973
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
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>
The extra details in Enhanced Rabin-Miller are only used in
RSA_check_key_fips, on the public RSA modulus, which the static linker
will drop in most of our consumers anyway. Implement normal Rabin-Miller
for RSA keygen and use Montgomery reduction so it runs in constant-time.
Note that we only need to avoid leaking information about the input if
it's a large prime. If the number ends up composite, or we find it in
our table of small primes, we can return immediately.
The leaks not addressed by this CL are:
- The difficulty of selecting |b| leaks information about |w|.
- The distribution of whether step 4.4 runs leaks information about w.
- We leak |a| (the largest power of two which divides w) everywhere.
- BN_mod_word in the trial division is not constant-time.
These will be resolved in follow-up changes.
Median of 29 RSA keygens: 0m0.521 -> 0m0.621s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I0cf0ff22079732a0a3ababfe352bb4327e95b879
Reviewed-on: https://boringssl-review.googlesource.com/25886
Reviewed-by: Adam Langley <agl@google.com>
Constructed types with a recursive definition could eventually exceed
the stack given malicious input with excessive recursion. Therefore we
limit the stack depth.
CVE-2018-0739
Credit to OSSFuzz for finding this issue.
(Imported from upstream's 9310d45087ae546e27e61ddf8f6367f29848220d.)
BoringSSL does not contain any such structures, but import this anyway
with a test.
Change-Id: I0e84578ea795134f25dae2ac8b565f3c26ef3204
Reviewed-on: https://boringssl-review.googlesource.com/26844
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: Ia4fdd1eb848abacf43e18f6741ffa4ff79e40fd8
Reviewed-on: https://boringssl-review.googlesource.com/26664
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>
Folks should use curve25519 or P-256 if in doubt.
Change-Id: Ie35381ef739744788a80345286f7b21e2bb67c88
Reviewed-on: https://boringssl-review.googlesource.com/26646
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>
On the other hand, the type-specific
|CBS_get_optional_asn1_octet_string| must have a valid pointer and we
should check this in the “present” case or there could be a lucking
crash in some user waiting for an expected value to be missing.
Change-Id: Ida40e069ac7f0e50967e3f6c6b3fc01e49bd8894
Reviewed-on: https://boringssl-review.googlesource.com/26564
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 has now been folded into ServerHello. Additionally, TLS 1.2 and TLS
1.3 ServerHellos are now more uniform, so we can avoid the extra
ServerHello parser.
Change-Id: I46641128c3f65fe37e7effca5bef4a76bf3ba84c
Reviewed-on: https://boringssl-review.googlesource.com/26524
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 clearly was supposed to be a return 1. See
https://github.com/openssl/openssl/issues/5537 for details.
(Additionally, now that our BIGNUMs may be non-minimal, this function
violates the rule that BIGNUM functions should not depend on widths. We
should use w >= bn_minimal_width(a) to retain the original behavior. But
the original behavior is nuts, so let's just fix it.)
Update-Note: BN_mask_bits no longer reports failure in some cases. These
cases were platform-dependent and not useful, and code search confirms
nothing was relying on it.
Change-Id: I31b1c2de6c5de9432c17ec3c714a5626594ee03c
Reviewed-on: https://boringssl-review.googlesource.com/26464
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>
EC_KEY_copy left unset fields alone, which meant it was possible to
create an EC_KEY with mismatched private key and group. Nothing was
using EC_KEY_copy anyway, and in keeping of us generally preferring
fresh objects over object reuse, remove it. EC_KEY_dup itself can also
be made simpler by using the very setters available.
Additionally, skip copying the method table. As of
https://boringssl-review.googlesource.com/16344, we no longer copy the
ex_data, so we probably shouldn't copy the method pointers either,
aligning with RSAPrivateKey_dup.
Update-Note: If I missed anything and someone uses EC_KEY_copy, it
should be easy to port them to EC_KEY_dup.
Change-Id: Ibbdcea73345d91fa143fbe70a15bb527972693e8
Reviewed-on: https://boringssl-review.googlesource.com/26404
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>
Checking |initial_handshake_complete| was a mistake—it's not true for
False Start connections at the time when Chrome wants to measure whether
PQ padding was used or not.
Change-Id: I51757e00f3e02129666ee1ce31c30d63f1bcbe74
Reviewed-on: https://boringssl-review.googlesource.com/26444
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>
The probability of stumbling on a non-invertible b->A is negligible;
it's equivalent to accidentally factoring the RSA key. Relatedly,
document the slight caveat in BN_mod_inverse_blinded.
Change-Id: I308d17d12f5d6a12c444dda8c8fcc175ef2f5d45
Reviewed-on: https://boringssl-review.googlesource.com/26344
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 Bluetooth Mesh spec uses both apparently. Also extract a pile of
test vectors from that document (thanks to Kyle Lund for showing me
which to extract).
Change-Id: I04a04fafb7386ca28adfe1446fa388e841778931
Reviewed-on: https://boringssl-review.googlesource.com/26324
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>
On reflection, I think we'll need to note whether dummy PQ padding was
echoed on a given connection. Otherwise measurements in Chrome will be
mixed with cases where people have MITM proxies that ignored the
extension, or possibly Google frontends that haven't been updated.
Therefore this change will be used to filter latency measurements in
Chrome to only include those where the extension was echoed and we'll
measure at levels of 1 byte (for control), 400 bytes, and 1100 bytes.
This also makes it an error if the server didn't echo an extension of
the same length as was sent.
Change-Id: Ib2a0b29cfb8719a75a28f3cf96710c57d88eaa68
Reviewed-on: https://boringssl-review.googlesource.com/26284
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>
SSLv3_method, SSLv3_client_method, and SSLv3_server_method produce
SSL_CTXs which fail every handshake. They appear no longer necessary for
compatibility, so remove them.
SSLv3 is still accessible to callers who explicitly re-enable SSLv3 on a
TLS_method, but that will be removed completely later this year.
Meanwhile, clear out a weird hack we had here.
Update-Note: I believe there are no more callers of these functions. Any
that were were already non-functional as these methods haven't been
unable to handshake for a while now.
Change-Id: I622f785b428ab0ceab77b5a9db05b2b0df28145a
Reviewed-on: https://boringssl-review.googlesource.com/26004
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>
OpenSSL's RSA API is poorly designed and does not have a single place to
properly initialize the key. See
https://github.com/openssl/openssl/issues/5158.
To workaround this flaw, we must lazily instantiate pre-computed
Montgomery bits with locking. This is a ton of complexity. More
importantly, it makes it very difficult to implement RSA without side
channels. The correct in-memory representation of d, dmp1, and dmq1
depend on n, p, and q, respectively. (Those values have private
magnitudes and must be sized relative to the respective moduli.)
08805fe279 attempted to fix up the various
widths under lock, when we set up BN_MONT_CTX. However, this introduces
threading issues because other threads may access those exposed
components (RSA_get0_* also count as exposed for these purposes because
they are get0 functions), while a private key operation is in progress.
Instead, we do the following:
- There is no actual need to minimize n, p, and q, but we have minimized
copies in the BN_MONT_CTXs, so use those.
- Store additional copies of d, dmp1, and dmq1, at the cost of more
memory used. These copies have the correct width and are private,
unlike d, dmp1, and dmq1 which are sadly exposed. Fix private key
operations to use them.
- Move the frozen bit out of rsa->flags, as that too was historically
accessible without locking.
(Serialization still uses the original BIGNUMs, but the RSAPrivateKey
serialization format already inherently leaks the magnitude, so this
doesn't matter.)
Change-Id: Ia3a9b0629f8efef23abb30bfed110d247d1db42f
Reviewed-on: https://boringssl-review.googlesource.com/25824
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>
This allows a BIGNUM consumer to avoid messing around with bn->d and
bn->top/width.
Bug: 232
Change-Id: I134cf412fef24eb404ff66c84831b4591d921a17
Reviewed-on: https://boringssl-review.googlesource.com/25484
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>
This is a bit easier to read than BN_less_than_consttime when we must do
>= or <=, about as much work to compute, and lots of code calls BN_cmp
on secret data. This also, by extension, makes BN_cmp_word
constant-time.
BN_equal_consttime is probably a little more efficient and is perfectly
readable, so leave that one around.
Change-Id: Id2e07fe312f01cb6fd10a1306dcbf6397990cf13
Reviewed-on: https://boringssl-review.googlesource.com/25444
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>
Alas, the existence of RSA keys with q > p is obnoxious, but we can
canonicalize it away. To my knowledge, the remaining leaks in RSA are:
- Key generation. This is kind of hopelessly non-constant-time but
perhaps deserves a more careful ponder. Though hopefully it does not
come in at a measurable point for practical purposes.
- Private key serialization. RSAPrivateKey inherently leaks the
magnitudes of d, dmp1, dmq1, and iqmp. This is unavoidable but
hopefully does not come in at a measurable point for practical
purposes.
- If p and q have different word widths, we currently fall back to the
variable-time BN_mod rather than Montgomery reduction at the start of
CRT. I can think of ways to apply Montgomery reduction, but it's
probably better to deny CRT to such keys, if not reject them outright.
- bn_mul_fixed and bn_sqr_fixed which affect the Montgomery
multiplication bn_mul_mont-less configurations, as well as the final
CRT multiplication. We should fix this.
Bug: 233
Change-Id: I8c2ecf8f8ec104e9f26299b66ac8cbb0cad04616
Reviewed-on: https://boringssl-review.googlesource.com/25263
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>
d, dmp1, dmq1, and iqmp have private magnitudes. This is awkward because
the RSAPrivateKey serialization leaks the magnitudes. Do the best we can
and fix them up before any RSA operations.
This moves the piecemeal BN_MONT_CTX_set_locked into a common function
where we can do more complex canonicalization on the keys. Ideally this
would be done on key import, but the exposed struct (and OpenSSL 1.1.0's
bad API design) mean there is no single point in time when key import is
finished.
Also document the constraints on RSA_set0_* functions. (These
constraints aren't new. They just were never documented before.)
Update-Note: If someone tried to use an invalid RSA key where d >= n,
dmp1 >= p, dmq1 >= q, or iqmp >= p, this may break. Such keys would not
have passed RSA_check_key, but it's possible to manually assemble
keys that bypass it.
Bug: 232
Change-Id: I421f883128952f892ac0cde0d224873a625f37c5
Reviewed-on: https://boringssl-review.googlesource.com/25259
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>
This has no behavior change, but it has a semantic one. This CL is an
assertion that all BIGNUM functions tolerate non-minimal BIGNUMs now.
Specifically:
- Functions that do not touch top/width are assumed to not care.
- Functions that do touch top/width will be changed by this CL. These
should be checked in review that they tolerate non-minimal BIGNUMs.
Subsequent CLs will start adjusting the widths that BIGNUM functions
output, to fix timing leaks.
Bug: 232
Change-Id: I3a2b41b071f2174452f8d3801bce5c78947bb8f7
Reviewed-on: https://boringssl-review.googlesource.com/25257
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 empty states aren't any use to either caller or implementor.
Change-Id: If0b748afeeb79e4a1386182e61c5b5ecf838de62
Reviewed-on: https://boringssl-review.googlesource.com/25254
Reviewed-by: Adam Langley <agl@google.com>
Give a non-minimal modulus, there are two possible values of R we might
pick: 2^(BN_BITS2 * width) or 2^(BN_BITS2 * bn_minimal_width).
Potentially secret moduli would make the former attractive and things
might even work, but our only secret moduli (RSA) have public bit
widths. It's more cases to test and the usual BIGNUM invariant is that
widths do not affect numerical output.
Thus, settle on minimizing mont->N for now. With the top explicitly made
minimal, computing |lgBigR| is also a little simpler.
This CL also abstracts out the < R check in the RSA code, and implements
it in a width-agnostic way.
Bug: 232
Change-Id: I354643df30530db7866bb7820e34241d7614f3c2
Reviewed-on: https://boringssl-review.googlesource.com/25250
Reviewed-by: Adam Langley <agl@google.com>
This is actually a bit more complicated (the mismatching widths cases
will never actually happen in RSA), but it's easier to think about and
removes more width-sensitive logic.
Bug: 232
Change-Id: I85fe6e706be1f7d14ffaf587958e930f47f85b3c
Reviewed-on: https://boringssl-review.googlesource.com/25246
Reviewed-by: Adam Langley <agl@google.com>
The private key callback may not push one of its own (it's possible to
register a custom error library and whatnot, but this is tedious). If
the callback does not push any, we report SSL_ERROR_SYSCALL. This is not
completely wrong, as "syscall" really means "I don't know, something you
gave me, probably the BIO, failed so I assume you know what happened",
but most callers just check errno. And indeed cert_cb pushes its own
error, so this probably should as well.
Update-Note: Custom private key callbacks which push an error code on
failure will report both that error followed by
SSL_R_PRIVATE_KEY_OPERATION_FAILED. Callbacks which did not push any
error will switch from SSL_ERROR_SYSCALL to SSL_ERROR_SSL with
SSL_R_PRIVATE_KEY_OPERATION_FAILED.
Change-Id: I7e90cd327fe0cbcff395470381a3591364a82c74
Reviewed-on: https://boringssl-review.googlesource.com/25544
Reviewed-by: Adam Langley <agl@google.com>
Split handshakes allows the handshaking of a TLS connection to be
performed remotely. This encompasses not just the private-key and ticket
operations – support for that was already available – but also things
such as selecting the certificates and cipher suites.
The the comment block in ssl.h for details. This is highly experimental
and will change significantly before its settled.
Change-Id: I337bdfa4c3262169e9b79dd4e70b57f0d380fcad
Reviewed-on: https://boringssl-review.googlesource.com/25387
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Change-Id: I2486dc810ea842c534015fc04917712daa26cfde
Update-Note: Now that tls13_experiment2 is gone, the server should remove the set_tls13_variant call. To avoid further churn, we'll make the server default for future variants to be what we'd like to deploy.
Reviewed-on: https://boringssl-review.googlesource.com/25104
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>
This adds support for sending the quic_transport_parameters
(draft-ietf-quic-tls) in ClientHello and EncryptedExtensions, as well as
reading the value sent by the peer.
Bug: boringssl:224
Change-Id: Ied633f557cb13ac87454d634f2bd81ab156f5399
Reviewed-on: https://boringssl-review.googlesource.com/24464
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>
Having it in base.h pollutes the global namespace a bit and, in
particular, causes clang to give unhelpful suggestions in consuming
projects.
Change-Id: I6ca1a88bdd1701f0c49192a0df56ac0953c7067c
Reviewed-on: https://boringssl-review.googlesource.com/25464
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>
Since SSL{,_CTX}_set_custom_verify take a |mode| parameter that may be
|SSL_VERIFY_NONE|, it should do what it says on the tin, which is to
perform verification and ignore the result.
Change-Id: I0d8490111fb199c6b325cc167cf205316ecd4b49
Reviewed-on: https://boringssl-review.googlesource.com/25224
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>
Mono's legacy TLS 1.0 stack, as a server, does not implement any form of
resumption, but blindly echos the ClientHello session ID in the
ServerHello for no particularly good reason.
This is invalid, but due to quirks of how our client checked session ID
equality, we only noticed on the second connection, rather than the
first. Flaky failures do no one any good, so break deterministically on
the first connection, when we realize something strange is going on.
Bug: chromium:796910
Change-Id: I1f255e915fcdffeafb80be481f6c0acb3c628846
Reviewed-on: https://boringssl-review.googlesource.com/25424
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>
Change-Id: I7932258890b0b2226ff6841af45926e1b11979ba
Reviewed-on: https://boringssl-review.googlesource.com/24844
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 change adds |BORINGSSL_self_test|, which allows applications to run
the FIPS KAT tests on demand, even in non-FIPS builds.
Change-Id: I950b30a02ab030d5e05f2d86148beb4ee1b5929c
Reviewed-on: https://boringssl-review.googlesource.com/25044
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>
Update-Note: Token Binding can no longer be configured with the custom
extensions API. Instead, use the new built-in implementation. (The
internal repository should be all set.)
Bug: 183
Change-Id: I007523a638dc99582ebd1d177c38619fa7e1ac38
Reviewed-on: https://boringssl-review.googlesource.com/20645
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>
This extension will be used to measure the latency impact of potentially
sending a post-quantum key share by default. At this time it's purely
measuring the impact of the client sending the key share, not the server
replying with a ciphertext.
We could use the existing padding extension for this but that extension
doesn't allow the server to echo it, so we would need a different
extension in the future anyway. Thus we just create one now.
We can assume that modern clients will be using TLS 1.3 by the time that
PQ key-exchange is established and thus the key share will be sent in
all ClientHello messages. However, since TLS 1.3 isn't quite here yet,
this extension is also sent for TLS 1.0–1.2 ClientHellos. The latency
impact should be the same either way.
Change-Id: Ie4a17551f6589b28505797e8c54cddbe3338dfe5
Reviewed-on: https://boringssl-review.googlesource.com/24585
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>
Updating clang seems to have upset the clang-cl build. I think because
they decided -Wall now matches MSVC's semantics, which is a little nuts.
Two of the warnings, however, weren't wrong, so fix those.
http://llvm.org/viewvc/llvm-project?view=revision&revision=319116
Change-Id: I168e52e4e70ca7b1069e0b0db241fb5305c12b1e
Reviewed-on: https://boringssl-review.googlesource.com/24684
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>
The Chromium certificate verifier ends up encoding a SET OF when
canonicalizing X.509 names. Requiring the caller canonicalize a SET OF
is complicated enough that we should probably sort it for folks. (We
really need to get this name canonicalization insanity out of X.509...)
This would remove the extra level of indirection in Chromium
net/cert/internal/verify_name_match.cc CBB usage.
Note this is not quite the same order as SET, but SET is kind of
useless. Since it's encoding heterogeneous values, it is reasonable to
require the caller just encode them in the correct order. In fact, a DER
SET is just SEQUENCE with a post-processing step on the definition to
fix the ordering of the fields. (Unless the SET contains an untagged
CHOICE, in which case the ordering is weird, but SETs are not really
used in the real world, much less SETs with untagged CHOICEs.)
Bug: 11
Change-Id: I51e7938a81529243e7514360f867330359ae4f2c
Reviewed-on: https://boringssl-review.googlesource.com/24444
Reviewed-by: Adam Langley <agl@google.com>
This is a reland of https://boringssl-review.googlesource.com/2330. I
believe I've now cleared the fallout.
Android's attestion format uses some ludicrously large tag numbers:
https://developer.android.com/training/articles/security-key-attestation.html#certificate_schema
Add support for these in CBS/CBB. The public API does not change for
callers who were using the CBS_ASN1_* constants, but it is no longer the
case that tag representations match their DER encodings for small tag
numbers. When passing tags into CBS/CBB, use CBS_ASN1_* constants. When
working with DER byte arrays (most commonly test vectors), use the
numbers themselves.
Bug: 214
Update-Note: The in-memory representation of CBS/CBB tags changes.
Additionally, we now support tag numbers above 30. I believe I've now
actually cleared the fallout of the former. There is one test in
Chromium and the same test in the internal repository that needs
fixing.
Change-Id: I49b9d30df01f023c646d31156360ff69c91626a3
Reviewed-on: https://boringssl-review.googlesource.com/24404
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 is to simplify
https://boringssl-review.googlesource.com/c/boringssl/+/24445/.
Setting or changing an EC_KEY's group after the public or private keys
have been configured is quite awkward w.r.t. consistency checks. It
becomes additionally messy if we mean to store private keys as
EC_SCALARs (and avoid the BIGNUM timing leak), whose size is
curve-dependent.
Instead, require that callers configure the group before setting either
half of the keypair. Additionally, reject EC_KEY_set_group calls that
change the group. This will simplify clearing one more BIGNUM timing
leak.
Update-Note: This will break code which sets the group and key in a
weird order. I checked calls of EC_KEY_new and confirmed they all
set the group first. If I missed any, let me know.
Change-Id: Ie89f90a318b31b6b98f71138e5ff3de5323bc9a6
Reviewed-on: https://boringssl-review.googlesource.com/24425
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 function maps |X509_V_ERR_*| to SSL alarm codes. It's used
internally when certs are verified with X509_verify_cert(), and is
helpful to callers who want to call that function, but who also want
to report its errors in a less implementation-dependent way.
Change-Id: I2900cce2eb631489f0947c317beafafd3ea57a75
Reviewed-on: https://boringssl-review.googlesource.com/24564
Commit-Queue: Matt Braithwaite <mab@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>
TLS 1.3 includes a server-random-based anti-downgrade signal, as a
workaround for TLS 1.2's ServerKeyExchange signature failing to cover
the entire handshake. However, because TLS 1.3 draft versions are each
doomed to die, we cannot deploy it until the final RFC. (Suppose a
draft-TLS-1.3 client checked the signal and spoke to a final-TLS-1.3
server. The server would correctly negotiate TLS 1.2 and send the
signal. But the client would then break. An anologous situation exists
with reversed roles.)
However, it appears that Cisco devices have non-compliant TLS 1.2
implementations[1] and copy over another server's server-random when
acting as a TLS terminator (client and server back-to-back).
Hopefully they are the only ones doing this. Implement a
measurement-only version with a different value. This sentinel must not
be enforced, but it will tell us whether enforcing it will cause
problems.
[1] https://www.ietf.org/mail-archive/web/tls/current/msg25168.html
Bug: 226
Change-Id: I976880bdb2ef26f51592b2f6b3b97664342679c8
Reviewed-on: https://boringssl-review.googlesource.com/24284
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>
RSA_METHOD_FLAG_NO_CHECK is the same as our RSA_FLAG_OPAQUE. cURL uses
this to determine if it should call SSL_CTX_check_private_key.
Change-Id: Ie2953632346a31de346a4452f4eaad8435cf76e8
Reviewed-on: https://boringssl-review.googlesource.com/24245
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: Some RSA_FLAG_* constants are gone. Code search says they
were unused, but they can be easily restored if this breaks anything.
Change-Id: I47f642af5af9f8d80972ca8da0a0c2bd271c20eb
Reviewed-on: https://boringssl-review.googlesource.com/24244
Reviewed-by: Adam Langley <agl@google.com>
Upgrade-Note: SSL_CTX_set_tls13_variant(tls13_experiment) on the server
should switch to SSL_CTX_set_tls13_variant(tls13_experiment2).
(Configuring any TLS 1.3 variants on the server enables all variants,
so this is a no-op. We're just retiring some old experiments.)
Change-Id: I60f0ca3f96ff84bdf59e1a282a46e51d99047462
Reviewed-on: https://boringssl-review.googlesource.com/23784
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>
QUIC will need to derive keys at this point. This also smooths over a
part of the server 0-RTT abstraction. Like with False Start, the SSL
object is largely in a functional state at this point.
Bug: 221
Change-Id: I4207d8cb1273a1156e728a7bff3943cc2c69e288
Reviewed-on: https://boringssl-review.googlesource.com/24224
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>
Originally, the only OpenSSL API to stringify errors was:
char *ERR_error_string(unsigned long e, char *buf);
This API leaves callers a choice to either be thread unsafe (buf = NULL)
or pass in a buffer with unknown size. Indeed the original
implementation was just a bunch of unchecked sprintfs with, in the buf =
NULL case, a static 256-byte buffer.
388f2f56f2/crypto/err/err.c (L374)
Then ERR_error_string was documented that the buffer must be size 120.
Nowhere in the code was 120 significant. I expect OpenSSL just made up a
number.
388f2f56f2
Then upstream added the ERR_error_string_n API. Although the
documentation stated 120 bytes, the internal buffer was 256, so the code
actually translates ERR_error_string to ERR_error_string_n(e, buf, 256),
not ERR_error_string_n(e, buf, 120)!
e5c84d5152
So the documentation was wrong all this time! OpenSSL 1.1.0 corrected
the documentation to 256, but, alas, a lot of code used the
documentation and sized the buffer at 120. We should fix all
ERR_error_string callers to ERR_error_string_n but, in the meantime,
using 120 is probably less effort.
Note this also affects ERR_print_errors_cb right now. We don't have
function codes, so 120 bytes leaves 60 bytes for the reason code. Our
longest one, TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST is 46 bytes,
so it's a little tight, but, if needed, we can recover 20-ish bytes by
shrinking the library names. We can also always make ERR_print_errors_cb
use a larger buffer.
Change-Id: I472a1a802f2e6281cc7515d2a452208d6bac1200
Reviewed-on: https://boringssl-review.googlesource.com/24184
Reviewed-by: Adam Langley <agl@google.com>
The newer clang-cl is unhappy about the tautological comparison on
Windows, but the comparison itself is unnecessary anyway, since the
values will never exceed uint32_t.
I think the reason it's not firing elsewhere is because on other 64-bit
platforms, it is not tautological because long is 64-bit. On other
32-bit platforms, I'm not sure we actually have a standalone trunk clang
builder right now.
Update-Note: UTF8_getc and UTF8_putc were unexported. No one appears to
be calling them. (We're a crypto library, not a Unicode library.)
Change-Id: I0949ddea3131dca5f55d04e672c3ccf2915c41ab
Reviewed-on: https://boringssl-review.googlesource.com/23844
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 can probably do this globally at this point since the cipher
requirements are much more restrict than they were in the beginning.
(Firefox, in particular, has done so far a while.) For now add a flag
since some consumer wanted this.
I'll see about connecting it to a Chrome field trial after our breakage
budget is no longer reserved for TLS 1.3.
Change-Id: Ib61dd5aae2dfd48b56e79873a7f3061a7631a5f8
Reviewed-on: https://boringssl-review.googlesource.com/23725
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>
This reverts commit 66801feb17. This
turned out to break a lot more than expected. Hopefully we can reland it
soon, but we need to fix up some consumers first.
Note due to work that went in later, this is not a trivial revert and
should be re-reviewed.
Change-Id: I6474b67cce9a8aa03f722f37ad45914b76466bea
Reviewed-on: https://boringssl-review.googlesource.com/23644
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 need it in both directions. Also I missed that in OBJ_obj2txt we
allowed uint64_t components, but in my new OBJ_txt2obj we only allowed
uint32_t. For consistency, upgrade that to uint64_t.
Bug: chromium:706445
Change-Id: I38cfeea8ff64b9acf7998e552727c6c3b2cc600f
Reviewed-on: https://boringssl-review.googlesource.com/23544
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>
OBJ_txt2obj is currently implemented using BIGNUMs which is absurd. It
also depends on the giant OID table, which is undesirable. Write a new
one and expose the low-level function so Chromium can use it without the
OID table.
Bug: chromium:706445
Change-Id: I61ff750a914194f8776cb8d81ba5d3eb5eaa3c3d
Reviewed-on: https://boringssl-review.googlesource.com/23364
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>
DECLARE_STACK_OF adds a trailing ; so we don't need a second one added
here.
Compiling a project using boringssl which uses -Werror,-Wextra-semi I
get errors:
```
third_party/boringssl/include/openssl/stack.h:374:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_STACK_OF(void)
^
third_party/boringssl/include/openssl/stack.h:355:3: note: expanded from macro 'DEFINE_STACK_OF'
BORINGSSL_DEFINE_STACK_OF_IMPL(type, type *, const type *) \
^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
DECLARE_STACK_OF(name); \
^
third_party/boringssl/include/openssl/stack.h:375:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_SPECIAL_STACK_OF(OPENSSL_STRING)
^
third_party/boringssl/include/openssl/stack.h:369:3: note: expanded from macro 'DEFINE_SPECIAL_STACK_OF'
BORINGSSL_DEFINE_STACK_OF_IMPL(type, type, const type)
^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
DECLARE_STACK_OF(name); \
^
2 errors generated.
```
Change-Id: Icc39e2341eb76544be72d2d7d0bd29e2f1ed0bf9
Reviewed-on: https://boringssl-review.googlesource.com/23404
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>
Matches the OpenSSL 1.1.0 spelling, which is what we advertise in
OPENSSL_VERSION_NUMBER now. Otherwise third-party code which uses it
will, in the long term, need ifdefs. Note this will require updates to
any existing callers (there appear to only be a couple of them), but it
should be straightforward.
Change-Id: I9dd1013609abca547152728a293529055dacc239
Reviewed-on: https://boringssl-review.googlesource.com/23325
Reviewed-by: Adam Langley <agl@google.com>
None of the asymmetric crypto we inherented from OpenSSL is
constant-time because of BIGNUM. BIGNUM chops leading zeros off the
front of everything, so we end up leaking information about the first
word, in theory. BIGNUM functions additionally tend to take the full
range of inputs and then call into BN_nnmod at various points.
All our secret values should be acted on in constant-time, but k in
ECDSA is a particularly sensitive value. So, ecdsa_sign_setup, in an
attempt to mitigate the BIGNUM leaks, would add a couple copies of the
order.
This does not work at all. k is used to compute two values: k^-1 and kG.
The first operation when computing k^-1 is to call BN_nnmod if k is out
of range. The entry point to our tuned constant-time curve
implementations is to call BN_nnmod if the scalar has too many bits,
which this causes. The result is both corrections are immediately undone
but cause us to do more variable-time work in the meantime.
Replace all these computations around k with the word-based functions
added in the various preceding CLs. In doing so, replace the BN_mod_mul
calls (which internally call BN_nnmod) with Montgomery reduction. We can
avoid taking k^-1 out of Montgomery form, which combines nicely with
Brian Smith's trick in 3426d10119. Along
the way, we avoid some unnecessary mallocs.
BIGNUM still affects the private key itself, as well as the EC_POINTs.
But this should hopefully be much better now. Also it's 10% faster:
Before:
Did 15000 ECDSA P-224 signing operations in 1069117us (14030.3 ops/sec)
Did 18000 ECDSA P-256 signing operations in 1053908us (17079.3 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1087853us (990.9 ops/sec)
Did 473 ECDSA P-521 signing operations in 1069835us (442.1 ops/sec)
After:
Did 16000 ECDSA P-224 signing operations in 1064799us (15026.3 ops/sec)
Did 19000 ECDSA P-256 signing operations in 1007839us (18852.2 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1079413us (998.7 ops/sec)
Did 484 ECDSA P-521 signing operations in 1083616us (446.7 ops/sec)
Change-Id: I2a25e90fc99dac13c0616d0ea45e125a4bd8cca1
Reviewed-on: https://boringssl-review.googlesource.com/23075
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>
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: 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>
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>
We end up writing these switch cases everywhere. Let consumers decompose
these a bit. The original thought was folks should write switch-cases so
they handle everything they support, but that's a pain. As long as
algorithm preferences are always configured, we can still add new
dimensions because folks won't be asked to sign algorithms that depend
on dimensions they don't understand.
Change-Id: I3dd7f067f2c55212f0201876546bc70fee032bcf
Reviewed-on: https://boringssl-review.googlesource.com/22524
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>
Change-Id: I46686aea9b68105cfe70a11db0e88052781e179c
Reviewed-on: https://boringssl-review.googlesource.com/22164
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>
Reviewed-by: Steven Valdez <svaldez@google.com>
Currently we only check that the underlying EC_METHODs match, which
avoids the points being in different forms, but not that the points are
on the same curves. (We fixed the APIs early on so off-curve EC_POINTs
cannot be created.)
In particular, this comes up with folks implementating Java's crypto
APIs with ECDH_compute_key. These APIs are both unfortunate and should
not be mimicked, as they allow folks to mismatch the groups on the two
multiple EC_POINTs. Instead, ECDH APIs should take the public value as a
byte string.
Thanks also to Java's poor crypto APIs, we must support custom curves,
which makes this particularly gnarly. This CL makes EC_GROUP_cmp work
with custom curves and adds an additional subtle requirement to
EC_GROUP_set_generator.
Annoyingly, this change is additionally subtle because we now have a
reference cycle to hack around.
Change-Id: I2efbc4bd5cb65fee5f66527bd6ccad6b9d5120b9
Reviewed-on: https://boringssl-review.googlesource.com/22245
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>
There is also no need to make the struct public. Also tidy up includes a
bit.
Change-Id: I188848dfd8f9ed42925b2c55da8dc4751c29f146
Reviewed-on: https://boringssl-review.googlesource.com/22126
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>
I've left EVP_set_buggy_rsa_parser as a no-op stub for now, but it
shouldn't need to last very long. (Just waiting for a CL to land in a
consumer.)
Bug: chromium:735616
Change-Id: I6426588f84dd0803661a79c6636a0414f4e98855
Reviewed-on: https://boringssl-review.googlesource.com/22124
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>
This removes the last place where non-app-data hooks leave anything
uncomsumed in rrec. (There is still a place where non-app-data hooks see
a non-empty rrec an entrance. read_app_data calls into read_handshake.
That'll be fixed in a later patch in this series.)
This should not change behavior, though some error codes may change due
to some processing happening in a slightly different order.
Since we do this in a few places, this adds a BUF_MEM_append with tests.
Change-Id: I9fe1fc0103e47f90e3c9f4acfe638927aecdeff6
Reviewed-on: https://boringssl-review.googlesource.com/21345
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>
Change-Id: I63b9972034fdc85bf2d23e7d46516755855fafbe
Reviewed-on: https://boringssl-review.googlesource.com/22024
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>
We still have more links to cut for ssl.h to not pull in x509.h (notably
pem.h), but this resolves some easy ones. I've kept the constants the
same just in case, but nowhere are the constants mixed up by callers or
passed from one to the other in the functions' implementations. They're
completely independent.
Change-Id: Ic0896283378b5846afd6422bfe740951ac552f0e
Reviewed-on: https://boringssl-review.googlesource.com/21704
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>
It's no longer needed in the public header at all, now that we've hidden
the SSL_CTX struct.
Change-Id: I2fc6ddbeb52f000487627b433b9cdd7a4cde37a8
Reviewed-on: https://boringssl-review.googlesource.com/21684
Reviewed-by: Steven Valdez <svaldez@google.com>
Change-Id: Ifb227675cbc8e60128140768fb7d7f5f94928ac2
Reviewed-on: https://boringssl-review.googlesource.com/21764
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>
Commit 9a4876e193 broke NGINX builds with
BoringSSL due to this missing include (OpenSSL builds work fine):
src/event/ngx_event_openssl.c: In function ‘ngx_ssl_session_ticket_key_callback’:
src/event/ngx_event_openssl.c:3065:13: error: implicit declaration of function ‘HMAC_Init_ex’; did you mean ‘SHA1_Init’? [-Werror=implicit-function-declaration]
if (HMAC_Init_ex(hctx, key[0].hmac_key, size, digest, NULL) != 1) {
^~~~~~~~~~~~
Change-Id: Ie7170f05034d5fd8c85d1948b4ab9c9bb8447d13
Reviewed-on: https://boringssl-review.googlesource.com/21664
Reviewed-by: Adam Langley <agl@google.com>
Thanks to Alex Gaynor for reporting this.
Change-Id: I983ecb33cf017160f82582cc79e71f8ae7b30b99
Reviewed-on: https://boringssl-review.googlesource.com/21744
Reviewed-by: David Benjamin <davidben@google.com>
This frees us up to make SSL_CTX a C++ type and avoids a lot of
protrusions of otherwise private types into the global namespace.
Bug: 6
Change-Id: I8a0624a53a4d26ac4a483fa270c39ecdd07459ee
Reviewed-on: https://boringssl-review.googlesource.com/21584
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>
