Use dtls_record_header_write_len instead of DTLS1_RT_HEADER_LENGTH. In DTLS 1.3, the record header length is not constant. Replace the use of a constant for the record header length with a function that returns the record header length. Bug: 715 Change-Id: Ie742a7b6dd675d81c12ed1245c0b4046a84446ac Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69687 Commit-Queue: Bob Beck <bbe@google.com> Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index da898b4..9fead5c 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc
@@ -216,6 +216,11 @@ return 1; } +static size_t dtls_seal_align_prefix_len(const SSL *ssl, uint16_t epoch) { + return dtls_record_header_write_len(ssl, epoch) + + ssl->s3->aead_write_ctx->ExplicitNonceLen(); +} + int dtls1_write_record(SSL *ssl, int type, Span<const uint8_t> in, uint16_t epoch) { SSLBuffer *buf = &ssl->s3->write_buffer; @@ -231,7 +236,7 @@ } size_t ciphertext_len; - if (!buf->EnsureCap(ssl_seal_align_prefix_len(ssl), + if (!buf->EnsureCap(dtls_seal_align_prefix_len(ssl, epoch), in.size() + SSL_max_seal_overhead(ssl)) || !dtls_seal_record(ssl, buf->remaining().data(), &ciphertext_len, buf->remaining().size(), type, in.data(), in.size(),
diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc index 6551aa4..0b2a51b 100644 --- a/ssl/dtls_record.cc +++ b/ssl/dtls_record.cc
@@ -204,7 +204,8 @@ return ssl_open_record_discard; } - Span<const uint8_t> header = in.subspan(0, DTLS1_RT_HEADER_LENGTH); + Span<const uint8_t> header = + in.subspan(0, dtls_record_header_write_len(ssl, ssl->d1->r_epoch)); ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, header); uint64_t sequence = CRYPTO_load_u64_be(sequence_bytes); @@ -268,13 +269,19 @@ return ssl->s3->aead_write_ctx.get(); } +size_t dtls_record_header_write_len(const SSL *ssl, uint16_t epoch) { + // 13 is the value of the former DTLS1_RT_HEADER_LENGTH constant. + return 13; +} + size_t dtls_max_seal_overhead(const SSL *ssl, uint16_t epoch) { - return DTLS1_RT_HEADER_LENGTH + get_write_aead(ssl, epoch)->MaxOverhead(); + return dtls_record_header_write_len(ssl, epoch) + + get_write_aead(ssl, epoch)->MaxOverhead(); } size_t dtls_seal_prefix_len(const SSL *ssl, uint16_t epoch) { - return DTLS1_RT_HEADER_LENGTH + + return dtls_record_header_write_len(ssl, epoch) + get_write_aead(ssl, epoch)->ExplicitNonceLen(); } @@ -299,7 +306,8 @@ assert(epoch == ssl->d1->w_epoch); } - if (max_out < DTLS1_RT_HEADER_LENGTH) { + const size_t record_header_len = dtls_record_header_write_len(ssl, epoch); + if (max_out < record_header_len) { OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFER_TOO_SMALL); return false; } @@ -327,18 +335,18 @@ } out[11] = ciphertext_len >> 8; out[12] = ciphertext_len & 0xff; - Span<const uint8_t> header = MakeConstSpan(out, DTLS1_RT_HEADER_LENGTH); + Span<const uint8_t> header = MakeConstSpan(out, record_header_len); size_t len_copy; - if (!aead->Seal(out + DTLS1_RT_HEADER_LENGTH, &len_copy, - max_out - DTLS1_RT_HEADER_LENGTH, type, record_version, + if (!aead->Seal(out + record_header_len, &len_copy, + max_out - record_header_len, type, record_version, seq_with_epoch, header, in, in_len)) { return false; } assert(ciphertext_len == len_copy); (*seq)++; - *out_len = DTLS1_RT_HEADER_LENGTH + ciphertext_len; + *out_len = record_header_len + ciphertext_len; ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header); return true; }
diff --git a/ssl/internal.h b/ssl/internal.h index 4db9f17..4a84883 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1018,17 +1018,9 @@ size_t *out_consumed, uint8_t *out_alert, Span<uint8_t> in); -// ssl_seal_align_prefix_len returns the length of the prefix before the start -// of the bulk of the ciphertext when sealing a record with |ssl|. Callers may -// use this to align buffers. -// -// Note when TLS 1.0 CBC record-splitting is enabled, this includes the one byte -// record and is the offset into second record's ciphertext. Thus sealing a -// small record may result in a smaller output than this value. -// -// TODO(davidben): Is this alignment valuable? Record-splitting makes this a -// mess. -size_t ssl_seal_align_prefix_len(const SSL *ssl); +// ssl_needs_record_splitting returns one if |ssl|'s current outgoing cipher +// state needs record-splitting and zero otherwise. +bool ssl_needs_record_splitting(const SSL *ssl); // tls_seal_record seals a new record of type |type| and body |in| and writes it // to |out|. At most |max_out| bytes will be written. It returns true on success @@ -1044,6 +1036,10 @@ bool tls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, uint8_t type, const uint8_t *in, size_t in_len); +// dtls_record_header_write_len returns the length of the record header that +// will be written at |epoch|. +size_t dtls_record_header_write_len(const SSL *ssl, uint16_t epoch); + // dtls_max_seal_overhead returns the maximum overhead, in bytes, of sealing a // record. size_t dtls_max_seal_overhead(const SSL *ssl, uint16_t epoch); @@ -2939,7 +2935,7 @@ }; // lengths of messages -#define DTLS1_RT_HEADER_LENGTH 13 +#define DTLS1_RT_MAX_HEADER_LENGTH 13 #define DTLS1_HM_HEADER_LENGTH 12
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc index bc0d13d..a34c5a6 100644 --- a/ssl/s3_pkt.cc +++ b/ssl/s3_pkt.cc
@@ -198,6 +198,26 @@ } } +// tls_seal_align_prefix_len returns the length of the prefix before the start +// of the bulk of the ciphertext when sealing a record with |ssl|. Callers may +// use this to align buffers. +// +// Note when TLS 1.0 CBC record-splitting is enabled, this includes the one byte +// record and is the offset into second record's ciphertext. Thus sealing a +// small record may result in a smaller output than this value. +// +// TODO(davidben): Is this alignment valuable? Record-splitting makes this a +// mess. +static size_t tls_seal_align_prefix_len(const SSL *ssl) { + size_t ret = + SSL3_RT_HEADER_LENGTH + ssl->s3->aead_write_ctx->ExplicitNonceLen(); + if (ssl_needs_record_splitting(ssl)) { + ret += SSL3_RT_HEADER_LENGTH; + ret += ssl_cipher_get_record_split_len(ssl->s3->aead_write_ctx->cipher()); + } + return ret; +} + // do_tls_write writes an SSL record of the given type. On success, it sets // |*out_bytes_written| to number of bytes successfully written and returns one. // On error, it returns a value <= 0 from the underlying |BIO|. @@ -265,7 +285,7 @@ return 1; } - if (!buf->EnsureCap(pending_flight.size() + ssl_seal_align_prefix_len(ssl), + if (!buf->EnsureCap(pending_flight.size() + tls_seal_align_prefix_len(ssl), max_out)) { return -1; }
diff --git a/ssl/ssl_buffer.cc b/ssl/ssl_buffer.cc index 2ca14ef..7de8923 100644 --- a/ssl/ssl_buffer.cc +++ b/ssl/ssl_buffer.cc
@@ -172,14 +172,17 @@ if (SSL_is_dtls(ssl)) { static_assert( - DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH <= 0xffff, + DTLS1_RT_MAX_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH <= 0xffff, "DTLS read buffer is too large"); // The |len| parameter is ignored in DTLS. - len = DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH; + len = DTLS1_RT_MAX_HEADER_LENGTH + SSL3_RT_MAX_ENCRYPTED_LENGTH; } - if (!ssl->s3->read_buffer.EnsureCap(ssl_record_prefix_len(ssl), len)) { + // The DTLS record header can have a variable length, so the |header_len| + // value provided for buffer alignment only works if the header is the maximum + // length. + if (!ssl->s3->read_buffer.EnsureCap(DTLS1_RT_MAX_HEADER_LENGTH, len)) { return -1; } @@ -252,7 +255,7 @@ 0xffff, "maximum TLS write buffer is too large"); -static_assert(DTLS1_RT_HEADER_LENGTH + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + +static_assert(DTLS1_RT_MAX_HEADER_LENGTH + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + SSL3_RT_MAX_PLAIN_LENGTH <= 0xffff, "maximum DTLS write buffer is too large");
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index dfe1cb4..9ecbea4 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -752,9 +752,13 @@ } if (content_type == SSL3_RT_HEADER) { - size_t header_len = - config->is_dtls ? DTLS1_RT_HEADER_LENGTH : SSL3_RT_HEADER_LENGTH; - if (len != header_len) { + if (config->is_dtls) { + if (len > DTLS1_RT_MAX_HEADER_LENGTH) { + fprintf(stderr, "DTLS record header is too long: %zu.\n", len); + } + return; + } + if (len != SSL3_RT_HEADER_LENGTH) { fprintf(stderr, "Incorrect length for record header: %zu.\n", len); state->msg_callback_ok = false; }
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc index 5a820f6..5040862 100644 --- a/ssl/tls_record.cc +++ b/ssl/tls_record.cc
@@ -140,7 +140,7 @@ // ssl_needs_record_splitting returns one if |ssl|'s current outgoing cipher // state needs record-splitting and zero otherwise. -static bool ssl_needs_record_splitting(const SSL *ssl) { +bool ssl_needs_record_splitting(const SSL *ssl) { #if !defined(BORINGSSL_UNSAFE_FUZZER_MODE) return !ssl->s3->aead_write_ctx->is_null_cipher() && ssl->s3->aead_write_ctx->ProtocolVersion() < TLS1_1_VERSION && @@ -152,28 +152,8 @@ } size_t ssl_record_prefix_len(const SSL *ssl) { - size_t header_len; - if (SSL_is_dtls(ssl)) { - header_len = DTLS1_RT_HEADER_LENGTH; - } else { - header_len = SSL3_RT_HEADER_LENGTH; - } - - return header_len + ssl->s3->aead_read_ctx->ExplicitNonceLen(); -} - -size_t ssl_seal_align_prefix_len(const SSL *ssl) { - if (SSL_is_dtls(ssl)) { - return DTLS1_RT_HEADER_LENGTH + ssl->s3->aead_write_ctx->ExplicitNonceLen(); - } - - size_t ret = - SSL3_RT_HEADER_LENGTH + ssl->s3->aead_write_ctx->ExplicitNonceLen(); - if (ssl_needs_record_splitting(ssl)) { - ret += SSL3_RT_HEADER_LENGTH; - ret += ssl_cipher_get_record_split_len(ssl->s3->aead_write_ctx->cipher()); - } - return ret; + assert(!SSL_is_dtls(ssl)); + return SSL3_RT_HEADER_LENGTH + ssl->s3->aead_read_ctx->ExplicitNonceLen(); } static ssl_open_record_t skip_early_data(SSL *ssl, uint8_t *out_alert,