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