Don't call ssl3_read_message from ssl3_read_app_data.

With this change, it should now always be the case that rr->length is
zero on entry to ssl3_read_message. This will let us detach everything
but application data from rr. This pushes some init_buf invariants down
into tls_open_record so we don't need to maintain them everywhere.

Change-Id: I206747434e0a9603eea7d19664734fd16fa2de8e
Reviewed-on: https://boringssl-review.googlesource.com/21524
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>

ssl/internal.h

@@ -1001,6 +1001,10 @@ size_t ssl_max_handshake_message_len(const SSL *ssl);
 // dtls_clear_incoming_messages releases all buffered incoming messages.
 void dtls_clear_incoming_messages(SSL *ssl);
 
+// tls_can_accept_handshake_data returns whether |ssl| is able to accept more
+// data into handshake buffer.
+bool tls_can_accept_handshake_data(const SSL *ssl, uint8_t *out_alert);
+
 // tls_has_unprocessed_handshake_data returns whether there is buffered
 // handshake data that has not been consumed by |get_message|.
 bool tls_has_unprocessed_handshake_data(const SSL *ssl);

ssl/s3_both.cc

@@ -464,6 +464,26 @@ bool ssl3_get_message(SSL *ssl, SSLMessage *out) {
   return true;
 }
 
+bool tls_can_accept_handshake_data(const SSL *ssl, uint8_t *out_alert) {
+  // If there is a complete message, the caller must have consumed it first.
+  SSLMessage msg;
+  size_t bytes_needed;
+  if (parse_message(ssl, &msg, &bytes_needed)) {
+    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+    *out_alert = SSL_AD_INTERNAL_ERROR;
+    return false;
+  }
+
+  // Enforce the limit so the peer cannot force us to buffer 16MB.
+  if (bytes_needed > 4 + ssl_max_handshake_message_len(ssl)) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE);
+    *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+    return false;
+  }
+
+  return true;
+}
+
 bool tls_has_unprocessed_handshake_data(const SSL *ssl) {
   size_t msg_len = 0;
   if (ssl->s3->has_message) {
@@ -478,20 +498,6 @@ bool tls_has_unprocessed_handshake_data(const SSL *ssl) {
 }
 
 int ssl3_read_message(SSL *ssl) {
-  SSLMessage msg;
-  size_t bytes_needed;
-  if (parse_message(ssl, &msg, &bytes_needed)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return -1;
-  }
-
-  // Enforce the limit so the peer cannot force us to buffer 16MB.
-  if (bytes_needed > 4 + ssl_max_handshake_message_len(ssl)) {
-    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
-    OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE);
-    return -1;
-  }
-
   // Re-create the handshake buffer if needed.
   if (ssl->init_buf == NULL) {
     ssl->init_buf = BUF_MEM_new();

ssl/s3_pkt.cc

@@ -365,22 +365,20 @@ int ssl3_read_app_data(SSL *ssl, bool *out_got_handshake, uint8_t *buf, int len,
   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) {
+    if (rr->length == 0) {
       int ret = ssl3_get_record(ssl);
       if (ret <= 0) {
         return ret;
       }
     }
 
-    if (has_hs_data || rr->type == SSL3_RT_HANDSHAKE) {
+    const bool is_early_data_read = ssl->server && SSL_in_early_data(ssl);
+
+    if (rr->type == SSL3_RT_HANDSHAKE) {
       // If reading 0-RTT data, reject handshake data. 0-RTT data is terminated
       // by an alert.
-      if (SSL_in_init(ssl)) {
+      if (is_early_data_read) {
         OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
         ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
         return -1;
@@ -395,20 +393,19 @@ int ssl3_read_app_data(SSL *ssl, bool *out_got_handshake, uint8_t *buf, int len,
         return -1;
       }
 
-      // Parse post-handshake handshake messages.
-      int ret = ssl3_read_message(ssl);
-      if (ret <= 0) {
-        return ret;
+      if (ssl->init_buf == NULL) {
+        ssl->init_buf = BUF_MEM_new();
+      }
+      if (ssl->init_buf == NULL ||
+          !BUF_MEM_append(ssl->init_buf, rr->data, rr->length)) {
+        return -1;
       }
       *out_got_handshake = true;
+      rr->length = 0;
+      ssl_read_buffer_discard(ssl);
       return -1;
     }
 
-    const int is_early_data_read = ssl->server &&
-                                   ssl->s3->hs != NULL &&
-                                   ssl->s3->hs->can_early_read &&
-                                   ssl_protocol_version(ssl) >= TLS1_3_VERSION;
-
     // Handle the end_of_early_data alert.
     if (rr->type == SSL3_RT_ALERT &&
         rr->length == 2 &&
@@ -457,8 +454,7 @@ int ssl3_read_change_cipher_spec(SSL *ssl) {
     }
   }
 
-  if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC ||
-      tls_has_unprocessed_handshake_data(ssl)) {
+  if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC) {
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
     OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
     return -1;

ssl/tls_record.cc

@@ -192,6 +192,12 @@ static enum ssl_open_record_t do_tls_open_record(SSL *ssl, uint8_t *out_type,
                                                  size_t *out_consumed,
                                                  uint8_t *out_alert,
                                                  Span<uint8_t> in) {
+  // If there is an unprocessed handshake message or we are already buffering
+  // too much, stop before decrypting another handshake record.
+  if (!tls_can_accept_handshake_data(ssl, out_alert)) {
+    return ssl_open_record_error;
+  }
+
   CBS cbs = CBS(in);
 
   // Decode the record header.
@@ -321,6 +327,14 @@ static enum ssl_open_record_t do_tls_open_record(SSL *ssl, uint8_t *out_type,
     return ssl_process_alert(ssl, out_alert, *out);
   }
 
+  // Handshake messages may not interleave with any other record type.
+  if (type != SSL3_RT_HANDSHAKE &&
+      tls_has_unprocessed_handshake_data(ssl)) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+    *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
+    return ssl_open_record_error;
+  }
+
   ssl->s3->warning_alert_count = 0;
 
   *out_type = type;