Among many many problems with renegotiation is it makes every API
ambiguous. Do we return the pending handshake's properties, or the most
recently completed handshake? Neither answer is unambiguously correct:
On the one hand, OpenSSL's API makes renegotiation transparent, so the
pending handshake breaks invariants. E.g., currently,
SSL_get_current_cipher and other functions can return NULL mid
renegotiation. See https://crbug.com/1010748.
On the other hand, OpenSSL's API is callback-heavy. During a handshake
callback, the application most likely wants to check the pending
parameters. Most notably, cert verify callbacks calling
SSL_get_peer_certificate.
Historically, only the pending state was available to return anyway.
We've since changed this
(https://boringssl-review.googlesource.com/8612), but we kept the public
APIs as-is. I was particularly worried about cert verify callbacks.
As of https://boringssl-review.googlesource.com/c/boringssl/+/14028/ and
https://boringssl-review.googlesource.com/c/boringssl/+/19665/, cert
verify is moot. We implement the 3-SHAKE mitigation in library, so the
peer cert cannot change, and we don't reverify the certificate at all.
With that, I think we should switch to returning the established
parameters. Chromium is the main consumer that enables renegotiation,
and it would be better off with this behavior. (Maybe we should try to
forbid other properties, like the cipher suite, from changing on
renegotiation. Unchangeable properties make this issue moot.)
This CL would break if the handshake internally used SSL_get_session,
but this is no longer true as of
https://boringssl-review.googlesource.com/c/boringssl/+/41865.
Update-Note: Some APIs will now behave differently mid-renegotation. I
think this is the safer option, but it is possible code was relying on
the other one.
Fixed: chromium:1010748
Change-Id: I42157ccd9704cde3eebf947136d47cda6754c36e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
ssl_update_cache takes the cache lock to add to the session cache,
releases it, and then immediately takes and releases the lock to
increment handshakes_since_cache_flush. Then, in 1/255 connections, does
the same thing again to flush stale sessions.
Merge the first two into one lock. In doing so, move ssl_update_cache to
ssl_session.cc, so it can access a newly-extracted add_session_lock.
Also remove the mode parameter (the SSL knows if it's a client or
server), and move the established_session != session check to the
caller, which more directly knows whether there was a new session.
Also add some TSan coverage for this path in the tests. In an earlier
iteration of this patch, I managed to introduce a double-locking bug
because we weren't testing it at all. Confirmed this test catches both
double-locking and insufficient locking. (It doesn't seem able to catch
using a read lock instead of a write lock in SSL_CTX_flush_sessions,
however. I suspect the hash table is distributing the cells each thread
touches.)
Update-Note: This reshuffles some locks around the session cache.
(Hopefully for the better.)
Change-Id: I78dca53fda74e036b90110cca7fbcc306a5c8ebe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48133
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The remaining remnants of Channel ID all configure the private key ahead
of time. Unwind the callback machinery, which cuts down on async points
and the cases we need to test.
This also unwinds some odd interaction between the callback and
SSL_set_tls_channel_id_enabled: If a client uses
SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake
would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now
removed, so SSL_set_tls_channel_id_enabled only affects the server and
SSL_CTX_set1_tls_channel_id only affects the client.
Update-Note: SSL_CTX_set_channel_id_cb is removed.
SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client,
only as a server.
Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907
Reviewed-by: Adam Langley <agl@google.com>
This aligns with OpenSSL. In particular, we clear not_resumable as soon
as the SSL_SESSION is complete, but it may not have an ID or ticket.
(Due to APIs like SSL_get_session, SSL_SESSION needs to act both as a
resumption handle and a bundle of connection properties.)
Along the way, use the modified function in a few internal checks which,
with the ssl_update_cache change, removes the last dependency within the
library on the placeholder SHA256 IDs.
Change-Id: Ic225109ff31ec63ec08625e9f61a20cf0d9dd648
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47447
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's kind of weird that we assign a session ID, based on whether we
detect the handshake wants stateful resumption, and then erase it
afterwards.
Also remove the is_server parameter, which we can get from hs.
Change-Id: I94ac817c63abb08a457e0e0c29f5c2d2b60aa498
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47444
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's not even accurate. The term "master key" dates to SSL 2, which we
do not implement. (Starting SSL 3, "key" was replaced with "secret".)
The field stores, at various points, the TLS 1.2 master secret, the TLS
1.3 resumption master secret, and the TLS 1.3 resumption PSK. Simply
rename the field to 'secret', which is as descriptive of a name as we
can get at this point.
I've left SSL_SESSION_get_master_key alone for now, as it's there for
OpenSSL compatibility, as well as references to the various TLS secrets
since those refer to concepts in the spec. (When the dust settles a bit
on rfc8446bis, we can fix those.)
Change-Id: I3c1007eb7982788789cc5db851de8724c7f35baf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44144
Reviewed-by: Adam Langley <agl@google.com>
(Original CL by svaldez, reworked by davidben.)
Change-Id: I8570808fa5e96a1c9e6e03c4877039a22e73254f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42404
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The quic_early_data_context should always be saved in the SSL_SESSION.
This change fixes a bug where it was only saved in the SSL_SESSION on
full handshakes (but not resumption handshakes).
Bug: 376
Change-Id: I3659d3398e85ac4263760b504d7ea8458fc7e1e2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43264
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
David Benjamin
Steven Valdez
Nick Harper
Adam Langley