Pack DTLS fragments into records when possible
We were sending at most one DTLS fragment per record, which prevents us
from amortizing the overhead. In DTLS 1.2, this doesn't impact the
encrypted epoch (just Finished), but it does make the unencrypted epoch
slightly more efficient.
In DTLS 1.3, it doesn't impact the unencrypted epochs, but the encrypted
epochs become a lot more efficient. This is the DTLS analog to
https://boringssl-review.googlesource.com/28744
As part of this, test that:
- Each handshake fragment contains as much of the message as can fit in
the packet.
- If the first record of packet N could have (partially) fit in packet
N-1, the caller should have put it in packet N-1.
- We never have two handshake records of the same epoch in a row in one
packet. If we did, those could have been merged for less overhead.
(That's what's fixed by this CL.)
As part of this, we can remove the MTUExceeded tests. Those were, badly,
testing that the peer successfully filled at least one packet. Now we
test packet-filling more directly.
Finally, although we don't particularly care about CBC ciphers, I've
made the packet-filling code more accurate for CBC ciphers. This is
purely so that we don't need as many special cases on the runner side
when deciding if there was more room available in the packet.
Bug: 42290594
Change-Id: Ia79b2a09d3008e95de08121fc4e35768c6f3ae77
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72275
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index c460e14..0b3711f 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -680,138 +680,159 @@
enum seal_result_t {
seal_error,
- seal_no_progress,
- seal_partial,
- seal_success,
+ seal_continue,
+ seal_flush,
};
-// seal_next_message seals |msg|, which must be the next message, to |out|. If
-// progress was made, it returns |seal_partial| or |seal_success| and sets
+// seal_next_record seals one record's worth of messages to |out| and advances
+// |ssl|'s internal state past the data that was sealed. If progress was made,
+// it returns |seal_flush| or |seal_continue| and sets
// |*out_len| to the number of bytes written.
-static enum seal_result_t seal_next_message(SSL *ssl, Span<uint8_t> out,
- size_t *out_len,
- const DTLSOutgoingMessage *msg) {
+//
+// If the function stopped because the next message could not be combined into
+// this record, it returns |seal_continue| and the caller should loop again.
+// Otherwise, it returns |seal_flush| and the packet is complete.
+static seal_result_t seal_next_record(SSL *ssl, Span<uint8_t> out,
+ size_t *out_len) {
assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size());
- assert(msg == &ssl->d1->outgoing_messages[ssl->d1->outgoing_written]);
+ const auto &first_msg = ssl->d1->outgoing_messages[ssl->d1->outgoing_written];
+ size_t prefix_len = dtls_seal_prefix_len(ssl, first_msg.epoch);
+ size_t max_in_len = dtls_seal_max_input_len(ssl, first_msg.epoch, out.size());
+ *out_len = 0;
- size_t overhead = dtls_max_seal_overhead(ssl, msg->epoch);
- size_t prefix = dtls_seal_prefix_len(ssl, msg->epoch);
+ if (max_in_len == 0) {
+ // There is no room for a single record.
+ return seal_flush;
+ }
- if (msg->is_ccs) {
- // Check there is room for the ChangeCipherSpec.
+ if (first_msg.is_ccs) {
static const uint8_t kChangeCipherSpec[1] = {SSL3_MT_CCS};
- if (out.size() < sizeof(kChangeCipherSpec) + overhead) {
- return seal_no_progress;
- }
-
DTLSRecordNumber record_number;
if (!dtls_seal_record(ssl, &record_number, out.data(), out_len, out.size(),
SSL3_RT_CHANGE_CIPHER_SPEC, kChangeCipherSpec,
- sizeof(kChangeCipherSpec), msg->epoch)) {
+ sizeof(kChangeCipherSpec), first_msg.epoch)) {
return seal_error;
}
- ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_CHANGE_CIPHER_SPEC,
+ ssl_do_msg_callback(ssl, /*is_write=*/1, SSL3_RT_CHANGE_CIPHER_SPEC,
kChangeCipherSpec);
- return seal_success;
+ ssl->d1->outgoing_offset = 0;
+ ssl->d1->outgoing_written++;
+ return seal_continue;
}
- // DTLS messages are serialized as a single fragment in |msg|.
- CBS cbs(msg->data), body;
- struct hm_header_st hdr;
- if (!dtls1_parse_fragment(&cbs, &hdr, &body) || //
- hdr.frag_off != 0 || //
- hdr.frag_len != CBS_len(&body) || //
- hdr.msg_len != CBS_len(&body) ||
- !CBS_skip(&body, ssl->d1->outgoing_offset) || //
- CBS_len(&cbs) != 0) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return seal_error;
+ // Pack as many handshake fragments into one record as we can. We stage the
+ // fragments in the output buffer, to be sealed in-place.
+ bool should_continue = false;
+ Span<uint8_t> fragments = out.subspan(prefix_len, max_in_len);
+ CBB cbb;
+ CBB_init_fixed(&cbb, fragments.data(), fragments.size());
+ while (ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size()) {
+ const auto &msg = ssl->d1->outgoing_messages[ssl->d1->outgoing_written];
+ if (msg.epoch != first_msg.epoch || msg.is_ccs) {
+ // We can only pack messages if the epoch matches. There may be more room
+ // in the packet, so tell the caller to keep going.
+ should_continue = true;
+ break;
+ }
+
+ // Decode |msg|'s header.
+ CBS cbs(msg.data), body;
+ struct hm_header_st hdr;
+ if (!dtls1_parse_fragment(&cbs, &hdr, &body) || //
+ hdr.frag_off != 0 || //
+ hdr.frag_len != CBS_len(&body) || //
+ hdr.msg_len != CBS_len(&body) ||
+ !CBS_skip(&body, ssl->d1->outgoing_offset) || //
+ CBS_len(&cbs) != 0) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ return seal_error;
+ }
+
+ // Determine how much progress can be made.
+ size_t capacity = fragments.size() - CBB_len(&cbb);
+ if (capacity < DTLS1_HM_HEADER_LENGTH + 1) {
+ // We could not fit even 1 byte.
+ break;
+ }
+ size_t todo = std::min(CBS_len(&body), capacity - DTLS1_HM_HEADER_LENGTH);
+
+ // Assemble the fragment.
+ size_t frag_start = CBB_len(&cbb);
+ CBB child;
+ if (!CBB_add_u8(&cbb, hdr.type) || //
+ !CBB_add_u24(&cbb, hdr.msg_len) || //
+ !CBB_add_u16(&cbb, hdr.seq) ||
+ !CBB_add_u24(&cbb, ssl->d1->outgoing_offset) ||
+ !CBB_add_u24_length_prefixed(&cbb, &child) ||
+ !CBB_add_bytes(&child, CBS_data(&body), todo) || //
+ !CBB_flush(&cbb)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ return seal_error;
+ }
+ size_t frag_end = CBB_len(&cbb);
+
+ // TODO(davidben): It is odd that, on output, we inform the caller of
+ // retransmits and individual fragments, but on input we only inform the
+ // caller of complete messages.
+ ssl_do_msg_callback(ssl, /*is_write=*/1, SSL3_RT_HANDSHAKE,
+ fragments.subspan(frag_start, frag_end - frag_start));
+
+ if (todo < CBS_len(&body)) {
+ // The packet was the limiting factor. Save the offset for the next packet
+ // and stop.
+ ssl->d1->outgoing_offset += todo;
+ break;
+ }
+
+ // There is still room. Continue to the next message.
+ ssl->d1->outgoing_offset = 0;
+ ssl->d1->outgoing_written++;
}
- // Determine how much progress can be made.
- if (out.size() < DTLS1_HM_HEADER_LENGTH + 1 + overhead ||
- out.size() < prefix) {
- return seal_no_progress;
+ // We could not fit anything. Don't try to make a record.
+ if (CBB_len(&cbb) == 0) {
+ assert(!should_continue);
+ return seal_flush;
}
- size_t todo = CBS_len(&body);
- if (todo > out.size() - DTLS1_HM_HEADER_LENGTH - overhead) {
- todo = out.size() - DTLS1_HM_HEADER_LENGTH - overhead;
- }
-
- // Assemble a fragment, to be sealed in-place.
- ScopedCBB cbb;
- CBB child;
- Span<uint8_t> frag = out.subspan(prefix);
- size_t frag_len;
- if (!CBB_init_fixed(cbb.get(), frag.data(), frag.size()) ||
- !CBB_add_u8(cbb.get(), hdr.type) ||
- !CBB_add_u24(cbb.get(), hdr.msg_len) ||
- !CBB_add_u16(cbb.get(), hdr.seq) ||
- !CBB_add_u24(cbb.get(), ssl->d1->outgoing_offset) ||
- !CBB_add_u24_length_prefixed(cbb.get(), &child) ||
- !CBB_add_bytes(&child, CBS_data(&body), todo) ||
- !CBB_finish(cbb.get(), NULL, &frag_len)) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return seal_error;
- }
-
- frag = frag.first(frag_len);
- ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HANDSHAKE, frag);
DTLSRecordNumber record_number;
if (!dtls_seal_record(ssl, &record_number, out.data(), out_len, out.size(),
- SSL3_RT_HANDSHAKE, frag.data(), frag.size(),
- msg->epoch)) {
+ SSL3_RT_HANDSHAKE, CBB_data(&cbb), CBB_len(&cbb),
+ first_msg.epoch)) {
return seal_error;
}
- if (todo == CBS_len(&body)) {
- // The next message is complete.
- ssl->d1->outgoing_offset = 0;
- return seal_success;
- }
-
- ssl->d1->outgoing_offset += todo;
- return seal_partial;
+ return should_continue ? seal_continue : seal_flush;
}
// seal_next_packet writes as much of the next flight as possible to |out| and
// advances |ssl->d1->outgoing_written| and |ssl->d1->outgoing_offset| as
// appropriate.
static bool seal_next_packet(SSL *ssl, Span<uint8_t> out, size_t *out_len) {
- bool made_progress = false;
size_t total = 0;
assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size());
- for (; ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size();
- ssl->d1->outgoing_written++) {
- const DTLSOutgoingMessage *msg =
- &ssl->d1->outgoing_messages[ssl->d1->outgoing_written];
+ while (ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size()) {
size_t len;
- enum seal_result_t ret = seal_next_message(ssl, out, &len, msg);
+ seal_result_t ret = seal_next_record(ssl, out, &len);
switch (ret) {
case seal_error:
return false;
- case seal_no_progress:
- goto packet_full;
-
- case seal_partial:
- case seal_success:
+ case seal_flush:
+ case seal_continue:
out = out.subspan(len);
total += len;
- made_progress = true;
-
- if (ret == seal_partial) {
- goto packet_full;
- }
break;
}
+
+ if (ret == seal_flush) {
+ break;
+ }
}
-packet_full:
// The MTU was too small to make any progress.
- if (!made_progress) {
+ if (total == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_MTU_TOO_SMALL);
return false;
}
diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc
index 586b852..8a17ee2 100644
--- a/ssl/dtls_record.cc
+++ b/ssl/dtls_record.cc
@@ -490,6 +490,24 @@
write_epoch->aead->ExplicitNonceLen();
}
+size_t dtls_seal_max_input_len(const SSL *ssl, uint16_t epoch, size_t max_out) {
+ DTLSWriteEpoch *write_epoch = get_write_epoch(ssl, epoch);
+ if (write_epoch == nullptr) {
+ return 0;
+ }
+ size_t header_len = dtls_record_header_write_len(ssl, epoch);
+ if (max_out <= header_len) {
+ return 0;
+ }
+ max_out -= header_len;
+ max_out = write_epoch->aead->MaxSealInputLen(max_out);
+ if (max_out > 0 && use_dtls13_record_header(ssl, epoch)) {
+ // Remove 1 byte for the encrypted record type.
+ max_out--;
+ }
+ return max_out;
+}
+
bool dtls_seal_record(SSL *ssl, DTLSRecordNumber *out_number, uint8_t *out,
size_t *out_len, size_t max_out, uint8_t type,
const uint8_t *in, size_t in_len, uint16_t epoch) {
diff --git a/ssl/internal.h b/ssl/internal.h
index f2b8b93..15634d6 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1151,6 +1151,10 @@
// MaxOverhead returns the maximum overhead of calling |Seal|.
size_t MaxOverhead() const;
+ // MaxSealInputLen returns the maximum length for |Seal| that can fit in
+ // |max_out| output bytes, or zero if no input may fit.
+ size_t MaxSealInputLen(size_t max_out) const;
+
// SuffixLen calculates the suffix length written by |SealScatter| and writes
// it to |*out_suffix_len|. It returns true on success and false on error.
// |in_len| and |extra_in_len| should equal the argument of the same names
@@ -1426,6 +1430,10 @@
// front of the plaintext when sealing a record in-place.
size_t dtls_seal_prefix_len(const SSL *ssl, uint16_t epoch);
+// dtls_seal_max_input_len returns the maximum number of input bytes that can
+// fit in a record of up to |max_out| bytes, or zero if none may fit.
+size_t dtls_seal_max_input_len(const SSL *ssl, uint16_t epoch, size_t max_out);
+
// dtls_seal_record implements |tls_seal_record| for DTLS. |epoch| selects which
// epoch's cipher state to use. Unlike |tls_seal_record|, |in| and |out| may
// alias but, if they do, |in| must be exactly |dtls_seal_prefix_len| bytes
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc
index b0e298c..8110725 100644
--- a/ssl/s3_both.cc
+++ b/ssl/s3_both.cc
@@ -192,7 +192,7 @@
// cipher. The benefit is smaller and there is a risk of breaking buggy
// implementations.
//
- // TODO(davidben): See if we can do this uniformly.
+ // TODO(crbug.com/374991962): See if we can do this uniformly.
Span<const uint8_t> rest = msg;
if (ssl->quic_method == nullptr &&
ssl->s3->aead_write_ctx->is_null_cipher()) {
diff --git a/ssl/ssl_aead_ctx.cc b/ssl/ssl_aead_ctx.cc
index 2db919f..7a2dda1 100644
--- a/ssl/ssl_aead_ctx.cc
+++ b/ssl/ssl_aead_ctx.cc
@@ -173,6 +173,43 @@
: EVP_AEAD_max_overhead(EVP_AEAD_CTX_aead(ctx_.get())));
}
+size_t SSLAEADContext::MaxSealInputLen(size_t max_out) const {
+ size_t explicit_nonce_len = ExplicitNonceLen();
+ if (max_out <= explicit_nonce_len) {
+ return 0;
+ }
+ max_out -= explicit_nonce_len;
+ if (is_null_cipher() || FUZZER_MODE) {
+ return max_out;
+ }
+ // TODO(crbug.com/42290602): This should be part of |EVP_AEAD_CTX|.
+ size_t overhead = EVP_AEAD_max_overhead(EVP_AEAD_CTX_aead(ctx_.get()));
+ if (SSL_CIPHER_is_block_cipher(cipher())) {
+ size_t block_size;
+ switch (cipher()->algorithm_enc) {
+ case SSL_AES128:
+ case SSL_AES256:
+ block_size = 16;
+ break;
+ case SSL_3DES:
+ block_size = 8;
+ break;
+ default:
+ abort();
+ }
+
+ // The output for a CBC cipher is always a whole number of blocks. Round the
+ // remaining capacity down.
+ max_out &= ~(block_size - 1);
+ // The maximum overhead is a full block of padding and the MAC, but the
+ // minimum overhead is one byte of padding, once we know the output is
+ // rounded down.
+ assert(overhead > block_size);
+ overhead -= block_size - 1;
+ }
+ return max_out <= overhead ? 0 : max_out - overhead;
+}
+
Span<const uint8_t> SSLAEADContext::GetAdditionalData(
uint8_t storage[13], uint8_t type, uint16_t record_version, uint64_t seqnum,
size_t plaintext_len, Span<const uint8_t> header) {
diff --git a/ssl/ssl_internal_test.cc b/ssl/ssl_internal_test.cc
index 42c9c79..90d91bd 100644
--- a/ssl/ssl_internal_test.cc
+++ b/ssl/ssl_internal_test.cc
@@ -14,6 +14,7 @@
#include <gtest/gtest.h>
+#include <openssl/aead.h>
#include <openssl/ssl.h>
#include "internal.h"
@@ -663,6 +664,218 @@
expect_queue({1, 2, 3});
}
+#if !defined(BORINGSSL_UNSAFE_FUZZER_MODE)
+TEST(SSLAEADContextTest, Lengths) {
+ struct LengthTest {
+ // All plaintext lengths from |min_plaintext_len| to |max_plaintext_len|
+ // should return in |cipertext_len|.
+ size_t min_plaintext_len;
+ size_t max_plaintext_len;
+ size_t ciphertext_len;
+ };
+
+ struct CipherLengthTest {
+ // |SSL3_CK_*| and |TLS1_CK_*| constants include an extra byte at the front,
+ // so these constants must be masked with 0xffff.
+ uint16_t cipher;
+ uint16_t version;
+ size_t enc_key_len, mac_key_len, fixed_iv_len;
+ size_t block_size;
+ std::vector<LengthTest> length_tests;
+ };
+
+ const CipherLengthTest kTests[] = {
+ // 20-byte MAC, 8-byte CBC blocks with padding
+ {
+ /*cipher=*/SSL3_CK_RSA_DES_192_CBC3_SHA & 0xffff,
+ /*version=*/TLS1_2_VERSION,
+ /*enc_key_len=*/24,
+ /*mac_key_len=*/20,
+ /*fixed_iv_len=*/0,
+ /*block_size=*/8,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/3,
+ /*ciphertext_len=*/32},
+ {/*min_plaintext_len=*/4,
+ /*max_plaintext_len=*/11,
+ /*ciphertext_len=*/40},
+ {/*min_plaintext_len=*/12,
+ /*max_plaintext_len=*/19,
+ /*ciphertext_len=*/48},
+ },
+ },
+ // 20-byte MAC, 16-byte CBC blocks with padding
+ {
+ /*cipher=*/TLS1_CK_RSA_WITH_AES_128_SHA & 0xffff,
+ /*version=*/TLS1_2_VERSION,
+ /*enc_key_len=*/16,
+ /*mac_key_len=*/20,
+ /*fixed_iv_len=*/0,
+ /*block_size=*/16,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/11,
+ /*ciphertext_len=*/48},
+ {/*min_plaintext_len=*/12,
+ /*max_plaintext_len=*/27,
+ /*ciphertext_len=*/64},
+ {/*min_plaintext_len=*/38,
+ /*max_plaintext_len=*/43,
+ /*ciphertext_len=*/80},
+ },
+ },
+ // 32-byte MAC, 16-byte CBC blocks with padding
+ {
+ /*cipher=*/TLS1_CK_ECDHE_RSA_WITH_AES_128_CBC_SHA256 & 0xffff,
+ /*version=*/TLS1_2_VERSION,
+ /*enc_key_len=*/16,
+ /*mac_key_len=*/32,
+ /*fixed_iv_len=*/0,
+ /*block_size=*/16,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/15,
+ /*ciphertext_len=*/64},
+ {/*min_plaintext_len=*/16,
+ /*max_plaintext_len=*/31,
+ /*ciphertext_len=*/80},
+ {/*min_plaintext_len=*/32,
+ /*max_plaintext_len=*/47,
+ /*ciphertext_len=*/96},
+ },
+ },
+ // 8-byte explicit IV, 16-byte tag
+ {
+ /*cipher=*/TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256 & 0xffff,
+ /*version=*/TLS1_2_VERSION,
+ /*enc_key_len=*/16,
+ /*mac_key_len=*/0,
+ /*fixed_iv_len=*/4,
+ /*block_size=*/1,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/0,
+ /*ciphertext_len=*/24},
+ {/*min_plaintext_len=*/1,
+ /*max_plaintext_len=*/1,
+ /*ciphertext_len=*/25},
+ {/*min_plaintext_len=*/2,
+ /*max_plaintext_len=*/2,
+ /*ciphertext_len=*/26},
+ {/*min_plaintext_len=*/42,
+ /*max_plaintext_len=*/42,
+ /*ciphertext_len=*/66},
+ },
+ },
+ // No explicit IV, 16-byte tag. TLS 1.3's padding and record type overhead
+ // is added at another layer.
+ {
+ /*cipher=*/TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 & 0xffff,
+ /*version=*/TLS1_2_VERSION,
+ /*enc_key_len=*/32,
+ /*mac_key_len=*/0,
+ /*fixed_iv_len=*/12,
+ /*block_size=*/1,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/0,
+ /*ciphertext_len=*/16},
+ {/*min_plaintext_len=*/1,
+ /*max_plaintext_len=*/1,
+ /*ciphertext_len=*/17},
+ {/*min_plaintext_len=*/2,
+ /*max_plaintext_len=*/2,
+ /*ciphertext_len=*/18},
+ {/*min_plaintext_len=*/42,
+ /*max_plaintext_len=*/42,
+ /*ciphertext_len=*/58},
+ },
+ },
+ {
+ /*cipher=*/TLS1_CK_AES_128_GCM_SHA256 & 0xffff,
+ /*version=*/TLS1_3_VERSION,
+ /*enc_key_len=*/16,
+ /*mac_key_len=*/0,
+ /*fixed_iv_len=*/12,
+ /*block_size=*/1,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/0,
+ /*ciphertext_len=*/16},
+ {/*min_plaintext_len=*/1,
+ /*max_plaintext_len=*/1,
+ /*ciphertext_len=*/17},
+ {/*min_plaintext_len=*/2,
+ /*max_plaintext_len=*/2,
+ /*ciphertext_len=*/18},
+ {/*min_plaintext_len=*/42,
+ /*max_plaintext_len=*/42,
+ /*ciphertext_len=*/58},
+ },
+ },
+ {
+ /*cipher=*/TLS1_CK_CHACHA20_POLY1305_SHA256 & 0xffff,
+ /*version=*/TLS1_3_VERSION,
+ /*enc_key_len=*/32,
+ /*mac_key_len=*/0,
+ /*fixed_iv_len=*/12,
+ /*block_size=*/1,
+ {
+ {/*min_plaintext_len=*/0,
+ /*max_plaintext_len=*/0,
+ /*ciphertext_len=*/16},
+ {/*min_plaintext_len=*/1,
+ /*max_plaintext_len=*/1,
+ /*ciphertext_len=*/17},
+ {/*min_plaintext_len=*/2,
+ /*max_plaintext_len=*/2,
+ /*ciphertext_len=*/18},
+ {/*min_plaintext_len=*/42,
+ /*max_plaintext_len=*/42,
+ /*ciphertext_len=*/58},
+ },
+ },
+ };
+
+ for (const auto &cipher_test : kTests) {
+ const SSL_CIPHER *cipher =
+ SSL_get_cipher_by_value(static_cast<uint16_t>(cipher_test.cipher));
+ ASSERT_TRUE(cipher) << "Could not find cipher " << cipher_test.cipher;
+ SCOPED_TRACE(SSL_CIPHER_standard_name(cipher));
+
+ const uint8_t kZeros[EVP_AEAD_MAX_KEY_LENGTH] = {0};
+ UniquePtr<SSLAEADContext> aead = SSLAEADContext::Create(
+ evp_aead_seal, cipher_test.version, cipher,
+ MakeConstSpan(kZeros).first(cipher_test.enc_key_len),
+ MakeConstSpan(kZeros).first(cipher_test.mac_key_len),
+ MakeConstSpan(kZeros).first(cipher_test.fixed_iv_len));
+ ASSERT_TRUE(aead);
+
+ for (const auto &t : cipher_test.length_tests) {
+ SCOPED_TRACE(t.ciphertext_len);
+
+ for (size_t plaintext_len = t.min_plaintext_len;
+ plaintext_len <= t.max_plaintext_len; plaintext_len++) {
+ SCOPED_TRACE(plaintext_len);
+ size_t out_len;
+ ASSERT_TRUE(aead->CiphertextLen(&out_len, plaintext_len, 0));
+ EXPECT_EQ(out_len, t.ciphertext_len);
+ }
+
+ EXPECT_EQ(aead->MaxSealInputLen(t.ciphertext_len), t.max_plaintext_len);
+ for (size_t extra = 0; extra < cipher_test.block_size; extra++) {
+ // Adding up to block_size - 1 bytes of space should not change how much
+ // room we have.
+ SCOPED_TRACE(extra);
+ EXPECT_EQ(aead->MaxSealInputLen(t.ciphertext_len + extra),
+ t.max_plaintext_len);
+ }
+ }
+ }
+}
+#endif // !BORINGSSL_UNSAFE_FUZZER_MODE
+
} // namespace
BSSL_NAMESPACE_END
#endif // !BORINGSSL_SHARED_LIBRARY
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index d308db9..b5bf783 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1259,7 +1259,8 @@
SendInitialRecordVersion uint16
// MaxPacketLength, if non-zero, is the maximum acceptable size for a
- // packet.
+ // packet. The shim will also be expected to maximally fill packets in the
+ // handshake up to this limit.
MaxPacketLength int
// SendCipherSuite, if non-zero, is the cipher suite value that the
@@ -1862,8 +1863,10 @@
MaxReceivePlaintext int
// ExpectPackedEncryptedHandshake, if non-zero, requires that the peer maximally
- // pack their encrypted handshake messages, fitting at most the
- // specified number of plaintext bytes per record.
+ // pack their encrypted handshake messages, fitting at most the specified number
+ // of bytes per record. In TLS, the limit counts plaintext bytes. In DTLS, it
+ // counts packet size and checks both that fragments are packed into records and
+ // records are packed into packets.
ExpectPackedEncryptedHandshake int
// SendTicketLifetime, if non-zero, is the ticket lifetime to send in
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index e8b6c0a..167ab13 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -26,6 +26,14 @@
"golang.org/x/crypto/cryptobyte"
)
+type dtlsRecordInfo struct {
+ typ recordType
+ epoch uint16
+ // bytesAvailable is the number of additional bytes of plaintext that could
+ // have been added to this record without exceeding the packet limit.
+ bytesAvailable int
+}
+
// A Conn represents a secured connection.
// It implements the net.Conn interface.
type Conn struct {
@@ -124,6 +132,16 @@
// handshake data may be received until the next flight or epoch change.
seenHandshakePackEnd bool
+ // lastRecordInFlight contains information about the previous handshake or
+ // ChangeCipherSpec record from the current flight, or nil if we are not in
+ // the middle of reading a flight from the peer.
+ lastRecordInFlight *dtlsRecordInfo
+
+ // bytesAvailableInPacket is the number of bytes that were still available
+ // in the current DTLS packet, up to a budget of
+ // config.Bugs.MaxPacketLength.
+ bytesAvailableInPacket int
+
// echAccepted indicates whether ECH was accepted for this connection.
echAccepted bool
@@ -1176,6 +1194,7 @@
// c.out.Mutex <= L.
func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
c.seenHandshakePackEnd = false
+ c.lastRecordInFlight = nil
if typ == recordTypeHandshake {
msgType := data[0]
if c.config.Bugs.SendWrongMessageType != 0 && msgType == c.config.Bugs.SendWrongMessageType {
@@ -1507,6 +1526,7 @@
return errors.New("tls: TimeoutSchedule set without PacketAdapter")
}
for _, timeout := range c.config.Bugs.TimeoutSchedule {
+ c.lastRecordInFlight = nil
// Simulate a timeout.
packets, err := c.config.Bugs.PacketAdaptor.SendReadTimeout(timeout)
if err != nil {
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index 3804609..87a8081 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -164,6 +164,7 @@
func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, []byte, error) {
// Read a new packet only if the current one is empty.
var newPacket bool
+ bytesAvailableInLastPacket := c.bytesAvailableInPacket
if c.rawInput.Len() == 0 {
// Pick some absurdly large buffer size.
c.rawInput.Grow(maxCiphertext + dtlsMaxRecordHeaderLen)
@@ -172,8 +173,13 @@
if err != nil {
return 0, nil, err
}
- if c.config.Bugs.MaxPacketLength != 0 && n > c.config.Bugs.MaxPacketLength {
- return 0, nil, fmt.Errorf("dtls: exceeded maximum packet length")
+ if maxPacket := c.config.Bugs.MaxPacketLength; maxPacket != 0 {
+ if n > maxPacket {
+ return 0, nil, fmt.Errorf("dtls: exceeded maximum packet length")
+ }
+ c.bytesAvailableInPacket = maxPacket - n
+ } else {
+ c.bytesAvailableInPacket = 0
}
c.rawInput.Write(buf[:n])
newPacket = true
@@ -212,12 +218,43 @@
typ = encTyp
}
- // Require that ChangeCipherSpec always share a packet with either the
- // previous or next handshake message.
- if newPacket && typ == recordTypeChangeCipherSpec && c.rawInput.Len() == 0 {
- return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: ChangeCipherSpec not packed together with Finished"))
- }
+ if typ == recordTypeChangeCipherSpec || typ == recordTypeHandshake {
+ // If this is not the first record in the flight, check if it was packed
+ // efficiently.
+ if c.lastRecordInFlight != nil {
+ // 12-byte header + 1-byte fragment is the minimum to make progress.
+ const handshakeBytesNeeded = 13
+ if typ == recordTypeHandshake && c.lastRecordInFlight.typ == recordTypeHandshake && epoch.epoch == c.lastRecordInFlight.epoch {
+ // The previous record was compatible with this one. The shim
+ // should have fit more in this record before making a new one.
+ if c.lastRecordInFlight.bytesAvailable > handshakeBytesNeeded {
+ return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: previous handshake record had %d bytes available, but shim did not fit another fragment in it", c.lastRecordInFlight.bytesAvailable))
+ }
+ } else if newPacket {
+ // The shim had to make a new record, but it did not need to
+ // make a new packet if this record fit in the previous.
+ bytesNeeded := 1
+ if typ == recordTypeHandshake {
+ bytesNeeded = handshakeBytesNeeded
+ }
+ bytesNeeded += recordHeaderLen + c.in.maxEncryptOverhead(epoch, bytesNeeded)
+ if bytesNeeded < bytesAvailableInLastPacket {
+ return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: previous packet had %d bytes available, but shim did not fit record of type %d into it", bytesAvailableInLastPacket, typ))
+ }
+ }
+ }
+ // Save information about the current record, including how many more
+ // bytes the shim could have added.
+ recordBytesAvailable := c.bytesAvailableInPacket + c.rawInput.Len()
+ if cbc, ok := epoch.cipher.(cbcMode); ok {
+ // It is possible that adding a byte would have added another block.
+ recordBytesAvailable = max(0, recordBytesAvailable-cbc.BlockSize())
+ }
+ c.lastRecordInFlight = &dtlsRecordInfo{typ: typ, epoch: epoch.epoch, bytesAvailable: recordBytesAvailable}
+ } else {
+ c.lastRecordInFlight = nil
+ }
return typ, data, nil
}
@@ -620,6 +657,16 @@
if fragOff+fragLen > c.handMsgLen {
return nil, errors.New("dtls: bad fragment length")
}
+ // If the message isn't complete, check that the peer could not have
+ // fit more into the record.
+ if fragOff+fragLen < c.handMsgLen {
+ if c.hand.Len() != 0 {
+ return nil, errors.New("dtls: truncated handshake fragment was not last in the record")
+ }
+ if c.lastRecordInFlight.bytesAvailable > 0 {
+ return nil, fmt.Errorf("dtls: handshake fragment was truncated, but record could have fit %d more bytes", c.lastRecordInFlight.bytesAvailable)
+ }
+ }
c.handMsg = append(c.handMsg, fragment...)
}
c.recvHandshakeSeq++
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 66d0849..8c745e3 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2377,8 +2377,10 @@
{
protocol: dtls,
testType: serverTest,
- name: "MTU",
+ name: "MTU-DTLS12-AEAD",
config: Config{
+ MaxVersion: VersionTLS12,
+ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
Bugs: ProtocolBugs{
MaxPacketLength: 256,
},
@@ -2388,15 +2390,40 @@
{
protocol: dtls,
testType: serverTest,
- name: "MTUExceeded",
+ name: "MTU-DTLS12-AES-CBC",
config: Config{
+ MaxVersion: VersionTLS12,
+ CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256},
Bugs: ProtocolBugs{
- MaxPacketLength: 255,
+ MaxPacketLength: 256,
},
},
- flags: []string{"-mtu", "256"},
- shouldFail: true,
- expectedLocalError: "dtls: exceeded maximum packet length",
+ flags: []string{"-mtu", "256"},
+ },
+ {
+ protocol: dtls,
+ testType: serverTest,
+ name: "MTU-DTLS12-3DES-CBC",
+ config: Config{
+ MaxVersion: VersionTLS12,
+ CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA},
+ Bugs: ProtocolBugs{
+ MaxPacketLength: 256,
+ },
+ },
+ flags: []string{"-mtu", "256", "-cipher", "TLS_RSA_WITH_3DES_EDE_CBC_SHA"},
+ },
+ {
+ protocol: dtls,
+ testType: serverTest,
+ name: "MTU-DTLS13",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ MaxPacketLength: 256,
+ },
+ },
+ flags: []string{"-mtu", "256"},
},
{
name: "EmptyCertificateList",
