Reject empty records of unexpected type.

The old empty record logic discarded the records at a very low-level.
Let the error bubble up to ssl3_read_bytes so the type mismatch logic
may kick in before the empty record is skipped.

Add tests for when the record in question is application data, before
before the handshake and post ChangeCipherSpec.

BUG=521840

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

7 files changed, 104 insertions, 30 deletions

diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 6681490e..81e9b0a7 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -154,15 +154,6 @@ again:
     case ssl_open_record_success:
       ssl_read_buffer_consume(ssl, consumed);
 
-      /* Discard empty records.
-       * TODO(davidben): This logic should be moved to a higher level. See
-       * https://crbug.com/521840.
-       * TODO(davidben): Limit the number of empty records as in TLS? This is
-       * useful if we also limit discarded packets. */
-      if (len == 0) {
-        goto again;
-      }
-
       if (len > 0xffff) {
         OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
         return -1;
@@ -316,6 +307,11 @@ start:
       goto f_err;
     }
 
+    /* Discard empty records. */
+    if (rr->length == 0) {
+      goto start;
+    }
+
     if (len <= 0) {
       return len;
     }
diff --git a/ssl/dtls_record.c b/ssl/dtls_record.c
index d3e3192b..940494a1 100644
--- a/ssl/dtls_record.c
+++ b/ssl/dtls_record.c
@@ -236,6 +236,9 @@ enum ssl_open_record_t dtls_open_record(
 
   dtls1_bitmap_record(&ssl->d1->bitmap, sequence);
 
+  /* TODO(davidben): Limit the number of empty records as in TLS? This is only
+   * useful if we also limit discarded packets. */
+
   *out_type = type;
   *out_len = plaintext_len;
   *out_consumed = in_len - CBS_len(&cbs);
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 5bddf980..d4eb7ef9 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -122,12 +122,6 @@
 
 static int do_ssl3_write(SSL *s, int type, const uint8_t *buf, unsigned len);
 
-/* kMaxEmptyRecords is the number of consecutive, empty records that will be
- * processed. Without this limit an attacker could send empty records at a
- * faster rate than we can process and cause record processing to loop
- * forever. */
-static const uint8_t kMaxEmptyRecords = 32;
-
 /* kMaxWarningAlerts is the number of consecutive warning alerts that will be
  * processed. */
 static const uint8_t kMaxWarningAlerts = 4;
@@ -154,20 +148,6 @@ again:
     case ssl_open_record_success:
       ssl_read_buffer_consume(ssl, consumed);
 
-      /* Discard empty records.
-       * TODO(davidben): This logic should be moved to a higher level. See
-       * https://crbug.com/521840. */
-      if (len == 0) {
-        ssl->s3->empty_record_count++;
-        if (ssl->s3->empty_record_count > kMaxEmptyRecords) {
-          OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MANY_EMPTY_FRAGMENTS);
-          ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
-          return -1;
-        }
-        goto again;
-      }
-      ssl->s3->empty_record_count = 0;
-
       if (len > 0xffff) {
         OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
         return -1;
@@ -493,6 +473,11 @@ start:
       goto f_err;
     }
 
+    /* Discard empty records. */
+    if (rr->length == 0) {
+      goto start;
+    }
+
     if (len <= 0) {
       return len;
     }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 6c109929..f8412939 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -639,7 +639,11 @@ type ProtocolBugs struct {
 	// the server believes it has actually negotiated.
 	SendCipherSuite uint16
 
-	// AppDataAfterChangeCipherSpec, if not null, causes application data to
+	// AppDataBeforeHandshake, if not nil, causes application data to be
+	// sent immediately before the first handshake message.
+	AppDataBeforeHandshake []byte
+
+	// AppDataAfterChangeCipherSpec, if not nil, causes application data to
 	// be sent immediately after ChangeCipherSpec.
 	AppDataAfterChangeCipherSpec []byte
 
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index b755c467..cf6700d6 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1287,6 +1287,9 @@ func (c *Conn) Handshake() error {
 		})
 		c.conn.Write([]byte{alertLevelError, byte(alertInternalError)})
 	}
