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,