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>

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;
     }

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);

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;
     }

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
 

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 {

ssl/test/runner/runner.go

@@ -1307,6 +1307,48 @@ func addBasicTests() {
 			damageFirstWrite: true,
 			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{
@@ -1317,6 +1359,16 @@ func addBasicTests() {
 			shouldFail:    true,
 			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",
@@ -1328,6 +1380,17 @@ func addBasicTests() {
 			// BoringSSL's DTLS implementation will drop the out-of-order
 			// 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{

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);