Stop pretending to ssl_clear_bad_session.
We broke this to varying degrees ages ago.
This is the logic to implement the variations of rules in TLS to discard
sessions after a failed connection, where a failed connection could be
one of:
- A connection that was not cleanly shut down.
- A connection that received a fatal alert.
The first one is nonsense since close_notify does not actually work in
the real world. The second is a vaguely more plausible but...
- A stateless ticket-based server can't drop sessions anyway.
- In TLS 1.3, a client may receive many tickets over the lifetime of a
single connection. With an external session cache like ours which may,
in theory, but multithreaded, this will be a huge hassle to track.
- A client may well attempt to establish a connection and reuse the
session before we receive the fatal alert, so any application state we
hope to manage won't really work.
- An attacker can always close the connection before the fatal alert, so
whatever security policy clearing the session gave is easily
bypassable.
Implementation-wise, this has basically never worked. The
ssl_clear_bad_session logic called into SSL_CTX_remove_session which
relied on the internal session cache. (Sessions not in the internal
session cache don't get removed.) The internal session cache was only
useful for a server, where tickets prevent this mechanism from doing
anything. For a client, we since removed the internal session cache, so
nothing got removed. The API for a client also did not work as it gave
the SSL_SESSION, not the SSL, so a consumer would not know the key to
invalidate anyway.
The recent session state splitting change further broke this.
Moreover, calling into SSL_CTX_remove_session logic like that is
extremely dubious because it mutates the not_resumable flag on the
SSL_SESSION which isn't thread-safe.
Spec-wise, TLS 1.3 has downgraded the MUST to a SHOULD.
Given all that mess, just remove this code. It is no longer necessary to
call SSL_shutdown just to make session caching work.
Change-Id: Ib601937bfc5f6b40436941e1c86566906bb3165d
Reviewed-on: https://boringssl-review.googlesource.com/9091
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>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 90db2ca..d9c8bf6 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -397,14 +397,7 @@
*
* |SSL_shutdown| returns -1 on failure. The caller should pass the return value
* into |SSL_get_error| to determine how to proceed. If the underlying |BIO| is
- * non-blocking, both stages may require retry.
- *
- * |SSL_shutdown| must be called to retain |ssl|'s session in the session
- * cache. Use |SSL_CTX_set_quiet_shutdown| to configure |SSL_shutdown| to
- * neither send nor wait for close_notify but still retain the session.
- *
- * TODO(davidben): Is there any point in the session cache interaction? Remove
- * it? */
+ * non-blocking, both stages may require retry. */
OPENSSL_EXPORT int SSL_shutdown(SSL *ssl);
/* SSL_CTX_set_quiet_shutdown sets quiet shutdown on |ctx| to |mode|. If
diff --git a/ssl/internal.h b/ssl/internal.h
index 6ab8f4b..5c8f32c 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1210,7 +1210,6 @@
#define SSL_TICKET_ALLOW_DHE_RESUMPTION 2
#define SSL_TICKET_ALLOW_PSK_RESUMPTION 4
-int ssl_clear_bad_session(SSL *ssl);
CERT *ssl_cert_new(void);
CERT *ssl_cert_dup(CERT *cert);
void ssl_cert_clear_certs(CERT *c);
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 459352d..b1a6aa9 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -456,9 +456,6 @@
}
if (level == SSL3_AL_FATAL) {
- if (ssl->session != NULL) {
- SSL_CTX_remove_session(ssl->ctx, ssl->session);
- }
ssl->s3->send_shutdown = ssl_shutdown_fatal_alert;
} else if (level == SSL3_AL_WARNING && desc == SSL_AD_CLOSE_NOTIFY) {
ssl->s3->send_shutdown = ssl_shutdown_close_notify;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 9fe78df..a9df4e8 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -494,7 +494,6 @@
ssl_cipher_preference_list_free(ssl->cipher_list);
sk_SSL_CIPHER_free(ssl->cipher_list_by_id);
- ssl_clear_bad_session(ssl);
SSL_SESSION_free(ssl->session);
ssl_cert_free(ssl->cert);
@@ -2895,11 +2894,6 @@
return 0;
}
- if (ssl_clear_bad_session(ssl)) {
- SSL_SESSION_free(ssl->session);
- ssl->session = NULL;
- }
-
/* SSL_clear may be called before or after the |ssl| is initialized in either
* accept or connect state. In the latter case, SSL_clear should preserve the
* half and reset |ssl->state| accordingly. */
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index cb1edb9..3bd52e0 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -845,17 +845,6 @@
CRYPTO_MUTEX_unlock_write(&ctx->lock);
}
-int ssl_clear_bad_session(SSL *ssl) {
- if (ssl->session != NULL &&
- ssl->s3->send_shutdown != ssl_shutdown_close_notify &&
- !SSL_in_init(ssl)) {
- SSL_CTX_remove_session(ssl->ctx, ssl->session);
- return 1;
- }
-
- return 0;
-}
-
/* locked by SSL_CTX in the calling function */
static void SSL_SESSION_list_remove(SSL_CTX *ctx, SSL_SESSION *session) {
if (session->next == NULL || session->prev == NULL) {
diff --git a/ssl/tls_record.c b/ssl/tls_record.c
index 2cf95ac..133fc8e 100644
--- a/ssl/tls_record.c
+++ b/ssl/tls_record.c
@@ -448,7 +448,6 @@
if (alert_level == SSL3_AL_FATAL) {
ssl->s3->recv_shutdown = ssl_shutdown_fatal_alert;
- SSL_CTX_remove_session(ssl->ctx, ssl->session);
char tmp[16];
OPENSSL_PUT_ERROR(SSL, SSL_AD_REASON_OFFSET + alert_descr);
