diff --git a/src/ssl/d1_both.cc b/src/ssl/d1_both.cc index c460e143a..0b3711fdc 100644 --- a/src/ssl/d1_both.cc +++ b/src/ssl/d1_both.cc @@ -680,138 +680,159 @@ static void dtls1_update_mtu(SSL *ssl) { 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 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 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 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 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 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/src/ssl/dtls_record.cc b/src/ssl/dtls_record.cc index 586b8528f..8a17ee299 100644 --- a/src/ssl/dtls_record.cc +++ b/src/ssl/dtls_record.cc @@ -490,6 +490,24 @@ size_t dtls_seal_prefix_len(const SSL *ssl, uint16_t epoch) { 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/src/ssl/internal.h b/src/ssl/internal.h index f2b8b9314..15634d6f8 100644 --- a/src/ssl/internal.h +++ b/src/ssl/internal.h @@ -1151,6 +1151,10 @@ class SSLAEADContext { // 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 @@ size_t dtls_max_seal_overhead(const SSL *ssl, uint16_t epoch); // 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/src/ssl/s3_both.cc b/src/ssl/s3_both.cc index b0e298cc0..8110725a3 100644 --- a/src/ssl/s3_both.cc +++ b/src/ssl/s3_both.cc @@ -192,7 +192,7 @@ bool tls_add_message(SSL *ssl, Array msg) { // 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 rest = msg; if (ssl->quic_method == nullptr && ssl->s3->aead_write_ctx->is_null_cipher()) { diff --git a/src/ssl/ssl_aead_ctx.cc b/src/ssl/ssl_aead_ctx.cc index 2db919f4b..7a2dda164 100644 --- a/src/ssl/ssl_aead_ctx.cc +++ b/src/ssl/ssl_aead_ctx.cc @@ -173,6 +173,43 @@ size_t SSLAEADContext::MaxOverhead() const { : 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 SSLAEADContext::GetAdditionalData( uint8_t storage[13], uint8_t type, uint16_t record_version, uint64_t seqnum, size_t plaintext_len, Span header) { diff --git a/src/ssl/ssl_internal_test.cc b/src/ssl/ssl_internal_test.cc index 42c9c79a1..90d91bd13 100644 --- a/src/ssl/ssl_internal_test.cc +++ b/src/ssl/ssl_internal_test.cc @@ -14,6 +14,7 @@ #include +#include #include #include "internal.h" @@ -663,6 +664,218 @@ TEST(MRUQueueTest, Basic) { 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 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(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 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/src/ssl/test/runner/common.go b/src/ssl/test/runner/common.go index d308db9be..b5bf78345 100644 --- a/src/ssl/test/runner/common.go +++ b/src/ssl/test/runner/common.go @@ -1259,7 +1259,8 @@ type ProtocolBugs struct { 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 @@ type ProtocolBugs struct { 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/src/ssl/test/runner/conn.go b/src/ssl/test/runner/conn.go index e8b6c0a94..167ab1340 100644 --- a/src/ssl/test/runner/conn.go +++ b/src/ssl/test/runner/conn.go @@ -26,6 +26,14 @@ import ( "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 @@ type Conn struct { // 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 @@ func (c *Conn) writeV2Record(data []byte) (n int, err error) { // 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 @@ func (c *Conn) simulatePacketLoss(resendFunc func()) error { 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/src/ssl/test/runner/dtls.go b/src/ssl/test/runner/dtls.go index 3804609ea..87a808151 100644 --- a/src/ssl/test/runner/dtls.go +++ b/src/ssl/test/runner/dtls.go @@ -164,6 +164,7 @@ func (c *Conn) writeACKs(seqnums []uint64) { 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 @@ func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, []byte, error) { 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 @@ func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, []byte, error) { 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 @@ func (c *Conn) dtlsDoReadHandshake() ([]byte, error) { 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/src/ssl/test/runner/runner.go b/src/ssl/test/runner/runner.go index 66d084909..8c745e367 100644 --- a/src/ssl/test/runner/runner.go +++ b/src/ssl/test/runner/runner.go @@ -2377,8 +2377,10 @@ read alert 1 0 { 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 @@ read alert 1 0 { 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",