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>
This commit is contained in:
David Benjamin 2015-06-06 03:04:39 -04:00 committed by Adam Langley
parent 58084affbe
commit a8ebe2261f
4 changed files with 40 additions and 9 deletions

View File

@ -420,6 +420,9 @@ typedef struct ssl3_state_st {
int total_renegotiations; 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. /* State pertaining to the pending handshake.
* *
* TODO(davidben): State is current spread all over the place. Move * TODO(davidben): State is current spread all over the place. Move

View File

@ -252,11 +252,11 @@ int ssl3_read_n(SSL *s, int n, int extend) {
return n; return n;
} }
/* MAX_EMPTY_RECORDS defines the number of consecutive, empty records that will /* kMaxEmptyRecords is the number of consecutive, empty records that will be
* be processed per call to ssl3_get_record. Without this limit an attacker * processed. Without this limit an attacker could send empty records at a
* could send empty records at a faster rate than we can process and cause * faster rate than we can process and cause |ssl3_get_record| to loop
* ssl3_get_record to loop forever. */ * forever. */
#define MAX_EMPTY_RECORDS 32 static const uint8_t kMaxEmptyRecords = 32;
/* Call this to get a new input record. It will return <= 0 if more data is /* 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 * needed, normally due to an error or non-blocking IO. When it finishes, one
@ -272,7 +272,6 @@ static int ssl3_get_record(SSL *s) {
uint8_t *p; uint8_t *p;
uint16_t version; uint16_t version;
size_t extra; size_t extra;
unsigned empty_record_count = 0;
again: again:
/* check if we have the header */ /* check if we have the header */
@ -381,14 +380,15 @@ again:
/* just read a 0 length packet */ /* just read a 0 length packet */
if (rr->length == 0) { if (rr->length == 0) {
empty_record_count++; s->s3->empty_record_count++;
if (empty_record_count > MAX_EMPTY_RECORDS) { if (s->s3->empty_record_count > kMaxEmptyRecords) {
al = SSL_AD_UNEXPECTED_MESSAGE; al = SSL_AD_UNEXPECTED_MESSAGE;
OPENSSL_PUT_ERROR(SSL, ssl3_get_record, SSL_R_TOO_MANY_EMPTY_FRAGMENTS); OPENSSL_PUT_ERROR(SSL, ssl3_get_record, SSL_R_TOO_MANY_EMPTY_FRAGMENTS);
goto f_err; goto f_err;
} }
goto again; goto again;
} }
s->s3->empty_record_count = 0;
return 1; return 1;

View File

@ -856,7 +856,7 @@ func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
b := c.out.newBlock() b := c.out.newBlock()
first := true first := true
isClientHello := typ == recordTypeHandshake && len(data) > 0 && data[0] == typeClientHello isClientHello := typ == recordTypeHandshake && len(data) > 0 && data[0] == typeClientHello
for len(data) > 0 { for len(data) > 0 || first {
m := len(data) m := len(data)
if m > maxPlaintext { if m > maxPlaintext {
m = maxPlaintext m = maxPlaintext

View File

@ -204,6 +204,9 @@ type testCase struct {
// testTLSUnique, if true, causes the shim to send the tls-unique value // testTLSUnique, if true, causes the shim to send the tls-unique value
// which will be compared against the expected value. // which will be compared against the expected value.
testTLSUnique bool testTLSUnique bool
// sendEmptyRecords is the number of consecutive empty records to send
// before and after the test message.
sendEmptyRecords int
} }
var testCases = []testCase{ var testCases = []testCase{
@ -1136,6 +1139,23 @@ var testCases = []testCase{
shouldFail: true, shouldFail: true,
expectedError: ":NO_SHARED_CIPHER:", 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 { func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, isResume bool) error {
@ -1271,6 +1291,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, i
} }
} }
for i := 0; i < test.sendEmptyRecords; i++ {
tlsConn.Write(nil)
}
if test.renegotiate { if test.renegotiate {
if test.renegotiateCiphers != nil { if test.renegotiateCiphers != nil {
config.CipherSuites = test.renegotiateCiphers config.CipherSuites = test.renegotiateCiphers
@ -1306,6 +1330,10 @@ func doExchange(test *testCase, config *Config, conn net.Conn, messageLen int, i
} }
tlsConn.Write(testMessage) tlsConn.Write(testMessage)
for i := 0; i < test.sendEmptyRecords; i++ {
tlsConn.Write(nil)
}
buf := make([]byte, len(testMessage)) buf := make([]byte, len(testMessage))
if test.protocol == dtls { if test.protocol == dtls {
bufTmp := make([]byte, len(buf)+1) bufTmp := make([]byte, len(buf)+1)