When not offering to resume a session, the client populates s->session with a
fresh SSL_SESSION before the ServerHello is processed and, in DTLS_ANY_VERSION,
before the version is even determined. Don't create a fresh SSL_SESSION until
we know we are doing a full handshake.
This brings ssl3_send_client_hello closer to ssl23_client_hello in behavior. It
also fixes ssl_version in the client in DTLS_ANY_VERSION.
SSLv23_client_method is largely unchanged. If no session is offered, s->session
continues to be NULL until the ServerHello is received. The one difference is
that s->session isn't populated until the entire ServerHello is received,
rather than just the first half, in the case of a fragmented ServerHello. Apart
from info_callback, no external hooks get called between those points, so this
shouldn't expose new missing NULL checks.
The other client methods change significantly to match SSLv23_client_method's
behavior. For TLS, any exposed missing NULL checks are also in
SSLv23_client_method (and version-specific methods are already weird), so that
should be safe. For DTLS, I've verified that accesses in d1_*.c either handle
NULL or are after the ServerHello.
Change-Id: Idcae6bd242480e28a57dbba76ce67f1ac1ae1d1d
Reviewed-on: https://boringssl-review.googlesource.com/2404
Reviewed-by: Adam Langley <agl@google.com>
This fixes bugs that kept the tests from working:
- Resolve DTLS version and cookie before the session.
- In DTLS_ANY_VERSION, ServerHello should be read with first_packet = 1. This
is a regression from f2fedefdca. We'll want to
do the same for TLS, but first let's change this to a boolean has_version in a
follow-up.
Things not yet fixed:
- DTLS code is not EVP_AEAD-aware. Those ciphers are disabled for now.
- On the client, DTLS_ANY_VERSION creates SSL_SESSIONs with the wrong
ssl_version. The tests pass because we no longer enforce the match as of
e37216f56009fbf48c3a1e733b7a546ca6dfc2af. (In fact, we've gone from the server
ignoring ssl_version and client enforcing to the client mostly ignoring
ssl_version and the server enforcing.)
- ssl3_send_client_hello's ssl_version check checks for equality against
s->version rather than >.
Change-Id: I5a0dde221b2009413df9b9443882b9bf3b29519c
Reviewed-on: https://boringssl-review.googlesource.com/2403
Reviewed-by: Adam Langley <agl@google.com>
This is a bit of cleanup that probably should have been done at the same time
as 30ddb434bf.
For now, version negotiation is implemented with a method swap. It also
performs this swap on SSL_set_session, but this was neutered in
30ddb434bf. Rather than hackishly neuter it,
remove it outright. In addition, remove SSL_set_ssl_method. Now all method
swaps are internal: SSLv23_method switch to a version-specific method and
SSL_clear undoing it.
Note that this does change behavior: if an SSL* is created with one
version-specific method and we SSL_set_session to a session from a /different/
version, we would switch to the /other/ version-specific method. This is
extremely confusing, so it's unlikely anyone was actually expecting it.
Version-specific methods in general don't work well.
Change-Id: I72a5c1f321ca9aeb1b52ebe0317072950ba25092
Reviewed-on: https://boringssl-review.googlesource.com/2390
Reviewed-by: Adam Langley <agl@google.com>
We forgot to add those when we implemented the features. (Also relevant because
they will provide test coverage later for configuring features when using the
generic method tables rather than *_client_method.)
Change-Id: Ie08b27de893095e01a05a7084775676616459807
Reviewed-on: https://boringssl-review.googlesource.com/2410
Reviewed-by: Adam Langley <agl@google.com>
Although the comment suggests this was added with an s->session check to
account for SSL_set_session switching methods (which we will remove in the next
commit) and to account for SSLv23_method switching methods (which we hope to
remove after a long tower of cleanup), the current codepath never runs and
can't work:
If it is called prior to handshaking or setting a session, no method switch has
happened so that codepath is dead. If it is called after setting a session, the
s->session check will keep it from running. If it is called after a handshake,
we will have established a session so that check will again keep it from
running. (Finally, if it is called during the handshake, the in_handshake check
will stop; that there is an SSL_clear call in the handshake state machine at
all is a bug that will be addressed once more things are disentangled. See
upstream's 979689aa5cfa100ccbc1f25064e9398be4b7b05c.)
Were that code to ever run, the SSL* would be in an inconsistent state. It
switches the method, but not the handshake_func. The handshake_func isn't
switched to NULL, so that will keep the SSL_connect and SSL_accept code from fixing it.
It seems the intent was that the caller would always call
SSL_set_{connect,accept}_state to fix this. But as of upstream's
b31b04d951e9b65bde29657e1ae057b76f0f0a73, this is not necessary and indeed
isn't called by a lot of consumer code.
Change-Id: I710652b1d565b77bc26f913c2066ce749a9025c9
Reviewed-on: https://boringssl-review.googlesource.com/2430
Reviewed-by: Adam Langley <agl@google.com>
The data is owned by the SSL_SESSION, so the caller should not modify it. This
will require changes in Chromium, but they should be trivial.
Change-Id: I314718530c7d810f7c7b8852339b782b4c2dace1
Reviewed-on: https://boringssl-review.googlesource.com/2409
Reviewed-by: Adam Langley <agl@google.com>
This is only used for EAP-FAST which we apparently don't need to support.
Remove it outright. We broke it in 9eaeef81fa by
failing to account for session misses.
If this changes and we need it later, we can resurrect it. Preferably
implemented differently: the current implementation is bolted badly onto the
handshake. Ideally use the supplied callbacks to fabricate an appropriate
SSL_SESSION and resume that with as much of the normal session ticket flow as
possible.
The one difference is that EAP-FAST seems to require the probing mechanism for
session tickets rather than the sane session ID echoing version. We can
reimplement that by asking the record layer to probe ahead for one byte.
Change-Id: I38304953cc36b2020611556a91e8ac091691edac
Reviewed-on: https://boringssl-review.googlesource.com/2360
Reviewed-by: Adam Langley <agl@google.com>
This implements session IDs in client and server in runner.go.
Change-Id: I26655f996b7b44c7eb56340ef6a415d3f2ac3503
Reviewed-on: https://boringssl-review.googlesource.com/2350
Reviewed-by: Adam Langley <agl@google.com>
The ex_data index may fail to be allocated. Also don't leave a dangling pointer
in handshake_dgst if EVP_DigestInit_ex fails and check a few more init function
failures.
Change-Id: I2e99a89b2171c9d73ccc925a2f35651af34ac5fb
Reviewed-on: https://boringssl-review.googlesource.com/2342
Reviewed-by: Adam Langley <agl@google.com>
This commit fixes a number of crashes caused by malloc failures. They
were found using the -malloc-test=0 option to runner.go which runs tests
many times, causing a different allocation call to fail in each case.
(This test only works on Linux and only looks for crashes caused by
allocation failures, not memory leaks or other errors.)
This is not the complete set of crashes! More can be found by collecting
core dumps from running with -malloc-test=0.
Change-Id: Ia61d19f51e373bccb7bc604642c51e043a74bd83
Reviewed-on: https://boringssl-review.googlesource.com/2320
Reviewed-by: Adam Langley <agl@google.com>
tls1_process_sigalgs now only determines the intersection between the peer
algorithms and those configured locally. That list is queried later to
determine the hash algorithm to use when signing CertificateVerify or
ServerKeyExchange.
This is needed to support client auth on Windows where smartcards or CAPI may
not support all hash functions.
As a bonus, this does away with more connection-global state. This avoids the
current situation where digests are chosen before keys are known (for
CertificateVerify) or for slots that don't exist.
Change-Id: Iec3619a103d691291d8ebe08ef77d574f2faf0e8
Reviewed-on: https://boringssl-review.googlesource.com/2280
Reviewed-by: Adam Langley <agl@google.com>
CERT_PKEY_SIGN isn't meaningful since, without strict mode, we always fall back
to SHA-1 anyway. So the digest is never NULL when CERT_PKEY_SIGN is computed.
The entire valid_flags is now back to it's pre-1.0.2 check of seeing if the
certificate and key are configured.
This finally removes the sensitivity between valid_flags and selecting the
digest, so we can defer choosing the digest all we like.
Change-Id: I9f9952498f512d7f0cc799497f7c5b52145a48af
Reviewed-on: https://boringssl-review.googlesource.com/2288
Reviewed-by: Adam Langley <agl@google.com>
It doesn't depend on the cipher now that export ciphers are gone. It need only
be called once. Also remove the valid bit; nothing ever reads it. Its output is
also only used within a function, so make mask_k and mask_a local variables.
So all the configuration-based checks are in one place, change the input
parameter from CERT to SSL and move the PSK and ECDHE checks to the mask
computation. This avoids having to evaluate the temporary EC key for each
cipher.
The remaining uses are on the client which uses them differently (disabled
features rather than enabled ones). Those too may as well be local variables,
so leave a TODO.
Change-Id: Ibcb574341795d4016ea749f0290a793eed798874
Reviewed-on: https://boringssl-review.googlesource.com/2287
Reviewed-by: Adam Langley <agl@google.com>
Many are now unused. Only two are currently considered in cipher selection:
CERT_PKEY_VALID and CERT_PKEY_SIGN. (As per previous commits, this is either
bizarre due to limited slots or redundant with ssl_early_callback_ctx. We can
probably prune this too.)
This also fixes a bug where DTLS 1.0 went through a TLS 1.2 codepath. As the
DTLS code is currently arranged, all version comparisons must be done via
macros like SSL_USE_SIGALGS. (Probably we should add functions to map from DTLS
to TLS versions and slowly move the library to using the TLS version as
in-memory representation.)
Change-Id: I89bcf5b7b9ea5cdecf54f4445156586377328fe0
Reviewed-on: https://boringssl-review.googlesource.com/2286
Reviewed-by: Adam Langley <agl@google.com>
It's new in OpenSSL 1.0.2 so it's never set by existing code. This removes gobs
and gobs of complexity from tls1_check_chain. It only checks the local
certificate, not the peer certificate. The uses appear to be:
- Sanity-check configuration. Not worth the complexity.
- Guide in selecting ciphers based on ClientHello parameters and which
certificates in the CERT_PKEY are compatible. This isn't very useful one its
own since the CERT_PKEY array only stores one slot per type (e.g. you cannot
configure RSA/SHA-1 and RSA/SHA-256).
- For the (currently removed) SSL_check_chain to return more information based
on ClientHello parameters and guide selecting a certificate. This is
potentially useful but, as noted in the commit which removed it, redundant
with ssl_early_callback_ctx.
This CL is largely mechanical removing of dead codepaths. The follow-up will
clean up the now unnecessary parts of this function.
Change-Id: I2ebfa17e4f73e59aa1ee9e4ae7f615af2c6cf590
Reviewed-on: https://boringssl-review.googlesource.com/2285
Reviewed-by: Adam Langley <agl@google.com>
Get rid of now dead codepaths.
Change-Id: I3b5d49097cba70c5698a230cc6c1d79bdd0f0880
Reviewed-on: https://boringssl-review.googlesource.com/2284
Reviewed-by: Adam Langley <agl@google.com>
Both of these are newly-exported in OpenSSL 1.0.2, so they cannot be used by
current consumers.
This was added in upstream's 18d7158809c9722f4c6d2a8af7513577274f9b56 to
support custom selection of certificates. The intent seems to be that you
listen to cert_cb and use SSL_check_chain to lean on OpenSSL to process
signature algorithms list for you.
Unfortunately, the implementation is slightly suspect: it uses the same
function as the codepath which mutates and refers to the CERT_PKEY of the
matching type. Some access was guarded by check_flags, but this is too
complex. Part of it is also because the matching digest is selected early and
we intend to connect this to EVP_PKEY_supports_digest so it is no longer a
property of just the key type.
Let's remove the hook for now, to unblock removing a lot of complexity. After
cleaning up this area, a function like this could be cleaner to support, but
we already have a version of this: select_certificate_cb and
ssl_early_callback_ctx.
Change-Id: I3add425b3996e5e32d4a88e14cc607b4fdaa5aec
Reviewed-on: https://boringssl-review.googlesource.com/2283
Reviewed-by: Adam Langley <agl@google.com>
This is maintained just to distinguish whether the digest was negotiated or we
simply fell back to assuming SHA-1 support. No code is sensitive to this flag
and it adds complexity because it is set at a different time, for now, from the
rest of valid_flags.
The flag is new in OpenSSL 1.0.2, so nothing external could be sensitive to it.
Change-Id: I9304e358d56f44d912d78beabf14316d456bf389
Reviewed-on: https://boringssl-review.googlesource.com/2282
Reviewed-by: Adam Langley <agl@google.com>
This is new in OpenSSL 1.0.2 so it isn't used anywhere. Cuts down slightly on
connection-global state associated with signature algorithm processing.
Repurposing the digest field to mean both "the digest we choose to sign with
this key" and "the digest the last signature we saw happened to use" is
confusing.
Change-Id: Iec4d5078c33e271c8c7b0ab221c356ee8480b89d
Reviewed-on: https://boringssl-review.googlesource.com/2281
Reviewed-by: Adam Langley <agl@google.com>
Just the negotiation portion as everything else is external. This feature is
used in WebRTC.
Change-Id: Iccc3983ea99e7d054b59010182f9a56a8099e116
Reviewed-on: https://boringssl-review.googlesource.com/2310
Reviewed-by: Adam Langley <agl@google.com>
Prior to this change, BoringSSL maintained a 2-byte buffer for alerts,
and would support reassembly of fragmented alerts.
NSS does not support fragmented alerts, nor would any reasonable
implementation produce them. Remove fragmented alert handling and
produce an error if a fragmented alert has ever been encountered.
Change-Id: I31530ac372e8a90b47cf89404630c1c207cfb048
Reviewed-on: https://boringssl-review.googlesource.com/2125
Reviewed-by: Adam Langley <agl@google.com>
All of NSS, upstream OpenSSL, SChannel, and Secure Transport require, on the
client, that the ServerHello version match the session's version on resumption.
OpenSSL's current behavior is incompatible with all of these. Fall back to a
full handshake on the server instead of mismatch.
Add a comment on the client for why we are, as of
30ddb434bf, not currently enforcing the same in
the client.
Change-Id: I60aec972d81368c4ec30e2fd515dabd69401d175
Reviewed-on: https://boringssl-review.googlesource.com/2244
Reviewed-by: Adam Langley <agl@google.com>
Clients all consistently reject mismatches. If a different version was
negotiated, a server should ignore the resumption. This doesn't actually affect
current tests. We really want to be making this change in BoringSSL (and then
upstream), but get the Go half into shape first.
Change-Id: Ieee7e141331d9e08573592e661889bd756dccfa9
Reviewed-on: https://boringssl-review.googlesource.com/2243
Reviewed-by: Adam Langley <agl@google.com>
The limit increased from 32 to 255 between DTLS 1.0 and DTLS 1.2.
Change-Id: I329a59f9ba2bccc70282e2b47679c57b67e5ed43
Reviewed-on: https://boringssl-review.googlesource.com/2242
Reviewed-by: Adam Langley <agl@google.com>
These'll get removed once most of renego support is gone, but this is to prove
removing the warning alert from the previous commit still prevents legacy
renegotiations.
Change-Id: I7d9d95e1d4c5d23d3b6d170938a5499a65f2d5ea
Reviewed-on: https://boringssl-review.googlesource.com/2236
Reviewed-by: Adam Langley <agl@google.com>
Ensure that the client rejects it with UNEXPECTED_MESSAGE, not by attempting to
decode it.
Change-Id: Ifc5613cf1152e0f7dcbee73e05df1ef367dfbfd5
Reviewed-on: https://boringssl-review.googlesource.com/2232
Reviewed-by: Adam Langley <agl@google.com>
There's not much point in retaining the identity hint in the SSL_SESSION. This
avoids the complexity around setting psk_identity hint on either the SSL or the
SSL_SESSION. Introduce a peer_psk_identity_hint for the client to store the one
received from the server.
This changes the semantics of SSL_get_psk_identity_hint; it now only returns
the value configured for the server. The client learns the hint through the
callback. This is compatible with the one use of this API in conscrypt (it
pulls the hint back out to pass to a callback).
Change-Id: I6d9131636b47f13ac5800b4451436a057021054a
Reviewed-on: https://boringssl-review.googlesource.com/2213
Reviewed-by: Adam Langley <agl@google.com>
This is an experimental flag that dates back to SSLeay 0.8.1b or earlier. It's
never set internally and never set in consumers.
Change-Id: I922583635c9f3d8d93f08f1707531ad22a26ae6a
Reviewed-on: https://boringssl-review.googlesource.com/2214
Reviewed-by: Adam Langley <agl@google.com>
At the record layer, DTLS maintains a window of seen sequence numbers to detect
replays. Add tests to cover that case. Test both repeated sequence numbers
within the window and sequence numbers past the window's left edge. Also test
receiving sequence numbers far past the window's right edge.
Change-Id: If6a7a24869db37fdd8fb3c4b3521b730e31f8f86
Reviewed-on: https://boringssl-review.googlesource.com/2221
Reviewed-by: Adam Langley <agl@google.com>
If psk_len were 0, it would already have been an error earlier. The PSK cipher
suites don't lose the other_secret || psk construction if the PSK happens to be
empty.
Change-Id: I1917236720d0862658562bc8f014cb827ee9aed5
Reviewed-on: https://boringssl-review.googlesource.com/2233
Reviewed-by: Adam Langley <agl@google.com>
Server-side support was removed in 77a942b7fe,
but client-side support was retained as it appeared NSS supported this.
However, this is not the case: ssl3_HandleServerKeyExchange only allows a
ServerKeyExchange message if hs.ws is in an appropriate state.
ssl3_AuthCertificate only sets it to allow ServerKeyExchange if it is a key
exchange that normally uses it or if is_limited is set. is_limited is only set
for the export cipher suites.
Thus we can safely remove this without waiting on gathering UMA data.
BUG=chromium:400587
Change-Id: I9aefb742dbb2d99c13340ab48017e1ceee04bc2f
Reviewed-on: https://boringssl-review.googlesource.com/2230
Reviewed-by: Adam Langley <agl@google.com>
This was added in upstream's 82e610e2cfbbb5fd29c09785b6909a91e606f347. The
commit message cites draft-ietf-tls-renegotiation which was on
draft-ietf-tls-renegotiation-01 at the time. The text in question (6.2 Server
Considerations) is no longer in RFC 5746. The RFC now recommends terminating
the connection which is much simpler.
It also was wrong anyway as it checked s->ctx->options instead of s->options
for SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION.
Removing that block will result in the connection being terminated in
ssl_scan_clienthello_tlsext.
Change-Id: Ie222c78babd3654c5023ad07ac0d8e0adde68698
Reviewed-on: https://boringssl-review.googlesource.com/2235
Reviewed-by: Adam Langley <agl@google.com>
Parameters like these should not change between 32-bit and 64-bit. 64 is also
the value recommended in RFC 6347, section 4.1.2.6. Document those fields while
I'm here.
Change-Id: I8481ee0765ff3d261a96a2e1a53b6ad6695b2d42
Reviewed-on: https://boringssl-review.googlesource.com/2222
Reviewed-by: Adam Langley <agl@google.com>
This was added in http://rt.openssl.org/Ticket/Display.html?id=2033 to support
a mode where a DTLS socket would statelessly perform the ClientHello /
HelloVerifyRequest portion of the handshake, to be handed off to a socket
specific to this peer address.
This is not used by WebRTC or other current consumers. If we need to support
something like this, it would be cleaner to do the listen portion (cookieless
ClientHello + HelloVerifyRequest) externally and then spin up an SSL instance
on receipt of a cookied ClientHello. This would require a slightly more complex
BIO to replay the second ClientHello but would avoid peppering the DTLS
handshake state with a special short-circuiting mode.
Change-Id: I7a413932edfb62f8b9368912a9a0621d4155f1aa
Reviewed-on: https://boringssl-review.googlesource.com/2220
Reviewed-by: Adam Langley <agl@google.com>
(Imported form upstream's 455b65dfab0de51c9f67b3c909311770f2b3f801 and
0d6a11a91f4de238ce533c40bd9507fe5d95f288)
Change-Id: Ia195c7fe753cfa3a7f8c91d2d7b2cd40a547be43
Imported from upstream's 9bed73adaa6f834177f29e478d9a2247a6577c04.
Upstream's commit appears to have been based on BoringSSL's commits to
improve the constant-time behaviour of RSA padding checks and thus I've
not tried to import those bits of the change.
Change-Id: I0ea5775b0f1e18741bbbc9f792a6af0d3d2a4caf
Pull constant-time methods out to a separate header, add tests.
(Imported from upstream's 9a9b0c0401cae443f115ff19921d347b20aa396b and
27739e92659d38cdefa21e51b7f52b81a7ac3388)
Change-Id: Id570f5c531aca791112929e6258989f43c8a78d7
State hanging off the SSL gets freed in two places.
Change-Id: I41a8d2a7cab35f0098396006e1f6380038ec471a
Reviewed-on: https://boringssl-review.googlesource.com/2212
Reviewed-by: Adam Langley <agl@google.com>
Also add a few other assertions.
Change-Id: Iae0c65802f4d05c7585e2790be5295f478e1f614
Reviewed-on: https://boringssl-review.googlesource.com/2210
Reviewed-by: Adam Langley <agl@google.com>
s->s3 is never NULL if an ssl3_* function is called, and we'll crash later
anyway. (This also makes scan-build stop believing it can be NULL.)
Change-Id: Ibf8433bd4d945f9bf5416d72946102a9e50d2787
Reviewed-on: https://boringssl-review.googlesource.com/2206
Reviewed-by: Adam Langley <agl@google.com>
Check the return value while we're here. This avoids some arithmetic and
appease scan-build's dead assignment flagger.
Change-Id: If3615076e091eb44b9e3e9d50cd64f80e645337e
Reviewed-on: https://boringssl-review.googlesource.com/2204
Reviewed-by: Adam Langley <agl@google.com>
