Add tests for empty record limit and make it work in the async case.
We shouldn't have protocol constraints that are sensitive to whether
data is returned synchronously or not.
Per https://boringssl-review.googlesource.com/#/c/4112/, the original
limitation was to avoid OpenSSL ABI changes. This is no longer a
concern.
Add tests for the sync and async case. Send the empty records in two
batches to ensure the count is reset correctly.
Change-Id: I3fee839438527e71adb83d437879bb0d49ca5c07
Reviewed-on: https://boringssl-review.googlesource.com/5040
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 640a228..5cc0acc 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -420,6 +420,9 @@
int total_renegotiations;
+ /* empty_record_count is the number of consecutive empty records received. */
+ uint8_t empty_record_count;
+
/* State pertaining to the pending handshake.
*
* TODO(davidben): State is current spread all over the place. Move
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 4a9ae83..9599f59 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -252,11 +252,11 @@
return n;
}
-/* MAX_EMPTY_RECORDS defines the number of consecutive, empty records that will
- * be processed per call to ssl3_get_record. Without this limit an attacker
- * could send empty records at a faster rate than we can process and cause
- * ssl3_get_record to loop forever. */
-#define MAX_EMPTY_RECORDS 32
+/* 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 |ssl3_get_record| to loop
+ * forever. */
+static const uint8_t kMaxEmptyRecords = 32;
/* Call this to get a new input record. It will return <= 0 if more data is
* needed, normally due to an error or non-blocking IO. When it finishes, one
@@ -272,7 +272,6 @@
uint8_t *p;
uint16_t version;
size_t extra;
- unsigned empty_record_count = 0;
again:
/* check if we have the header */
@@ -381,14 +380,15 @@
/* just read a 0 length packet */
if (rr->length == 0) {
- empty_record_count++;
- if (empty_record_count > MAX_EMPTY_RECORDS) {
+ s->s3->empty_record_count++;
+ if (s->s3->empty_record_count > kMaxEmptyRecords) {
al = SSL_AD_UNEXPECTED_MESSAGE;
OPENSSL_PUT_ERROR(SSL, ssl3_get_record, SSL_R_TOO_MANY_EMPTY_FRAGMENTS);
goto f_err;
}
goto again;
}
+ s->s3->empty_record_count = 0;
return 1;
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index adbc1c3..84741fb 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -856,7 +856,7 @@
b := c.out.newBlock()
first := true
isClientHello := typ == recordTypeHandshake && len(data) > 0 && data[0] == typeClientHello
- for len(data) > 0 {
+ for len(data) > 0 || first {
m := len(data)
if m > maxPlaintext {
m = maxPlaintext
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index bd03cb1..d7ae850 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -204,6 +204,9 @@
// testTLSUnique, if true, causes the shim to send the tls-unique value
// which will be compared against the expected value.
testTLSUnique bool
+ // sendEmptyRecords is the number of consecutive empty records to send
+ // before and after the test message.
+ sendEmptyRecords int
}
var testCases = []testCase{
@@ -1136,6 +1139,23 @@
shouldFail: true,
expectedError: ":NO_SHARED_CIPHER:",
},
+ {
+ name: "SendEmptyRecords-Pass",
+ sendEmptyRecords: 32,
+ },
+ {
+ name: "SendEmptyRecords",
+ sendEmptyRecords: 33,
+ shouldFail: true,
+ expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
+ },
+ {
+ name: "SendEmptyRecords-Async",
+ sendEmptyRecords: 33,
+ flags: []string{"-async"},
+ shouldFail: true,
+ expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
+ },
}
func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {
@@ -1271,6 +1291,10 @@
}
}
+ for i := 0; i < test.sendEmptyRecords; i++ {
+ tlsConn.Write(nil)
+ }
+
if test.renegotiate {
if test.renegotiateCiphers != nil {
config.CipherSuites = test.renegotiateCiphers
@@ -1306,6 +1330,10 @@
}
tlsConn.Write(testMessage)
+ for i := 0; i < test.sendEmptyRecords; i++ {
+ tlsConn.Write(nil)
+ }
+
buf := make([]byte, len(testMessage))
if test.protocol == dtls {
bufTmp := make([]byte, len(buf)+1)
