Prefer established session properties mid renegotiation.
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>
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 76e6dc4..05bcf6f 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -1169,20 +1169,31 @@
}
SSL_SESSION *SSL_get_session(const SSL *ssl) {
- // Once the handshake completes we return the established session. Otherwise
- // we return the intermediate session, either |session| (for resumption) or
- // |new_session| if doing a full handshake.
- if (!SSL_in_init(ssl)) {
+ // Once the initially handshake completes, we return the most recently
+ // established session. In particular, if there is a pending renegotiation, we
+ // do not return information about it until it completes.
+ //
+ // Code in the handshake must either use |hs->new_session| (if updating a
+ // partial session) or |ssl_handshake_session| (if trying to query properties
+ // consistently across TLS 1.2 resumption and other handshakes).
+ if (ssl->s3->established_session != nullptr) {
return ssl->s3->established_session.get();
}
+
+ // Otherwise, we must be in the initial handshake.
SSL_HANDSHAKE *hs = ssl->s3->hs.get();
+ assert(hs != nullptr);
+ assert(!ssl->s3->initial_handshake_complete);
+
+ // Return the 0-RTT session, if in the 0-RTT state. While the handshake has
+ // not actually completed, the public accessors all report properties as if
+ // it has.
if (hs->early_session) {
return hs->early_session.get();
}
- if (hs->new_session) {
- return hs->new_session.get();
- }
- return ssl->session.get();
+
+ // Otherwise, return the partial session.
+ return (SSL_SESSION *)ssl_handshake_session(hs);
}
SSL_SESSION *SSL_get1_session(SSL *ssl) {
