Never resume sessions on renegotiations.

This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:

- OpenSSL will only ever offer the session it just established,
  whether or not a newer one with client auth was since established.

- Chrome will never cache sessions created on a renegotiation, so
  such a session would never make it to the session cache.

- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
  logic had a bug where it would unconditionally never offer tickets
  (but would advertise support) on renego, so any server doing renego
  resumption against an OpenSSL-derived client must not support
  session tickets.

This also gets rid of s->new_session which is now pointless.

BUG=429450

Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index e7cd176..134e69d 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -475,8 +475,6 @@
 /* Don't use RFC4507 ticket extension */
 #define SSL_OP_NO_TICKET 0x00004000L
 
-/* As server, disallow session resumption on renegotiation */
-#define SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION 0x00010000L
 /* Permit unsafe legacy renegotiation */
 #define SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION 0x00040000L
 /* Set on servers to choose the cipher according to the server's preferences */
@@ -502,6 +500,7 @@
 #define SSL_OP_MICROSOFT_SESS_ID_BUG 0
 #define SSL_OP_NETSCAPE_CHALLENGE_BUG 0
 #define SSL_OP_NO_COMPRESSION 0
+#define SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION 0
 #define SSL_OP_NO_SSLv2 0
 #define SSL_OP_SINGLE_DH_USE 0
 #define SSL_OP_SINGLE_ECDH_USE 0
@@ -1264,12 +1263,6 @@
    * the side is not determined. In this state, server is always false. */
   int server;
 
-
-  /* Generate a new session or reuse an old one. NB: For servers, the 'new'
-   * session may actually be a previously cached session or even the previous
-   * session unless SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION is set */
-  int new_session;
-
   /* quiet_shutdown is true if the connection should not send a close_notify on
    * shutdown. */
   int quiet_shutdown;
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index a5a7b0c..0d592d0 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -473,7 +473,6 @@
 
         s->init_num = 0;
         s->renegotiate = 0;
