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>

include/openssl/ssl.h

@@ -397,14 +397,7 @@ OPENSSL_EXPORT int SSL_write(SSL *ssl, const void *buf, int num);
  *
  * |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

ssl/internal.h

@@ -1210,7 +1210,6 @@ extern const SSL3_ENC_METHOD SSLv3_enc_data;
 #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);

ssl/s3_pkt.c

@@ -456,9 +456,6 @@ int ssl3_send_alert(SSL *ssl, int level, int desc) {
   }
 
   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;

ssl/ssl_lib.c

@@ -494,7 +494,6 @@ void SSL_free(SSL *ssl) {
   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 @@ int SSL_clear(SSL *ssl) {
     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. */

ssl/ssl_session.c

@@ -845,17 +845,6 @@ void SSL_CTX_flush_sessions(SSL_CTX *ctx, long time) {
   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) {

ssl/tls_record.c

@@ -448,7 +448,6 @@ enum ssl_open_record_t ssl_process_alert(SSL *ssl, uint8_t *out_alert,
 
   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);