commit 52bf690ba46480cd5e52878449fe099195e475d3
author David Benjamin <davidben@google.com> Sat Oct 08 12:05:03 2016 -0400
committer Adam Langley <agl@google.com> Sun Oct 09 17:52:18 2016 +0000
tree 5a5ecb8abd942e21e285dc6559d65d5e6b1c82fd
parent 34941c0cab2932fcb40652511ab071fc8d050912

Saved Finished messages are twelve bytes.

We only save them at TLS 1.0 through 1.2. This saves 104 bytes of
per-connection memory.

Change-Id: If397bdc10e40f0194cba01024e0e9857d6b812f0
Reviewed-on: https://boringssl-review.googlesource.com/11571
Reviewed-by: Adam Langley <agl@google.com>

include/openssl/ssl.h

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 7ff5e39..8d105aa 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4366,9 +4366,9 @@
   unsigned session_reused:1;
 
   /* Connection binding to prevent renegotiation attacks */
-  uint8_t previous_client_finished[EVP_MAX_MD_SIZE];
+  uint8_t previous_client_finished[12];
   uint8_t previous_client_finished_len;
-  uint8_t previous_server_finished[EVP_MAX_MD_SIZE];
+  uint8_t previous_server_finished[12];
   uint8_t previous_server_finished_len;
   int send_connection_binding;
 

ssl/s3_both.c

diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 0e0c059..cb5afab 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -254,13 +254,21 @@
     return 0;
   }
 
-  /* Copy the finished so we can use it for renegotiation checks. */
-  if (ssl->server) {
-    memcpy(ssl->s3->previous_server_finished, finished, finished_len);
-    ssl->s3->previous_server_finished_len = finished_len;
-  } else {
-    memcpy(ssl->s3->previous_client_finished, finished, finished_len);
-    ssl->s3->previous_client_finished_len = finished_len;
+  /* Copy the Finished so we can use it for renegotiation checks. */
+  if (ssl->version != SSL3_VERSION) {
+    if (finished_len > sizeof(ssl->s3->previous_client_finished) ||
+        finished_len > sizeof(ssl->s3->previous_server_finished)) {
+      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+      return -1;
+    }
+
+    if (ssl->server) {
+      memcpy(ssl->s3->previous_server_finished, finished, finished_len);
+      ssl->s3->previous_server_finished_len = finished_len;
+    } else {
+      memcpy(ssl->s3->previous_client_finished, finished, finished_len);
+      ssl->s3->previous_client_finished_len = finished_len;
+    }
   }
 
   CBB cbb, body;
@@ -303,13 +311,21 @@
     return -1;
   }
 
-  /* Copy the finished so we can use it for renegotiation checks. */
-  if (ssl->server) {
-    memcpy(ssl->s3->previous_client_finished, finished, finished_len);
-    ssl->s3->previous_client_finished_len = finished_len;
-  } else {
-    memcpy(ssl->s3->previous_server_finished, finished, finished_len);
-    ssl->s3->previous_server_finished_len = finished_len;
+  /* Copy the Finished so we can use it for renegotiation checks. */
+  if (ssl->version != SSL3_VERSION) {
+    if (finished_len > sizeof(ssl->s3->previous_client_finished) ||
+        finished_len > sizeof(ssl->s3->previous_server_finished)) {
+      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+      return -1;
+    }
+
+    if (ssl->server) {
+      memcpy(ssl->s3->previous_client_finished, finished, finished_len);
+      ssl->s3->previous_client_finished_len = finished_len;
+    } else {
+      memcpy(ssl->s3->previous_server_finished, finished, finished_len);
+      ssl->s3->previous_server_finished_len = finished_len;
+    }
   }
 
   return 1;

ssl/t1_lib.c

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 06595d2..02c713b 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -720,6 +720,9 @@
     return 1;
   }
 
+  assert(ssl->s3->initial_handshake_complete ==
+         (ssl->s3->previous_client_finished_len != 0));
+
   CBB contents, prev_finished;
   if (!CBB_add_u16(out, TLSEXT_TYPE_renegotiate) ||
       !CBB_add_u16_length_prefixed(out, &contents) ||
@@ -765,6 +768,10 @@
   /* Check for logic errors */
   assert(!expected_len || ssl->s3->previous_client_finished_len);
   assert(!expected_len || ssl->s3->previous_server_finished_len);
+  assert(ssl->s3->initial_handshake_complete ==
+         (ssl->s3->previous_client_finished_len != 0));
+  assert(ssl->s3->initial_handshake_complete ==
+         (ssl->s3->previous_server_finished_len != 0));
 
   /* Parse out the extension contents. */
   CBS renegotiated_connection;
@@ -823,10 +830,9 @@
     return 0;
   }
 
-  /* Check that the extension matches */
-  if (!CBS_mem_equal(&renegotiated_connection,
-                     ssl->s3->previous_client_finished,
-                     ssl->s3->previous_client_finished_len)) {
+  /* Check that the extension matches. We do not support renegotiation as a
+   * server, so this must be empty. */
+  if (CBS_len(&renegotiated_connection) != 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_RENEGOTIATION_MISMATCH);
     *out_alert = SSL_AD_HANDSHAKE_FAILURE;
     return 0;
@@ -838,19 +844,17 @@
 }
 
 static int ext_ri_add_serverhello(SSL *ssl, CBB *out) {
+  /* Renegotiation isn't supported as a server so this function should never be
+   * called after the initial handshake. */
+  assert(!ssl->s3->initial_handshake_complete);
+
   if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) {
     return 1;
   }
 
-  CBB contents, prev_finished;
   if (!CBB_add_u16(out, TLSEXT_TYPE_renegotiate) ||
-      !CBB_add_u16_length_prefixed(out, &contents) ||
-      !CBB_add_u8_length_prefixed(&contents, &prev_finished) ||
-      !CBB_add_bytes(&prev_finished, ssl->s3->previous_client_finished,
-                     ssl->s3->previous_client_finished_len) ||
-      !CBB_add_bytes(&prev_finished, ssl->s3->previous_server_finished,
-                     ssl->s3->previous_server_finished_len) ||
-      !CBB_flush(out)) {
+      !CBB_add_u16(out, 1 /* length */) ||
+      !CBB_add_u8(out, 0 /* empty renegotiation info */)) {
     return 0;
   }