-        s->new_session = 0;
         s->s3->initial_handshake_complete = 1;
 
         ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index d93b26f..442e5e1 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -474,7 +474,6 @@
         if (s->renegotiate == 2) {
           /* skipped if we just sent a HelloRequest */
           s->renegotiate = 0;
-          s->new_session = 0;
           s->s3->initial_handshake_complete = 1;
 
           ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 700c938..516ca4a 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -552,7 +552,6 @@
 
         s->init_num = 0;
         s->renegotiate = 0;
-        s->new_session = 0;
         s->s3->tmp.in_false_start = 0;
         s->s3->initial_handshake_complete = 1;
 
@@ -667,7 +666,8 @@
     p += SSL3_RANDOM_SIZE;
 
     /* Session ID */
-    if (s->new_session || s->session == NULL) {
+    if (s->s3->initial_handshake_complete || s->session == NULL) {
+      /* Renegotiations do not participate in session resumption. */
       i = 0;
     } else {
       i = s->session->session_id_length;
@@ -806,8 +806,9 @@
   memcpy(s->s3->server_random, CBS_data(&server_random), SSL3_RANDOM_SIZE);
 
   assert(s->session == NULL || s->session->session_id_length > 0);
-  if (s->session != NULL && CBS_mem_equal(&session_id, s->session->session_id,
-                                          s->session->session_id_length)) {
+  if (!s->s3->initial_handshake_complete && s->session != NULL &&
+      CBS_mem_equal(&session_id, s->session->session_id,
+                    s->session->session_id_length)) {
     if (s->sid_ctx_length != s->session->sid_ctx_length ||
         memcmp(s->session->sid_ctx, s->sid_ctx, s->sid_ctx_length)) {
       /* actually a client application bug */
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 75d4df7..dded371 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -1048,7 +1048,6 @@
     if ((s->state & SSL_ST_MASK) == SSL_ST_OK) {
       s->state = s->server ? SSL_ST_ACCEPT : SSL_ST_CONNECT;
       s->renegotiate = 1;
-      s->new_session = 1;
     }
     i = s->handshake_func(s);
     if (i < 0) {
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 3e1e7c2..a5280e9 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -643,7 +643,6 @@
         if (s->renegotiate == 2) {
           /* skipped if we just sent a HelloRequest */
           s->renegotiate = 0;
-          s->new_session = 0;
           s->s3->initial_handshake_complete = 1;
 
           ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
@@ -1040,19 +1039,8 @@
   }
 
   s->hit = 0;
-  /* Versions before 0.9.7 always allow clients to resume sessions in
-   * renegotiation. 0.9.7 and later allow this by default, but optionally
-   * ignore resumption requests with flag
-   * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION (it's a new flag rather than
-   * a change to default behavior so that applications relying on this for
-   * security won't even compile against older library versions).
-   *
-   * 1.0.1 and later also have a function SSL_renegotiate_abbreviated() to
-   * request renegotiation but not a new session (s->new_session remains
-   * unset): for servers, this essentially just means that the
-   * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION setting will be ignored. */
-  if (s->new_session &&
-      (s->options & SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION)) {
+  if (s->s3->initial_handshake_complete) {
+    /* Renegotiations do not participate in session resumption. */
     if (!ssl_get_new_session(s, 1)) {
       goto err;
     }
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 15bb8be..567f61e 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -955,7 +955,6 @@
     s->renegotiate = 1;
   }
 
-  s->new_session = 1;
   return s->method->ssl_renegotiate(s);
 }
 
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 4830fd5..7899561 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -905,7 +905,12 @@
 
   if (!(SSL_get_options(s) & SSL_OP_NO_TICKET)) {
     int ticklen = 0;
-    if (!s->new_session && s->session && s->session->tlsext_tick) {
+    /* Renegotiation does not participate in session resumption. However, still
+     * advertise the extension to avoid potentially breaking servers which carry
+     * over the state from the previous handshake, such as OpenSSL servers
+     * without upstream's 3c3f0259238594d77264a78944d409f2127642c4. */
+    if (!s->s3->initial_handshake_complete && s->session != NULL &&
+        s->session->tlsext_tick != NULL) {
       ticklen = s->session->tlsext_ticklen;
     }
 
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 4ac7250..2e6ddf3 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -681,9 +681,9 @@
 	// fragments in DTLS.
 	SendEmptyFragments bool
 
-	// NeverResumeOnRenego, if true, causes renegotiations to always be full
-	// handshakes.
-	NeverResumeOnRenego bool
+	// FailIfResumeOnRenego, if true, causes renegotiations to fail if the
+	// client offers a resumption or the server accepts one.
+	FailIfResumeOnRenego bool
 
 	// NoSignatureAlgorithmsOnRenego, if true, causes renegotiations to omit
 	// the signature_algorithms extension.
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 0dac05d..5129b8f 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -140,9 +140,6 @@
 	var session *ClientSessionState
 	var cacheKey string
 	sessionCache := c.config.ClientSessionCache
-	if c.config.Bugs.NeverResumeOnRenego && c.cipherSuite != nil {
-		sessionCache = nil
-	}
 
 	if sessionCache != nil {
 		hello.ticketSupported = !c.config.SessionTicketsDisabled
@@ -727,6 +724,12 @@
 	}
 
 	if hs.serverResumedSession() {
+		// For test purposes, assert that the server never accepts the
+		// resumption offer on renegotiation.
+		if c.cipherSuite != nil && c.config.Bugs.FailIfResumeOnRenego {
+			return false, errors.New("tls: server resumed session on renegotiation")
+		}
+
 		// Restore masterSecret and peerCerts from previous state
 		hs.masterSecret = hs.session.masterSecret
 		c.peerCertificates = hs.session.serverCertificates
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 59ed9df..f159aff 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -333,6 +333,12 @@
 
 	_, hs.ecdsaOk = hs.cert.PrivateKey.(*ecdsa.PrivateKey)
 
+	// For test purposes, check that the peer never offers a session when
+	// renegotiating.
+	if c.cipherSuite != nil && len(hs.clientHello.sessionId) > 0 && c.config.Bugs.FailIfResumeOnRenego {
+		return false, errors.New("tls: offered resumption on renegotiation")
+	}
+
 	if hs.checkForResumption() {
 		return true, nil
 	}
@@ -382,10 +388,6 @@
 func (hs *serverHandshakeState) checkForResumption() bool {
 	c := hs.c
 
-	if c.config.Bugs.NeverResumeOnRenego && c.cipherSuite != nil {
-		return false
-	}
-
 	if len(hs.clientHello.sessionTicket) > 0 {
 		if c.config.SessionTicketsDisabled {
 			return false
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index ec2fede..6c16020 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2858,17 +2858,11 @@
 
 func addRenegotiationTests() {
 	testCases = append(testCases, testCase{
-		testType:        serverTest,
-		name:            "Renegotiate-Server",
-		flags:           []string{"-renegotiate"},
-		shimWritesFirst: true,
-	})
-	testCases = append(testCases, testCase{
 		testType: serverTest,
-		name:     "Renegotiate-Server-Full",
+		name:     "Renegotiate-Server",
 		config: Config{
 			Bugs: ProtocolBugs{
-				NeverResumeOnRenego: true,
+				FailIfResumeOnRenego: true,
 			},
 		},
 		flags:           []string{"-renegotiate"},
@@ -2943,7 +2937,6 @@
 		name:     "Renegotiate-Server-NoSignatureAlgorithms",
 		config: Config{
 			Bugs: ProtocolBugs{
-				NeverResumeOnRenego:           true,
 				NoSignatureAlgorithmsOnRenego: true,
 			},
 		},
@@ -2952,14 +2945,10 @@
 	})
 	// TODO(agl): test the renegotiation info SCSV.
 	testCases = append(testCases, testCase{
-		name:        "Renegotiate-Client",
-		renegotiate: true,
-	})
-	testCases = append(testCases, testCase{
-		name: "Renegotiate-Client-Full",
+		name: "Renegotiate-Client",
 		config: Config{
 			Bugs: ProtocolBugs{
-				NeverResumeOnRenego: true,
+				FailIfResumeOnRenego: true,
 			},
 		},
 		renegotiate: true,