Move post-handshake message handling out of read_app_data.

This finishes getting rid of ssl_read_bytes! Now we have separate
entry-points for the various cases. For now, I've kept TLS handshake
consuming records partially. When we do the BIO-less API, I expect that
will need to change, since we won't have the record buffer available.

(Instead, the ssl3_read_handshake_bytes and extend_handshake_buffer pair
will look more like the DTLS side or Go and pull the entire record into
init_buf.)

This change opts to make read_app_data drive the message to completion
in anticipation of DTLS 1.3. That hasn't been specified, but
NewSessionTicket certainly will exist. Knowing that DTLS necessarily has
interleave seems something better suited for the SSL_PROTOCOL_METHOD
internals to drive.

It needs refining, but SSL_PROTOCOL_METHOD is now actually a half-decent
abstraction boundary between the higher-level protocol logic and
DTLS/TLS-specific record-layer and message dispatchy bits.

BUG=83

Change-Id: I9b4626bb8a29d9cb30174d9e6912bb420ed45aff
Reviewed-on: https://boringssl-review.googlesource.com/9001
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

7 files changed, 211 insertions, 187 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 7875fe19..82d503ee 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4221,10 +4221,6 @@ typedef struct ssl3_state_st {
 
   SSL3_RECORD rrec; /* each decoded record goes in here */
 
-  /* hello_request_len is the number of bytes of HelloRequest received, possibly
-   * split over multiple records. */
-  uint8_t hello_request_len;
-
   /* partial write - check the numbers match */
   unsigned int wnum; /* number of bytes sent so far */
   int wpend_tot;     /* number bytes written */
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index ab9b9119..574fd4cd 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -197,9 +197,11 @@ again:
   return -1;
 }
 
