Fix TLSInnerPlaintext limit.
See https://github.com/tlswg/tls13-spec/pull/1083. We misread the
original text spec, but it turns out the original spec text required
senders have version-specific maximum send fragments. The PR fixes this
off-by-one issue. Align with the new spec text uniformly.
This is a wire format change for our existing drafts *only if* records
have padding. We don't currently send padding, so this is fine. Unpadded
records continue to be capped at 2^14 bytes of plaintext (or 2^14+1
bytes of TLSInnerPlaintext structure).
Change-Id: I01017cfd13162504bb163dd59afd74aff0896cc4
Reviewed-on: https://boringssl-review.googlesource.com/23004
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 57bc20c..0cd1e81 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2442,9 +2442,10 @@
SendLargeRecords: true,
},
},
- messageLen: maxPlaintext + 1,
- shouldFail: true,
- expectedError: ":DATA_LENGTH_TOO_LONG:",
+ messageLen: maxPlaintext + 1,
+ shouldFail: true,
+ expectedError: ":DATA_LENGTH_TOO_LONG:",
+ expectedLocalError: "remote error: record overflow",
},
{
protocol: dtls,
@@ -2454,9 +2455,64 @@
SendLargeRecords: true,
},
},
- messageLen: maxPlaintext + 1,
- shouldFail: true,
- expectedError: ":DATA_LENGTH_TOO_LONG:",
+ messageLen: maxPlaintext + 1,
+ shouldFail: true,
+ expectedError: ":DATA_LENGTH_TOO_LONG:",
+ expectedLocalError: "remote error: record overflow",
+ },
+ {
+ name: "LargePlaintext-TLS13-Padded-8192-8192",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ RecordPadding: 8192,
+ SendLargeRecords: true,
+ },
+ },
+ messageLen: 8192,
+ },
+ {
+ name: "LargePlaintext-TLS13-Padded-8193-8192",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ RecordPadding: 8193,
+ SendLargeRecords: true,
+ },
+ },
+ messageLen: 8192,
+ shouldFail: true,
+ expectedError: ":DATA_LENGTH_TOO_LONG:",
+ expectedLocalError: "remote error: record overflow",
+ },
+ {
+ name: "LargePlaintext-TLS13-Padded-16383-1",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ RecordPadding: 1,
+ SendLargeRecords: true,
+ },
+ },
+ messageLen: 16383,
+ },
+ {
+ name: "LargePlaintext-TLS13-Padded-16384-1",
+ config: Config{
+ MinVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ RecordPadding: 1,
+ SendLargeRecords: true,
+ },
+ },
+ messageLen: 16384,
+ shouldFail: true,
+ expectedError: ":DATA_LENGTH_TOO_LONG:",
+ expectedLocalError: "remote error: record overflow",
},
{
name: "LargeCiphertext",
@@ -11774,7 +11830,7 @@
testCases = append(testCases, testCase{
testType: clientTest,
- name: "TLS13Draft22-SkipChangeCipherSpec-Client",
+ name: "TLS13Draft22-SkipChangeCipherSpec-Client",
config: Config{
MaxVersion: VersionTLS13,
Bugs: ProtocolBugs{
@@ -11786,7 +11842,7 @@
testCases = append(testCases, testCase{
testType: serverTest,
- name: "TLS13Draft22-SkipChangeCipherSpec-Server",
+ name: "TLS13Draft22-SkipChangeCipherSpec-Server",
config: Config{
MaxVersion: VersionTLS13,
Bugs: ProtocolBugs{
@@ -11798,30 +11854,30 @@
testCases = append(testCases, testCase{
testType: clientTest,
- name: "TLS13Draft22-TooManyChangeCipherSpec-Client",
+ name: "TLS13Draft22-TooManyChangeCipherSpec-Client",
config: Config{
MaxVersion: VersionTLS13,
Bugs: ProtocolBugs{
SendExtraChangeCipherSpec: 33,
},
},
- tls13Variant: TLS13Draft22,
- shouldFail: true,
- expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
+ tls13Variant: TLS13Draft22,
+ shouldFail: true,
+ expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
})
testCases = append(testCases, testCase{
testType: serverTest,
- name: "TLS13Draft22-TooManyChangeCipherSpec-Server",
+ name: "TLS13Draft22-TooManyChangeCipherSpec-Server",
config: Config{
MaxVersion: VersionTLS13,
Bugs: ProtocolBugs{
SendExtraChangeCipherSpec: 33,
},
},
- tls13Variant: TLS13Draft22,
- shouldFail: true,
- expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
+ tls13Variant: TLS13Draft22,
+ shouldFail: true,
+ expectedError: ":TOO_MANY_EMPTY_FRAGMENTS:",
})
fooString := "foo"
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc
index 7e8e968..a1363fa 100644
--- a/ssl/tls_record.cc
+++ b/ssl/tls_record.cc
@@ -187,9 +187,25 @@
return ret;
}
-enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type,
- Span<uint8_t> *out, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in) {
+static ssl_open_record_t skip_early_data(SSL *ssl, uint8_t *out_alert,
+ size_t consumed) {
+ ssl->s3->early_data_skipped += consumed;
+ if (ssl->s3->early_data_skipped < consumed) {
+ ssl->s3->early_data_skipped = kMaxEarlyDataSkipped + 1;
+ }
+
+ if (ssl->s3->early_data_skipped > kMaxEarlyDataSkipped) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MUCH_SKIPPED_EARLY_DATA);
+ *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
+ return ssl_open_record_error;
+ }
+
+ return ssl_open_record_discard;
+}
+
+ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type,
+ Span<uint8_t> *out, size_t *out_consumed,
+ uint8_t *out_alert, Span<uint8_t> in) {
*out_consumed = 0;
if (ssl->s3->read_shutdown == ssl_shutdown_close_notify) {
return ssl_open_record_close_notify;
@@ -267,7 +283,7 @@
if (ssl->s3->skip_early_data &&
ssl->s3->aead_read_ctx->is_null_cipher() &&
type == SSL3_RT_APPLICATION_DATA) {
- goto skipped_data;
+ return skip_early_data(ssl, out_alert, *out_consumed);
}
// Decrypt the body in-place.
@@ -276,7 +292,7 @@
MakeSpan(const_cast<uint8_t *>(CBS_data(&body)), CBS_len(&body)))) {
if (ssl->s3->skip_early_data && !ssl->s3->aead_read_ctx->is_null_cipher()) {
ERR_clear_error();
- goto skipped_data;
+ return skip_early_data(ssl, out_alert, *out_consumed);
}
OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
@@ -292,8 +308,21 @@
}
// TLS 1.3 hides the record type inside the encrypted data.
- if (!ssl->s3->aead_read_ctx->is_null_cipher() &&
- ssl->s3->aead_read_ctx->ProtocolVersion() >= TLS1_3_VERSION) {
+ bool has_padding =
+ !ssl->s3->aead_read_ctx->is_null_cipher() &&
+ ssl->s3->aead_read_ctx->ProtocolVersion() >= TLS1_3_VERSION;
+
+ // If there is padding, the plaintext limit includes the padding, but includes
+ // extra room for the inner content type.
+ size_t plaintext_limit =
+ has_padding ? SSL3_RT_MAX_PLAIN_LENGTH + 1 : SSL3_RT_MAX_PLAIN_LENGTH;
+ if (out->size() > plaintext_limit) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_LENGTH_TOO_LONG);
+ *out_alert = SSL_AD_RECORD_OVERFLOW;
+ return ssl_open_record_error;
+ }
+
+ if (has_padding) {
// The outer record type is always application_data.
if (type != SSL3_RT_APPLICATION_DATA) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_OUTER_RECORD_TYPE);
@@ -312,13 +341,6 @@
} while (type == 0);
}
- // Check the plaintext length.
- if (out->size() > SSL3_RT_MAX_PLAIN_LENGTH) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_LENGTH_TOO_LONG);
- *out_alert = SSL_AD_RECORD_OVERFLOW;
- return ssl_open_record_error;
- }
-
// Limit the number of consecutive empty records.
if (out->empty()) {
ssl->s3->empty_record_count++;
@@ -358,20 +380,6 @@
*out_type = type;
return ssl_open_record_success;
-
-skipped_data:
- ssl->s3->early_data_skipped += *out_consumed;
- if (ssl->s3->early_data_skipped < *out_consumed) {
- ssl->s3->early_data_skipped = kMaxEarlyDataSkipped + 1;
- }
-
- if (ssl->s3->early_data_skipped > kMaxEarlyDataSkipped) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MUCH_SKIPPED_EARLY_DATA);
- *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
- return ssl_open_record_error;
- }
-
- return ssl_open_record_discard;
}
static int do_seal_record(SSL *ssl, uint8_t *out_prefix, uint8_t *out,
