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>
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 6681490..81e9b0a 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -154,15 +154,6 @@
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 @@
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 d3e3192..940494a 100644
--- a/ssl/dtls_record.c
+++ b/ssl/dtls_record.c
@@ -236,6 +236,9 @@
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 5bddf98..d4eb7ef 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 @@
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 @@
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 6c10992..f841293 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -639,7 +639,11 @@
// 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 b755c46..cf6700d 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1287,6 +1287,9 @@
})
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 6c774eb..950c02a 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1308,6 +1308,48 @@
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 @@
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 @@
// 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 ae75495..5fd9792 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 @@
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);