-int dtls1_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek) {
+int dtls1_read_app_data(SSL *ssl, int *out_got_handshake, uint8_t *buf, int len,
+                        int peek) {
   assert(!SSL_in_init(ssl));
 
+  *out_got_handshake = 0;
   SSL3_RECORD *rr = &ssl->s3->rrec;
 
 again:
diff --git a/ssl/internal.h b/ssl/internal.h
index 1b3f28a9..7a4a9586 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1052,7 +1052,13 @@ struct ssl_protocol_method_st {
   /* release_current_message is called to release the current handshake message.
    * If |free_buffer| is one, buffers will also be released. */
   void (*release_current_message)(SSL *ssl, int free_buffer);
-  int (*read_app_data)(SSL *ssl, uint8_t *buf, int len, int peek);
+  /* read_app_data reads up to |len| bytes of application data into |buf|. On
+   * success, it returns the number of bytes read. Otherwise, it returns <= 0
+   * and sets |*out_got_handshake| to whether the failure was due to a
+   * post-handshake handshake message. If so, it fills in the current message as
+   * in |ssl_get_message|. */
+  int (*read_app_data)(SSL *ssl, int *out_got_handshake,  uint8_t *buf, int len,
+                       int peek);
   int (*read_change_cipher_spec)(SSL *ssl);
   void (*read_close_notify)(SSL *ssl);
   int (*write_app_data)(SSL *ssl, const void *buf_, int len);
@@ -1255,10 +1261,11 @@ int ssl3_cert_verify_hash(SSL *ssl, const EVP_MD **out_md, uint8_t *out,
 int ssl3_send_finished(SSL *ssl, int a, int b);
 int ssl3_supports_cipher(const SSL_CIPHER *cipher);
 int ssl3_dispatch_alert(SSL *ssl);
-int ssl3_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek);
+int ssl3_read_app_data(SSL *ssl, int *out_got_handshake, uint8_t *buf, int len,
+                       int peek);
 int ssl3_read_change_cipher_spec(SSL *ssl);
 void ssl3_read_close_notify(SSL *ssl);
-int ssl3_read_bytes(SSL *ssl, int type, uint8_t *buf, int len, int peek);
+int ssl3_read_handshake_bytes(SSL *ssl, uint8_t *buf, int len);
 int ssl3_write_app_data(SSL *ssl, const void *buf, int len);
 int ssl3_write_bytes(SSL *ssl, int type, const void *buf, int len);
 int ssl3_output_cert_chain(SSL *ssl);
@@ -1287,7 +1294,8 @@ int dtls1_write_message(SSL *ssl);
  * more data is needed. */
 int dtls1_get_record(SSL *ssl);
 
-int dtls1_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek);
+int dtls1_read_app_data(SSL *ssl, int *out_got_handshake, uint8_t *buf, int len,
+                        int peek);
 int dtls1_read_change_cipher_spec(SSL *ssl);
 void dtls1_read_close_notify(SSL *ssl);
 
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 7608c342..0dd67bc7 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -337,10 +337,28 @@ size_t ssl_max_handshake_message_len(const SSL *ssl) {
    * not accept peer certificate chains. */
   static const size_t kMaxMessageLen = 16384;
 
-  if ((!ssl->server || (ssl->verify_mode & SSL_VERIFY_PEER)) &&
-      kMaxMessageLen < ssl->max_cert_list) {
-    return ssl->max_cert_list;
+  if (SSL_in_init(ssl)) {
+    if ((!ssl->server || (ssl->verify_mode & SSL_VERIFY_PEER)) &&
+        kMaxMessageLen < ssl->max_cert_list) {
+      return ssl->max_cert_list;
+    }
+    return kMaxMessageLen;
   }
+
+  if (ssl3_protocol_version(ssl) < TLS1_3_VERSION) {
+    /* In TLS 1.2 and below, the largest acceptable post-handshake message is
+     * a HelloRequest. */
+    return 0;
+  }
+
+  if (ssl->server) {
+    /* The largest acceptable post-handshake message for a server is a
+     * KeyUpdate. We will never initiate post-handshake auth. */
+    return 0;
+  }
+
+  /* Clients must accept NewSessionTicket and CertificateRequest, so allow the
+   * default size. */
   return kMaxMessageLen;
 }
 
@@ -349,10 +367,9 @@ static int extend_handshake_buffer(SSL *ssl, size_t length) {
     return -1;
   }
   while (ssl->init_buf->length < length) {
-    int ret =
-        ssl3_read_bytes(ssl, SSL3_RT_HANDSHAKE,
-                        (uint8_t *)ssl->init_buf->data + ssl->init_buf->length,
-                        length - ssl->init_buf->length, 0);
+    int ret = ssl3_read_handshake_bytes(
+        ssl, (uint8_t *)ssl->init_buf->data + ssl->init_buf->length,
+        length - ssl->init_buf->length);
     if (ret <= 0) {
       return ret;
     }
@@ -584,9 +601,9 @@ again:
   ssl->init_msg = (uint8_t*)ssl->init_buf->data + SSL3_HM_HEADER_LENGTH;
   ssl->init_num = ssl->init_buf->length - SSL3_HM_HEADER_LENGTH;
 
-  /* Ignore stray HelloRequest messages. Per RFC 5246, section 7.4.1.1, the
-   * server may send HelloRequest at any time. */
-  if (!ssl->server &&
+  /* Ignore stray HelloRequest messages in the handshake before TLS 1.3. Per RFC
+   * 5246, section 7.4.1.1, the server may send HelloRequest at any time. */
+  if (!ssl->server && SSL_in_init(ssl) &&
       (!ssl->s3->have_version || ssl3_protocol_version(ssl) < TLS1_3_VERSION) &&
       ssl->s3->tmp.message_type == SSL3_MT_HELLO_REQUEST &&
       ssl->init_num == 0) {
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 3e674e6f..459352d7 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -308,11 +308,81 @@ static int do_ssl3_write(SSL *ssl, int type, const uint8_t *buf, unsigned len) {
   return ssl3_write_pending(ssl, type, buf, len);
 }
 
-int ssl3_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek) {
+static int consume_record(SSL *ssl, uint8_t *out, int len, int peek) {
+  SSL3_RECORD *rr = &ssl->s3->rrec;
+
+  if (len <= 0) {
+    return len;
+  }
+
+  if (len > (int)rr->length) {
+    len = (int)rr->length;
+  }
+
+  memcpy(out, rr->data, len);
+  if (!peek) {
+    rr->length -= len;
+    rr->data += len;
+    if (rr->length == 0) {
+      /* The record has been consumed, so we may now clear the buffer. */
+      ssl_read_buffer_discard(ssl);
+    }
+  }
+  return len;
+}
+
+int ssl3_read_app_data(SSL *ssl, int *out_got_handshake, uint8_t *buf, int len,
+                       int peek) {
   assert(!SSL_in_init(ssl));
   assert(ssl->s3->initial_handshake_complete);
+  *out_got_handshake = 0;
+
+  SSL3_RECORD *rr = &ssl->s3->rrec;
+
+  for (;;) {
+    /* A previous iteration may have read a partial handshake message. Do not
+     * allow more app data in that case. */
+    int has_hs_data = ssl->init_buf != NULL && ssl->init_buf->length > 0;
+
+    /* Get new packet if necessary. */
+    if (rr->length == 0 && !has_hs_data) {
+      int ret = ssl3_get_record(ssl);
+      if (ret <= 0) {
+        return ret;
+      }
+    }
+
+    if (has_hs_data || rr->type == SSL3_RT_HANDSHAKE) {
+      /* Post-handshake data prior to TLS 1.3 is always renegotiation, which we
+       * never accept as a server. Otherwise |ssl3_get_message| will send
+       * |SSL_R_EXCESSIVE_MESSAGE_SIZE|. */
+      if (ssl->server && ssl3_protocol_version(ssl) < TLS1_3_VERSION) {
+        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
+        OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+        return -1;
+      }
+
+      /* Parse post-handshake handshake messages. */
+      int ret = ssl3_get_message(ssl, -1, ssl_dont_hash_message);
+      if (ret <= 0) {
+        return ret;
+      }
+      *out_got_handshake = 1;
+      return -1;
+    }
+
+    if (rr->type != SSL3_RT_APPLICATION_DATA) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+      return -1;
+    }
 
-  return ssl3_read_bytes(ssl, SSL3_RT_APPLICATION_DATA, buf, len, peek);
+    if (rr->length != 0) {
+      return consume_record(ssl, buf, len, peek);
+    }
+
+    /* Discard empty records and loop again. */
+  }
 }
 
 int ssl3_read_change_cipher_spec(SSL *ssl) {
@@ -352,171 +422,30 @@ void ssl3_read_close_notify(SSL *ssl) {
   }
 }
 
-static int ssl3_can_renegotiate(SSL *ssl) {
-  if (ssl->server || ssl3_protocol_version(ssl) >= TLS1_3_VERSION) {
-    return 0;
-  }
-
-  switch (ssl->renegotiate_mode) {
-    case ssl_renegotiate_never:
-      return 0;
-    case ssl_renegotiate_once:
-      return ssl->s3->total_renegotiations == 0;
-    case ssl_renegotiate_freely:
-      return 1;
-    case ssl_renegotiate_ignore:
-      return 1;
-  }
-
-  assert(0);
-  return 0;
-}
-
-/* Return up to 'len' payload bytes received in 'type' records.
- * 'type' is one of the following:
- *
- *   -  SSL3_RT_HANDSHAKE (when ssl3_get_message calls us)
- *   -  SSL3_RT_APPLICATION_DATA (when ssl3_read_app_data calls us)
- *
- * If we don't have stored data to work from, read a SSL/TLS record first
- * (possibly multiple records if we still don't have anything to return).
- *
- * This function must handle any surprises the peer may have for us, such as
- * Alert records (e.g. close_notify) or renegotiation requests. */
-int ssl3_read_bytes(SSL *ssl, int type, uint8_t *buf, int len, int peek) {
-  int al, i, ret;
-  unsigned int n;
-  SSL3_RECORD *rr;
-
-  if ((type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE) ||
-      (peek && type != SSL3_RT_APPLICATION_DATA)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return -1;
-  }
-
-start:
-  /* ssl->s3->rrec.type    - is the type of record
-   * ssl->s3->rrec.data    - data
-   * ssl->s3->rrec.off     - offset into 'data' for next read
-   * ssl->s3->rrec.length  - number of bytes. */
-  rr = &ssl->s3->rrec;
-
-  /* get new packet if necessary */
-  if (rr->length == 0) {
-    ret = ssl3_get_record(ssl);
-    if (ret <= 0) {
-      return ret;
-    }
-  }
-
-  /* we now have a packet which can be read and processed */
+int ssl3_read_handshake_bytes(SSL *ssl, uint8_t *buf, int len) {
+  SSL3_RECORD *rr = &ssl->s3->rrec;
 
-  /* Do not allow interleaving application data and HelloRequest. */
-  if (type == rr->type && ssl->s3->hello_request_len == 0) {
-    /* Discard empty records. */
+  for (;;) {
+    /* Get new packet if necessary. */
     if (rr->length == 0) {
-      goto start;
-    }
-
-    if (len <= 0) {
-      return len;
-    }
-
-    if ((unsigned int)len > rr->length) {
-      n = rr->length;
-    } else {
-      n = (unsigned int)len;
-    }
-
-    memcpy(buf, rr->data, n);
-    if (!peek) {
-      rr->length -= n;
-      rr->data += n;
-      if (rr->length == 0) {
-        /* The record has been consumed, so we may now clear the buffer. */
-        ssl_read_buffer_discard(ssl);
+      int ret = ssl3_get_record(ssl);
+      if (ret <= 0) {
+        return ret;
       }
     }
 
-    return n;
-  }
-
-  /* Process unexpected records. */
-
-  if (type == SSL3_RT_APPLICATION_DATA && rr->type == SSL3_RT_HANDSHAKE) {
-    if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) {
-      /* TODO(svaldez): Handle TLS 1.3 post-handshake messages. For now,
-       * silently drop all handshake records. */
-      rr->length = 0;
-      goto start;
-    }
-
-    /* If peer renegotiations are disabled, all out-of-order handshake records
-     * are fatal. Renegotiations as a server are never supported. */
-    if (ssl->server || !ssl3_can_renegotiate(ssl)) {
-      al = SSL_AD_NO_RENEGOTIATION;
-      OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
-      goto f_err;
-    }
-
-    /* This must be a HelloRequest, possibly fragmented over multiple records.
-     * Consume data from the handshake protocol until it is complete. */
-    static const uint8_t kHelloRequest[] = {SSL3_MT_HELLO_REQUEST, 0, 0, 0};
-    while (ssl->s3->hello_request_len < sizeof(kHelloRequest)) {
-      if (rr->length == 0) {
-        /* Get a new record. */
-        goto start;
-      }
-      if (rr->data[0] != kHelloRequest[ssl->s3->hello_request_len]) {
-        al = SSL_AD_DECODE_ERROR;
-        OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HELLO_REQUEST);
-        goto f_err;
-      }
-      rr->data++;
-      rr->length--;
-      ssl->s3->hello_request_len++;
-    }
-    ssl->s3->hello_request_len = 0;
-
-    ssl_do_msg_callback(ssl, 0 /* read */, ssl->version, SSL3_RT_HANDSHAKE,
-                        kHelloRequest, sizeof(kHelloRequest));
-
-    if (ssl->renegotiate_mode == ssl_renegotiate_ignore) {
-      goto start;
-    }
-
-    /* Renegotiation is only supported at quiescent points in the application
-     * protocol, namely in HTTPS, just before reading the HTTP response. Require
-     * the record-layer be idle and avoid complexities of sending a handshake
-     * record while an application_data record is being written. */
-    if (ssl_write_buffer_is_pending(ssl)) {
-      al = SSL_AD_NO_RENEGOTIATION;
-      OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
-      goto f_err;
+    if (rr->type != SSL3_RT_HANDSHAKE) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+      return -1;
     }
 
-    /* Begin a new handshake. */
-    ssl->s3->total_renegotiations++;
-    ssl->state = SSL_ST_CONNECT;
-    i = ssl->handshake_func(ssl);
-    if (i < 0) {
-      return i;
-    }
-    if (i == 0) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_HANDSHAKE_FAILURE);
-      return -1;
+    if (rr->length != 0) {
+      return consume_record(ssl, buf, len, 0 /* consume data */);
     }
 
-    /* The handshake completed synchronously. Continue reading records. */
-    goto start;
+    /* Discard empty records and loop again. */
   }
-
-  al = SSL_AD_UNEXPECTED_MESSAGE;
-  OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
-
-f_err:
-  ssl3_send_alert(ssl, SSL3_AL_FATAL, al);
-  return -1;
 }
 
 int ssl3_send_alert(SSL *ssl, int level, int desc) {
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 71505ebf..4da52332 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -620,6 +620,66 @@ int SSL_accept(SSL *ssl) {
   return SSL_do_handshake(ssl);
 }
 
+static int ssl_do_renegotiate(SSL *ssl) {
+  /* We do not accept renegotiations as a server. */
+  if (ssl->server) {
+    goto no_renegotiation;
+  }
+
+  if (ssl->s3->tmp.message_type != SSL3_MT_HELLO_REQUEST ||
+      ssl->init_num != 0) {
+    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HELLO_REQUEST);
+    return 0;
+  }
+
+  switch (ssl->renegotiate_mode) {
+    case ssl_renegotiate_ignore:
+      /* Ignore the HelloRequest. */
+      return 1;
+
+    case ssl_renegotiate_once:
+      if (ssl->s3->total_renegotiations != 0) {
+        goto no_renegotiation;
+      }
+      break;
+
+    case ssl_renegotiate_never:
+      goto no_renegotiation;
+
+    case ssl_renegotiate_freely:
+      break;
+  }
+
+  /* Renegotiation is only supported at quiescent points in the application
+   * protocol, namely in HTTPS, just before reading the HTTP response. Require
+   * the record-layer be idle and avoid complexities of sending a handshake
+   * record while an application_data record is being written. */
+  if (ssl_write_buffer_is_pending(ssl)) {
+    goto no_renegotiation;
+  }
+
+  /* Begin a new handshake. */
+  ssl->s3->total_renegotiations++;
+  ssl->state = SSL_ST_CONNECT;
+  return 1;
+
+no_renegotiation:
+  ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
+  OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+  return 0;
+}
+
+static int ssl_do_post_handshake(SSL *ssl) {
+  if (ssl3_protocol_version(ssl) < TLS1_3_VERSION) {
+    return ssl_do_renegotiate(ssl);
+  }
+
+  /* TODO(svaldez): Handle TLS 1.3 post-handshake messages. For now,
+   * silently drop them. */
+  return 1;
+}
+
 static int ssl_read_impl(SSL *ssl, void *buf, int num, int peek) {
   ssl->rwstate = SSL_NOTHING;
   /* Functions which use SSL_get_error must clear the error queue on entry. */
@@ -631,21 +691,33 @@ static int ssl_read_impl(SSL *ssl, void *buf, int num, int peek) {
     return -1;
   }
 
-  /* This may require multiple iterations. False Start will cause
-   * |ssl->handshake_func| to signal success one step early, but the handshake
-   * must be completely finished before other modes are accepted. */
-  while (SSL_in_init(ssl)) {
-    int ret = SSL_do_handshake(ssl);
-    if (ret < 0) {
+  for (;;) {
+    /* Complete the current handshake, if any. False Start will cause
+     * |SSL_do_handshake| to return mid-handshake, so this may require multiple
+     * iterations. */
+    while (SSL_in_init(ssl)) {
+      int ret = SSL_do_handshake(ssl);
+      if (ret < 0) {
+        return ret;
+      }
+      if (ret == 0) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_HANDSHAKE_FAILURE);
+        return -1;
+      }
+    }
+
+    int got_handshake;
+    int ret = ssl->method->read_app_data(ssl, &got_handshake, buf, num, peek);
+    if (ret > 0 || !got_handshake) {
       return ret;
     }
-    if (ret == 0) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_HANDSHAKE_FAILURE);
+
+    /* Handle the post-handshake message and try again. */
+    if (!ssl_do_post_handshake(ssl)) {
       return -1;
     }
+    ssl->method->release_current_message(ssl, 1 /* free buffer */);
   }
-
-  return ssl->method->read_app_data(ssl, buf, num, peek);
 }
 
 int SSL_read(SSL *ssl, void *buf, int num) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5c70e912..8815f8f4 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2067,7 +2067,7 @@ func addBasicTests() {
 				"-expect-total-renegotiations", "1",
 			},
 			shouldFail:    true,
-			expectedError: ":BAD_HELLO_REQUEST:",
+			expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
 		},
 		{
 			name:        "BadHelloRequest-2",