commit f01f42a2ce161657c6413ef71d7c04262c7abd56
author David Benjamin <davidben@google.com> Wed Nov 16 19:05:33 2016 +0900
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu Nov 17 01:02:42 2016 +0000
tree 4b793e5679eb928332f517a278af74a45df3cf0b
parent 34202b93b6195e9c55402a6bc2653956d0cd26d8

Negotiate ciphers before resumption.

This changes our resumption strategy. Before, we would negotiate ciphers
only on fresh handshakes. On resumption, we would blindly use whatever
was in the session.

Instead, evaluate cipher suite preferences on every handshake.
Resumption requires that the saved cipher suite match the one that would
have been negotiated anyway. If client or server preferences changed
sufficiently, we decline the session.

This is much easier to reason about (we always pick the best cipher
suite), simpler, and avoids getting stuck under old preferences if
tickets are continuously renewed. Notably, although TLS 1.2 ticket
renewal does not work in practice, TLS 1.3 will renew tickets like
there's no tomorrow.

It also means we don't need dedicated code to avoid resuming a cipher
which has since been disabled. (That dedicated code was a little odd
anyway since the mask_k, etc., checks didn't occur. When cert_cb was
skipped on resumption, one could resume without ever configuring a
certificate! So we couldn't know whether to mask off RSA or ECDSA cipher
suites.)

Add tests which assert on this new arrangement.

BUG=116

Change-Id: Id40d851ccd87e06c46c6ec272527fd8ece8abfc6
Reviewed-on: https://boringssl-review.googlesource.com/11847
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>

ssl/handshake_server.c

diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index ff91697..a0562d8 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -762,6 +762,16 @@
       }
     }
 
+    /* Negotiate the cipher suite. This must be done after |cert_cb| so the
+     * certificate is finalized. */
+    ssl->s3->tmp.new_cipher =
+        ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl));
+    if (ssl->s3->tmp.new_cipher == NULL) {
+      al = SSL_AD_HANDSHAKE_FAILURE;
+      OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
+      goto f_err;
+    }
+
     ssl->state = SSL3_ST_SR_CLNT_HELLO_E;
   }
 
@@ -827,27 +837,8 @@
     goto f_err;
   }
 
-  if (ssl->session != NULL) {
-    /* Check that the cipher is in the list. */
-    if (!ssl_client_cipher_list_contains_cipher(
-            &client_hello, (uint16_t)ssl->session->cipher->id)) {
-      al = SSL_AD_ILLEGAL_PARAMETER;
-      OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING);
-      goto f_err;
-    }
-
-    ssl->s3->tmp.new_cipher = ssl->session->cipher;
-  } else {
-    const SSL_CIPHER *c =
-        ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl));
-    if (c == NULL) {
-      al = SSL_AD_HANDSHAKE_FAILURE;
-      OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
-      goto f_err;
-    }
-
-    ssl->s3->new_session->cipher = c;
-    ssl->s3->tmp.new_cipher = c;
+  if (ssl->session == NULL) {
+    ssl->s3->new_session->cipher = ssl->s3->tmp.new_cipher;
 
     /* On new sessions, stash the SNI value in the session. */
     if (ssl->s3->hs->hostname != NULL) {
@@ -877,13 +868,13 @@
     }
   }
 
-  /* Resolve ALPN after the cipher suite is selected. HTTP/2 negotiation depends
-   * on the cipher suite. */
+  /* HTTP/2 negotiation depends on the cipher suite, so ALPN negotiation was
+   * deferred. Complete it now. */
   if (!ssl_negotiate_alpn(ssl, &al, &client_hello)) {
     goto f_err;
   }
 
-  /* Now that the cipher is known, initialize the handshake hash. */
+  /* Now that all parameters are known, initialize the handshake hash. */
   if (!ssl3_init_handshake_hash(ssl)) {
     goto f_err;
   }