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>

ssl/d1_pkt.cc

@@ -216,6 +216,11 @@ int dtls1_write_app_data(SSL *ssl, bool *out_needs_handshake,
   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 @@ int dtls1_write_record(SSL *ssl, int type, Span<const uint8_t> in,
   }
 
   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(),

ssl/dtls_record.cc

@@ -204,7 +204,8 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type,
     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 @@ static const SSLAEADContext *get_write_aead(const SSL *ssl,
   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 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out,
     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 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out,
   }
   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;
 }

ssl/internal.h

@@ -1018,17 +1018,9 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type,
                                         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 @@ size_t ssl_seal_align_prefix_len(const SSL *ssl);
 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 @@ struct SSL3_STATE {
 };
 
 // lengths of messages
-#define DTLS1_RT_HEADER_LENGTH 13
+#define DTLS1_RT_MAX_HEADER_LENGTH 13
 
 #define DTLS1_HM_HEADER_LENGTH 12
 

ssl/s3_pkt.cc

@@ -198,6 +198,26 @@ int tls_write_app_data(SSL *ssl, bool *out_needs_handshake,
   }
 }
 
+// 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 @@ static int do_tls_write(SSL *ssl, size_t *out_bytes_written, uint8_t type,
     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;
   }

ssl/ssl_buffer.cc

@@ -172,14 +172,17 @@ int ssl_read_buffer_extend_to(SSL *ssl, size_t len) {
 
   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 @@ static_assert(SSL3_RT_HEADER_LENGTH * 2 +
                   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");

ssl/test/test_config.cc

@@ -752,9 +752,13 @@ static void MessageCallback(int is_write, int version, int content_type,
   }
 
   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;
     }

ssl/tls_record.cc

@@ -140,7 +140,7 @@ static const uint8_t kMaxWarningAlerts = 4;
 
 // 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 @@ static bool ssl_needs_record_splitting(const SSL *ssl) {
 }
 
 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,