+	if data := c.config.Bugs.AppDataBeforeHandshake; data != nil {
+		c.writeRecord(recordTypeApplicationData, data)
+	}
 	if c.isClient {
 		c.handshakeErr = c.clientHandshake()
 	} else {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 6c774ebb..950c02ae 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1308,6 +1308,48 @@ func addBasicTests() {
 			flags:            []string{"-async"},
 		},
 		{
+			name: "AppDataBeforeHandshake",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataBeforeHandshake: []byte("TEST MESSAGE"),
+				},
+			},
+			shouldFail:    true,
+			expectedError: ":UNEXPECTED_RECORD:",
+		},
+		{
+			name: "AppDataBeforeHandshake-Empty",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataBeforeHandshake: []byte{},
+				},
+			},
+			shouldFail:    true,
+			expectedError: ":UNEXPECTED_RECORD:",
+		},
+		{
+			protocol: dtls,
+			name:     "AppDataBeforeHandshake-DTLS",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataBeforeHandshake: []byte("TEST MESSAGE"),
+				},
+			},
+			shouldFail:    true,
+			expectedError: ":UNEXPECTED_RECORD:",
+		},
+		{
+			protocol: dtls,
+			name:     "AppDataBeforeHandshake-DTLS-Empty",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataBeforeHandshake: []byte{},
+				},
+			},
+			shouldFail:    true,
+			expectedError: ":UNEXPECTED_RECORD:",
+		},
+		{
 			name: "AppDataAfterChangeCipherSpec",
 			config: Config{
 				Bugs: ProtocolBugs{
@@ -1318,6 +1360,16 @@ func addBasicTests() {
 			expectedError: ":DATA_BETWEEN_CCS_AND_FINISHED:",
 		},
 		{
+			name: "AppDataAfterChangeCipherSpec-Empty",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataAfterChangeCipherSpec: []byte{},
+				},
+			},
+			shouldFail:    true,
+			expectedError: ":DATA_BETWEEN_CCS_AND_FINISHED:",
+		},
+		{
 			protocol: dtls,
 			name:     "AppDataAfterChangeCipherSpec-DTLS",
 			config: Config{
@@ -1329,6 +1381,17 @@ func addBasicTests() {
 			// application data.
 		},
 		{
+			protocol: dtls,
+			name:     "AppDataAfterChangeCipherSpec-DTLS-Empty",
+			config: Config{
+				Bugs: ProtocolBugs{
+					AppDataAfterChangeCipherSpec: []byte{},
+				},
+			},
+			// BoringSSL's DTLS implementation will drop the out-of-order
+			// application data.
+		},
+		{
 			name: "AlertAfterChangeCipherSpec",
 			config: Config{
 				Bugs: ProtocolBugs{
diff --git a/ssl/tls_record.c b/ssl/tls_record.c
index ae754952..5fd9792f 100644
--- a/ssl/tls_record.c
+++ b/ssl/tls_record.c
@@ -116,6 +116,12 @@
 #include "internal.h"
 
 
+/* kMaxEmptyRecords is the number of consecutive, empty records that will be
+ * processed. Without this limit an attacker could send empty records at a
+ * faster rate than we can process and cause record processing to loop
+ * forever. */
+static const uint8_t kMaxEmptyRecords = 32;
+
 size_t ssl_record_prefix_len(const SSL *ssl) {
   if (SSL_IS_DTLS(ssl)) {
     return DTLS1_RT_HEADER_LENGTH +
@@ -224,6 +230,20 @@ enum ssl_open_record_t tls_open_record(
     return ssl_open_record_error;
   }
 
+  /* Limit the number of consecutive empty records. */
+  if (plaintext_len == 0) {
+    ssl->s3->empty_record_count++;
+    if (ssl->s3->empty_record_count > kMaxEmptyRecords) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MANY_EMPTY_FRAGMENTS);
+      *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
+      return ssl_open_record_error;
+    }
+    /* Apart from the limit, empty records are returned up to the caller. This
+     * allows the caller to reject records of the wrong type. */
+  } else {
+    ssl->s3->empty_record_count = 0;
+  }
+
   *out_type = type;
   *out_len = plaintext_len;
   *out_consumed = in_len - CBS_len(&cbs);