Rework how DTLS ACKs and retransmits are flushed

This CL addresses a number of issues:

1. If an ACK or retransmit hit SSL_ERROR_WANT_WRITE, we do not provide a
   way for the caller to retry it.

2. If the retransmit timer fires too many times, we do not track the
   fatal error. Instead, we return an error out of the
   DTLSv1_handle_timeout and leave the timeout unhandled. If the caller
   does not tear down the connection in response to
   DTLSv1_handle_timeout failing, they may register a timeout for 0s,
   wake up again and loop forever.

   This happened in https://crbug.com/42224241. I filed
   https://crbug.com/42290595 to track fixing this API wart.

3. When the DTLS open_app_data hook needs to retransmit or send an ACK,
   we write immediately in that callback, but the eventual aim was to
   provide an in-place encrypt/decrypt API, so those hooks should not
   perform I/O.

   DTLS 1.3 adds a lot more cases where we'll want to do this:

   - We might read a message and then want to ACK on a timer
   - We might read a message and then want to ACK immediately
   - We might read a past retransmit and then want to retransmit
     the next flight
   - We might read an ACK, clear the current flight, and then want to
     send a queued up KeyUpdate that was waiting on it.

To address all these, this CL rearranges things slightly. Now the DTLS
record layer keeps track of whether it owes the peer an ACK and a
flight. If so, the flush() operation will try to write those out. If it
fails, it keeps track of how far it got. If it succeeds, it clears those
flags.

Various operations can set those flags, notably the timeouts. This means
the actual "handle timeout" part of DTLSv1_handle_timeout is
infallible. Handling a timeout means cashing the timeout expiry in for
setting the flag.

From there, we drive flush() to completion where we need to. For now,
I've added it to the handshake and SSL_read(), though I'm not sure if
I've gotten exactly the right spots. (Should we also flush on
SSL_write?) We could also go further and have flush() drive
handle_timeout(), and then handle_timeout() can be deprecated
altogether. The model is just "you must retry this operation after the
timeout, in addition to the SSL_ERROR_WANT_WHATEVER you got". That's
quite attractive, but for now I haven't quite gone that far.

Another complication is that flush() currently behaves very differently
between TLS and DTLS. In DTLS, I had to add an explicit finish_flight()
to queue up the flight for sending. If you leave things in there, it's
no big deal because everything in DTLS is unordered. In TLS, everything
is ordered, so once we've encrypted records, we have to flush them
regardless. To that end, in TLS we generally like to flush things
implicitly on write, because that's when the caller is expecting I/O
anyway, and we can concatenate the, say, encrypted KeyUpdate with the
encrypted application data. Not sure what's best to do there. For now
I've just gated flush() calls on SSL_is_dtls().

I'm sure we'll find this also isn't quite right and rearrange it again
later, but hopefully this works a bit better than what we had before.

Bug: 42290594, 376718254
Fixed: 42290595
Change-Id: I4d9b5c753530b514a1bc5e4e69ddb25dfc8c1d06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73527
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>

include/openssl/ssl.h

@@ -623,7 +623,8 @@ OPENSSL_EXPORT void DTLSv1_set_initial_timeout_duration(SSL *ssl,
 
 // DTLSv1_get_timeout queries the running DTLS timers. If there are any in
 // progress, it sets |*out| to the time remaining until the first timer expires
-// and returns one. Otherwise, it returns zero.
+// and returns one. Otherwise, it returns zero. Timers may be scheduled both
+// during and after the handshake.
 //
 // When the timeout expires, call |DTLSv1_handle_timeout| to handle the
 // retransmit behavior.
@@ -633,9 +634,11 @@ OPENSSL_EXPORT void DTLSv1_set_initial_timeout_duration(SSL *ssl,
 OPENSSL_EXPORT int DTLSv1_get_timeout(const SSL *ssl, struct timeval *out);
 
 // DTLSv1_handle_timeout is called when a DTLS timeout expires. If no timeout
-// had expired, it returns 0. Otherwise, it retransmits the previous flight of
-// handshake messages, or post-handshake messages, and returns 1. If too many
-// timeouts had expired without progress or an error occurs, it returns -1.
+// had expired, it returns 0. Otherwise, it handles the timeout and returns 1 on
+// success or -1 on error.
+//
+// This function may write to the transport (e.g. to retransmit messages) or
+// update |ssl|'s internal state and schedule an updated timer.
 //
 // The caller's external timer should be compatible with the one |ssl| queries
 // within some fudge factor. Otherwise, the call will be a no-op, but
@@ -643,12 +646,16 @@ OPENSSL_EXPORT int DTLSv1_get_timeout(const SSL *ssl, struct timeval *out);
 //
 // If the function returns -1, checking if |SSL_get_error| returns
 // |SSL_ERROR_WANT_WRITE| may be used to determine if the retransmit failed due
-// to a non-fatal error at the write |BIO|. However, the operation may not be
-// retried until the next timeout fires.
+// to a non-fatal error at the write |BIO|. In this case, when the |BIO| is
+// writable, the operation may be retried by calling the original function,
+// |SSL_do_handshake| or |SSL_read|.
 //
 // WARNING: This function breaks the usual return value convention.
 //
-// TODO(davidben): This |SSL_ERROR_WANT_WRITE| behavior is kind of bizarre.
+// TODO(davidben): We can make this function entirely optional by just checking
+// the timers in |SSL_do_handshake| or |SSL_read|. Then timers behave like any
+// other retry condition: rerun the operation and the library will make what
+// progress it can.
 OPENSSL_EXPORT int DTLSv1_handle_timeout(SSL *ssl);
 
 

ssl/d1_both.cc

@@ -446,7 +446,7 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert,
 
     if (ssl_has_final_version(ssl) &&
         ssl_protocol_version(ssl) >= TLS1_3_VERSION &&
-        !ssl->d1->ack_timer.IsSet()) {
+        !ssl->d1->ack_timer.IsSet() && !ssl->d1->sending_ack) {
       // Schedule sending an ACK. The delay serves several purposes:
       // - If there are more records to come, we send only one ACK.
       // - If there are more records to come and the flight is now complete, we
@@ -616,6 +616,7 @@ void dtls_clear_outgoing_messages(SSL *ssl) {
   ssl->d1->outgoing_offset = 0;
   ssl->d1->outgoing_messages_complete = false;
   ssl->d1->flight_has_reply = false;
+  ssl->d1->sending_flight = false;
   dtls_clear_unused_write_epochs(ssl);
 }
 
@@ -971,6 +972,11 @@ static int send_flight(SSL *ssl) {
     return -1;
   }
 
+  if (ssl->d1->num_timeouts > DTLS1_MAX_TIMEOUTS) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_READ_TIMEOUT_EXPIRED);
+    return -1;
+  }
+
   dtls1_update_mtu(ssl);
 
   Array<uint8_t> packet;
@@ -1015,27 +1021,38 @@ static int send_flight(SSL *ssl) {
   return 1;
 }
 
-int dtls1_flush_flight(SSL *ssl, bool post_handshake) {
-  ssl->d1->outgoing_messages_complete = true;
-  if (!post_handshake) {
-    // Our new flight implicitly ACKs the previous flight, so there is no need
-    // to ACK previous records. This clears the ACK buffer slightly earlier than
-    // the specification suggests. See the discussion in
+void dtls1_finish_flight(SSL *ssl) {
+  if (ssl->d1->outgoing_messages.empty() ||
+      ssl->d1->outgoing_messages_complete) {
+    return;  // Nothing to do.
+  }
+
+  if (ssl->d1->outgoing_messages[0].epoch <= 2) {
+    // DTLS 1.3 handshake messages (epoch 2 and below) implicitly ACK the
+    // previous flight, so there is no need to ACK previous records. This
+    // clears the ACK buffer slightly earlier than the specification suggests.
+    // See the discussion in
     // https://mailarchive.ietf.org/arch/msg/tls/kjJnquJOVaWxu5hUCmNzB35eqY0/
     ssl->d1->records_to_ack.Clear();
     ssl->d1->ack_timer.Stop();
+    ssl->d1->sending_ack = false;
   }
-  // Start the retransmission timer for the next flight (if any).
-  dtls1_start_timer(ssl);
-  return send_flight(ssl);
+
+  ssl->d1->outgoing_messages_complete = true;
+  ssl->d1->sending_flight = true;
+  // Stop retransmitting the previous flight. In DTLS 1.3, we'll have stopped
+  // the timer already, but DTLS 1.2 keeps it running until the next flight is
+  // ready.
+  dtls1_stop_timer(ssl);
 }
 
-int dtls1_send_ack(SSL *ssl) {
-  assert(ssl_protocol_version(ssl) >= TLS1_3_VERSION);
+void dtls1_schedule_ack(SSL *ssl) {
   ssl->d1->ack_timer.Stop();
-  if (ssl->d1->records_to_ack.empty()) {
-    return 1;
-  }
+  ssl->d1->sending_ack = !ssl->d1->records_to_ack.empty();
+}
+
+static int send_ack(SSL *ssl) {
+  assert(ssl_protocol_version(ssl) >= TLS1_3_VERSION);
 
   // Ensure we don't send so many ACKs that we overflow the MTU. There is a
   // 2-byte length prefix and each ACK is 16 bytes.
@@ -1099,15 +1116,46 @@ int dtls1_send_ack(SSL *ssl) {
   return 1;
 }
 
-int dtls1_retransmit_outgoing_messages(SSL *ssl) {
-  // Rewind to the start of the flight and write it again.
-  //
-  // TODO(davidben): This does not allow retransmits to be resumed on
-  // non-blocking write.
-  ssl->d1->outgoing_written = 0;
-  ssl->d1->outgoing_offset = 0;
+int dtls1_flush(SSL *ssl) {
+  // Send the pending ACK, if any.
+  if (ssl->d1->sending_ack) {
+    int ret = send_ack(ssl);
+    if (ret <= 0) {
+      return ret;
+    }
+    ssl->d1->sending_ack = false;
+  }
 
-  return send_flight(ssl);
+  // Send the pending flight, if any.
+  if (ssl->d1->sending_flight) {
+    int ret = send_flight(ssl);
+    if (ret <= 0) {
+      return ret;
+    }
+
+    // Reset state for the next send.
+    ssl->d1->outgoing_written = 0;
+    ssl->d1->outgoing_offset = 0;
+    ssl->d1->sending_flight = false;
+
+    // Schedule the next retransmit timer. In DTLS 1.3, we retransmit all
+    // flights until ACKed. In DTLS 1.2, the final Finished flight is never
+    // ACKed, so we do not keep the timer running after the handshake.
+    if (SSL_in_init(ssl) || ssl_protocol_version(ssl) >= TLS1_3_VERSION) {
+      if (ssl->d1->num_timeouts == 0) {
+        ssl->d1->timeout_duration_ms = ssl->initial_timeout_duration_ms;
+      } else {
+        ssl->d1->timeout_duration_ms =
+            std::min(ssl->d1->timeout_duration_ms * 2, uint32_t{60000});
+      }
+
+      OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
+      ssl->d1->retransmit_timer.StartMicroseconds(
+          now, uint64_t{ssl->d1->timeout_duration_ms} * 1000);
+    }
+  }
+
+  return 1;
 }
 
 unsigned int dtls1_min_mtu(void) { return kMinMTU; }

ssl/d1_lib.cc

@@ -70,20 +70,14 @@
 
 BSSL_NAMESPACE_BEGIN
 
-// DTLS1_MTU_TIMEOUTS is the maximum number of timeouts to expire
-// before starting to decrease the MTU.
-#define DTLS1_MTU_TIMEOUTS 2
-
-// DTLS1_MAX_TIMEOUTS is the maximum number of timeouts to expire
-// before failing the DTLS handshake.
-#define DTLS1_MAX_TIMEOUTS 12
-
 DTLS1_STATE::DTLS1_STATE()
     : has_change_cipher_spec(false),
       outgoing_messages_complete(false),
       flight_has_reply(false),
       handshake_write_overflow(false),
-      handshake_read_overflow(false) {}
+      handshake_read_overflow(false),
+      sending_flight(false),
+      sending_ack(false) {}
 
 DTLS1_STATE::~DTLS1_STATE() {}
 
@@ -187,52 +181,12 @@ uint64_t DTLSTimer::MicrosecondsRemaining(OPENSSL_timeval now) const {
   return sec + usec;
 }
 
-void dtls1_start_timer(SSL *ssl) {
-  // If timer is not set, initialize duration.
-  if (!ssl->d1->retransmit_timer.IsSet()) {
-    ssl->d1->timeout_duration_ms = ssl->initial_timeout_duration_ms;
-  }
-
-  OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
-  ssl->d1->retransmit_timer.StartMicroseconds(
-      now, uint64_t{ssl->d1->timeout_duration_ms} * 1000);
-}
-
-static void dtls1_double_timeout(SSL *ssl) {
-  ssl->d1->timeout_duration_ms *= 2;
-  if (ssl->d1->timeout_duration_ms > 60000) {
-    ssl->d1->timeout_duration_ms = 60000;
-  }
-}
-
 void dtls1_stop_timer(SSL *ssl) {
   ssl->d1->num_timeouts = 0;
   ssl->d1->retransmit_timer.Stop();
   ssl->d1->timeout_duration_ms = ssl->initial_timeout_duration_ms;
 }
 
-bool dtls1_check_timeout_num(SSL *ssl) {
-  ssl->d1->num_timeouts++;
-
-  // Reduce MTU after 2 unsuccessful retransmissions
-  if (ssl->d1->num_timeouts > DTLS1_MTU_TIMEOUTS &&
-      !(SSL_get_options(ssl) & SSL_OP_NO_QUERY_MTU)) {
-    long mtu =
-        BIO_ctrl(ssl->wbio.get(), BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0, nullptr);
-    if (mtu >= 0 && mtu <= (1 << 30) && (unsigned)mtu >= dtls1_min_mtu()) {
-      ssl->d1->mtu = (unsigned)mtu;
-    }
-  }
-
-  if (ssl->d1->num_timeouts > DTLS1_MAX_TIMEOUTS) {
-    // fail the connection, enough alerts have been sent
-    OPENSSL_PUT_ERROR(SSL, SSL_R_READ_TIMEOUT_EXPIRED);
-    return false;
-  }
-
-  return true;
-}
-
 BSSL_NAMESPACE_END
 
 using namespace bssl;
@@ -287,25 +241,30 @@ int DTLSv1_handle_timeout(SSL *ssl) {
   bool any_timer_expired = false;
   if (ssl->d1->ack_timer.IsExpired(now)) {
     any_timer_expired = true;
-    int ret = dtls1_send_ack(ssl);
-    if (ret <= 0) {
-      return ret;
-    }
+    ssl->d1->sending_ack = true;
+    ssl->d1->ack_timer.Stop();
   }
 
   if (ssl->d1->retransmit_timer.IsExpired(now)) {
     any_timer_expired = true;
-    if (!dtls1_check_timeout_num(ssl)) {
-      return -1;
-    }
+    ssl->d1->sending_flight = true;
+    ssl->d1->retransmit_timer.Stop();
 
-    dtls1_double_timeout(ssl);
-    dtls1_start_timer(ssl);
-    int ret = dtls1_retransmit_outgoing_messages(ssl);
-    if (ret <= 0) {
-      return ret;
+    ssl->d1->num_timeouts++;
+    // Reduce MTU after 2 unsuccessful retransmissions.
+    if (ssl->d1->num_timeouts > DTLS1_MTU_TIMEOUTS &&
+        !(SSL_get_options(ssl) & SSL_OP_NO_QUERY_MTU)) {
+      long mtu = BIO_ctrl(ssl->wbio.get(), BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0,
+                          nullptr);
+      if (mtu >= 0 && mtu <= (1 << 30) && (unsigned)mtu >= dtls1_min_mtu()) {
+        ssl->d1->mtu = (unsigned)mtu;
+      }
     }
   }
 
-  return any_timer_expired ? 1 : 0;
+  if (!any_timer_expired) {
+    return 0;
+  }
+
+  return dtls1_flush(ssl);
 }

ssl/d1_pkt.cc

@@ -261,6 +261,10 @@ ssl_open_record_t dtls1_open_app_data(SSL *ssl, Span<uint8_t> *out,
     // Parse the first fragment header to determine if this is a pre-CCS or
     // post-CCS handshake record. DTLS resets handshake message numbers on each
     // handshake, so renegotiations and retransmissions are ambiguous.
+    //
+    // TODO(crbug.com/42290594): Move this logic into
+    // |dtls1_process_handshake_fragments| and integrate it into DTLS 1.3
+    // retransmit conditions.
     CBS cbs, body;
     struct hm_header_st msg_hdr;
     CBS_init(&cbs, record.data(), record.size());
@@ -272,16 +276,15 @@ ssl_open_record_t dtls1_open_app_data(SSL *ssl, Span<uint8_t> *out,
 
     if (msg_hdr.type == SSL3_MT_FINISHED &&
         msg_hdr.seq == ssl->d1->handshake_read_seq - 1) {
-      if (msg_hdr.frag_off == 0) {
+      if (!ssl->d1->sending_flight && msg_hdr.frag_off == 0) {
         // Retransmit our last flight of messages. If the peer sends the second
         // Finished, they may not have received ours. Only do this for the
         // first fragment, in case the Finished was fragmented.
-        if (!dtls1_check_timeout_num(ssl)) {
-          *out_alert = 0;  // TODO(davidben): Send an alert?
-          return ssl_open_record_error;
-        }
-
-        dtls1_retransmit_outgoing_messages(ssl);
+        //
+        // This is not really a timeout, but increment the timeout count so we
+        // eventually give up.
+        ssl->d1->num_timeouts++;
+        ssl->d1->sending_flight = true;
       }
       return ssl_open_record_discard;
     }

ssl/dtls_method.cc

@@ -195,8 +195,9 @@ static const SSL_PROTOCOL_METHOD kDTLSProtocolMethod = {
     dtls1_finish_message,
     dtls1_add_message,
     dtls1_add_change_cipher_spec,
-    dtls1_flush_flight,
-    dtls1_send_ack,
+    dtls1_finish_flight,
+    dtls1_schedule_ack,
+    dtls1_flush,
     dtls1_on_handshake_complete,
     dtls1_set_read_state,
     dtls1_set_write_state,

ssl/handshake.cc

@@ -589,6 +589,16 @@ const SSL_SESSION *ssl_handshake_session(const SSL_HANDSHAKE *hs) {
 int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
   SSL *const ssl = hs->ssl;
   for (;;) {
+    // If a timeout during the handshake triggered a DTLS ACK or retransmit, we
+    // resolve that first. E.g., if |ssl_hs_private_key_operation| is slow, the
+    // ACK timer may fire.
+    if (hs->wait != ssl_hs_error && SSL_is_dtls(ssl)) {
+      int ret = ssl->method->flush(ssl);
+      if (ret <= 0) {
+        return ret;
+      }
+    }
+
     // Resolve the operation the handshake was waiting on. Each condition may
     // halt the handshake by returning, or continue executing if the handshake
     // may immediately proceed. Cases which halt the handshake can clear
@@ -599,10 +609,8 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
         ERR_restore_state(hs->error.get());
         return -1;
 
-      case ssl_hs_flush_post_handshake:
       case ssl_hs_flush: {
-        bool post_handshake = hs->wait == ssl_hs_flush_post_handshake;
-        int ret = ssl->method->flush_flight(ssl, post_handshake);
+        int ret = ssl->method->flush(ssl);
         if (ret <= 0) {
           return ret;
         }
@@ -680,7 +688,7 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
         return -1;
 
       case ssl_hs_handback: {
-        int ret = ssl->method->flush_flight(ssl, /*post_handshake=*/false);
+        int ret = ssl->method->flush(ssl);
         if (ret <= 0) {
           return ret;
         }
@@ -733,15 +741,6 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
         ssl->s3->rwstate = SSL_ERROR_HANDSHAKE_HINTS_READY;
         return -1;
 
-      case ssl_hs_ack:
-        if (ssl->method->send_ack != nullptr) {
-          int ret = ssl->method->send_ack(ssl);
-          if (ret <= 0) {
-            return ret;
-          }
-        }
-        break;
-
       case ssl_hs_ok:
         break;
     }
@@ -761,9 +760,14 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
       *out_early_return = false;
       return 1;
     }
+    // If the handshake returns |ssl_hs_flush|, implicitly finish the flight.
+    // This is a convenience so we do not need to manually insert this
+    // throughout the handshake.
+    if (hs->wait == ssl_hs_flush) {
+      ssl->method->finish_flight(ssl);
+    }
 
-    // Otherwise, loop to the beginning and resolve what was blocking the
-    // handshake.
+    // Loop to the beginning and resolve what was blocking the handshake.
   }
 }
 

ssl/internal.h

@@ -2128,7 +2128,6 @@ enum ssl_hs_wait_t {
   ssl_hs_read_server_hello,
   ssl_hs_read_message,
   ssl_hs_flush,
-  ssl_hs_flush_post_handshake,
   ssl_hs_certificate_selection_pending,
   ssl_hs_handoff,
   ssl_hs_handback,
@@ -2142,7 +2141,6 @@ enum ssl_hs_wait_t {
   ssl_hs_read_change_cipher_spec,
   ssl_hs_certificate_verify,
   ssl_hs_hints_ready,
-  ssl_hs_ack,
 };
 
 enum ssl_grease_index_t {
@@ -2965,13 +2963,15 @@ struct SSL_PROTOCOL_METHOD {
   // add_change_cipher_spec adds a ChangeCipherSpec record to the pending
   // flight. It returns true on success and false on error.
   bool (*add_change_cipher_spec)(SSL *ssl);
-  // flush_flight flushes the pending flight to the transport. It returns one on
-  // success and <= 0 on error. If |post_handshake| is true, the flight is a
-  // post-handshake flight.
-  int (*flush_flight)(SSL *ssl, bool post_handshake);
-  // send_ack, if not NULL, sends a DTLS ACK record to the peer. It returns one
-  // on success and <= 0 on error.
-  int (*send_ack)(SSL *ssl);
+  // finish_flight marks the pending flight as finished and ready to send.
+  // |flush| must be called to write it.
+  void (*finish_flight)(SSL *ssl);
+  // schedule_ack schedules a DTLS 1.3 ACK to be sent, without an ACK delay.
+  // |flush| must be called to write it.
+  void (*schedule_ack)(SSL *ssl);
+  // flush writes any scheduled data to the transport. It returns one on success
+  // and <= 0 on error.
+  int (*flush)(SSL *ssl);
   // on_handshake_complete is called when the handshake is complete.
   void (*on_handshake_complete)(SSL *ssl);
   // set_read_state sets |ssl|'s read cipher state and level to |aead_ctx| and
@@ -3560,6 +3560,11 @@ struct DTLS1_STATE {
   bool handshake_write_overflow : 1;
   bool handshake_read_overflow : 1;
 
+  // sending_flight and sending_ack are true if we are in the process of sending
+  // a handshake flight and ACK, respectively.
+  bool sending_flight : 1;
+  bool sending_ack : 1;
+
   uint16_t handshake_write_seq = 0;
   uint16_t handshake_read_seq = 0;
 
@@ -3945,14 +3950,15 @@ bool tls_init_message(const SSL *ssl, CBB *cbb, CBB *body, uint8_t type);
 bool tls_finish_message(const SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg);
 bool tls_add_message(SSL *ssl, Array<uint8_t> msg);
 bool tls_add_change_cipher_spec(SSL *ssl);
-int tls_flush_flight(SSL *ssl, bool post_handshake);
+int tls_flush(SSL *ssl);
 
 bool dtls1_init_message(const SSL *ssl, CBB *cbb, CBB *body, uint8_t type);
 bool dtls1_finish_message(const SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg);
 bool dtls1_add_message(SSL *ssl, Array<uint8_t> msg);
 bool dtls1_add_change_cipher_spec(SSL *ssl);
-int dtls1_flush_flight(SSL *ssl, bool post_handshake);
-int dtls1_send_ack(SSL *ssl);
+void dtls1_finish_flight(SSL *ssl);
+void dtls1_schedule_ack(SSL *ssl);
+int dtls1_flush(SSL *ssl);
 
 // ssl_add_message_cbb finishes the handshake message in |cbb| and adds it to
 // the pending flight. It returns true on success and false on error.
@@ -3980,13 +3986,19 @@ int dtls1_write_app_data(SSL *ssl, bool *out_needs_handshake,
 int dtls1_write_record(SSL *ssl, int type, Span<const uint8_t> in,
                        uint16_t epoch);
 
-int dtls1_retransmit_outgoing_messages(SSL *ssl);
 bool dtls1_parse_fragment(CBS *cbs, struct hm_header_st *out_hdr,
                           CBS *out_body);
-bool dtls1_check_timeout_num(SSL *ssl);
 
-void dtls1_start_timer(SSL *ssl);
+// DTLS1_MTU_TIMEOUTS is the maximum number of retransmit timeouts to expire
+// before starting to decrease the MTU.
+#define DTLS1_MTU_TIMEOUTS 2
+
+// DTLS1_MAX_TIMEOUTS is the maximum number of retransmit timeouts to expire
+// before failing the DTLS handshake.
+#define DTLS1_MAX_TIMEOUTS 12
+
 void dtls1_stop_timer(SSL *ssl);
+
 unsigned int dtls1_min_mtu(void);
 
 bool dtls1_new(SSL *ssl);

ssl/s3_both.cc

@@ -280,7 +280,7 @@ bool tls_add_change_cipher_spec(SSL *ssl) {
   return true;
 }
 
-int tls_flush_flight(SSL *ssl, bool post_handshake) {
+int tls_flush(SSL *ssl) {
   if (!tls_flush_pending_hs_data(ssl)) {
     return -1;
   }
@@ -301,15 +301,6 @@ int tls_flush_flight(SSL *ssl, bool post_handshake) {
     return 1;
   }
 
-  if (post_handshake) {
-    // Don't flush post-handshake messages like NewSessionTicket until the
-    // server performs a write, to prevent a non-reading client from causing the
-    // server to hang in the case of a small server write buffer. Consumers
-    // which don't write data to the client will need to do a zero-byte write if
-    // they wish to flush the tickets.
-    return 1;
-  }
-
   if (ssl->s3->write_shutdown != ssl_shutdown_none) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN);
     return -1;

ssl/ssl_lib.cc

@@ -963,6 +963,15 @@ static int ssl_read_impl(SSL *ssl) {
       return -1;
     }
 
+    // If a read triggered a DTLS ACK or retransmit, resolve that before reading
+    // more.
+    if (SSL_is_dtls(ssl)) {
+      int ret = ssl->method->flush(ssl);
+      if (ret <= 0) {
+        return ret;
+      }
+    }
+
     // Complete the current handshake, if any. False Start will cause
     // |SSL_do_handshake| to return mid-handshake, so this may require multiple
     // iterations.

ssl/ssl_test.cc

@@ -9782,5 +9782,71 @@ TEST(SSLTest, IDOnlyTLS13Session) {
   EXPECT_FALSE(SSL_SESSION_is_resumable(session.get()));
 }
 
+TEST(SSLTest, DTLSReadTimeoutExpired) {
+  bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(DTLS_method()));
+  ASSERT_TRUE(ctx);
+
+  // Mock the clock.
+  g_current_time.tv_sec = 1000;
+  SSL_CTX_set_current_time_cb(ctx.get(), CurrentTimeCallback);
+  auto advance = [](timeval delta) {
+    g_current_time.tv_sec += delta.tv_sec;
+    g_current_time.tv_usec += delta.tv_usec;
+    if (g_current_time.tv_usec >= 1000000) {
+      g_current_time.tv_usec -= 1000000;
+      g_current_time.tv_sec++;
+    }
+  };
+
+  // Create a client and don't connect it to anything.
+  bssl::UniquePtr<SSL> client(SSL_new(ctx.get()));
+  ASSERT_TRUE(client);
+  SSL_set_connect_state(client.get());
+  bssl::UniquePtr<BIO> rbio(BIO_new(BIO_s_mem()));
+  ASSERT_TRUE(rbio);
+  SSL_set0_rbio(client.get(), rbio.release());
+  bssl::UniquePtr<BIO> wbio(BIO_new(BIO_s_mem()));
+  ASSERT_TRUE(wbio);
+  SSL_set0_wbio(client.get(), wbio.release());
+
+  // Write the ClientHello and wait for a ServerHello.
+  EXPECT_EQ(SSL_do_handshake(client.get()), -1);
+  EXPECT_EQ(SSL_get_error(client.get(), -1), SSL_ERROR_WANT_READ);
+
+  for (;;) {
+    // There should be a retransmit timer.
+    timeval timeout;
+    ASSERT_TRUE(DTLSv1_get_timeout(client.get(), &timeout));
+    EXPECT_TRUE(timeout.tv_sec != 0 || timeout.tv_usec != 0);
+
+    // Retransmit. At some point, the client will give up and fail.
+    advance(timeout);
+    int ret = DTLSv1_handle_timeout(client.get());
+    if (ret < 0) {
+      break;
+    }
+    ASSERT_EQ(ret, 1);
+  }
+
+  // The retransmit should have failed with |SSL_R_READ_TIMEOUT_EXPIRED|.
+  EXPECT_EQ(SSL_get_error(client.get(), -1), SSL_ERROR_SSL);
+  EXPECT_TRUE(
+      ErrorEquals(ERR_get_error(), ERR_LIB_SSL, SSL_R_READ_TIMEOUT_EXPIRED));
+
+  // There should not continue to be a timeout. Otherwise, a caller that forgets
+  // to check |DTLSv1_handle_timeout|'s error will infinite loop. See
+  // https://crbug.com/42224241.
+  timeval timeout;
+  EXPECT_FALSE(DTLSv1_get_timeout(client.get(), &timeout));
+
+  // The error should also be returned from |SSL_do_handshake|. This ensures
+  // that, if the caller missed the return from |DTLSv1_handle_timeout|, it will
+  // be picked up from a more normal codepath.
+  EXPECT_EQ(SSL_do_handshake(client.get()), -1);
+  EXPECT_EQ(SSL_get_error(client.get(), -1), SSL_ERROR_SSL);
+  EXPECT_TRUE(
+      ErrorEquals(ERR_get_error(), ERR_LIB_SSL, SSL_R_READ_TIMEOUT_EXPIRED));
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

ssl/test/async_bio.cc

@@ -29,7 +29,6 @@ extern const BIO_METHOD g_async_bio_method;
 
 struct AsyncBio {
   bool datagram;
-  bool enforce_write_quota;
   size_t read_quota;
   size_t write_quota;
 };
@@ -47,10 +46,6 @@ static int AsyncWrite(BIO *bio, const char *in, int inl) {
     return 0;
   }
 
-  if (!a->enforce_write_quota) {
-    return BIO_write(bio->next_bio, in, inl);
-  }
-
   BIO_clear_retry_flags(bio);
 
   if (a->write_quota == 0) {
@@ -112,7 +107,6 @@ static int AsyncNew(BIO *bio) {
   if (a == NULL) {
     return 0;
   }
-  a->enforce_write_quota = true;
   bio->init = 1;
   bio->ptr = (char *)a;
   return 1;
@@ -180,11 +174,3 @@ void AsyncBioAllowWrite(BIO *bio, size_t count) {
   }
   a->write_quota += count;
 }
-
-void AsyncBioEnforceWriteQuota(BIO *bio, bool enforce) {
-  AsyncBio *a = GetData(bio);
-  if (a == NULL) {
-    return;
-  }
-  a->enforce_write_quota = enforce;
-}

ssl/test/async_bio.h

@@ -36,8 +36,5 @@ void AsyncBioAllowRead(BIO *bio, size_t count);
 // AsyncBioAllowWrite increments |bio|'s write quota by |count|.
 void AsyncBioAllowWrite(BIO *bio, size_t count);
 
-// AsyncBioEnforceWriteQuota configures where |bio| enforces its write quota.
-void AsyncBioEnforceWriteQuota(BIO *bio, bool enforce);
-
 
 #endif  // HEADER_ASYNC_BIO

ssl/test/bssl_shim.cc

@@ -221,19 +221,10 @@ static int DoRead(SSL *ssl, uint8_t *out, size_t max_out) {
   }
   int ret;
   do {
-    if (config->async) {
-      // The DTLS retransmit logic silently ignores write failures. So the test
-      // may progress, allow writes through synchronously. |SSL_read| may
-      // trigger a retransmit, so disconnect the write quota.
-      AsyncBioEnforceWriteQuota(test_state->async_bio, false);
-    }
     ret = CheckIdempotentError("SSL_peek/SSL_read", ssl, [&]() -> int {
       return config->peek_then_read ? SSL_peek(ssl, out, max_out)
                                     : SSL_read(ssl, out, max_out);
     });
-    if (config->async) {
-      AsyncBioEnforceWriteQuota(test_state->async_bio, true);
-    }
 
     // Run the exporter after each read. This is to test that the exporter fails
     // during a renegotiation.

ssl/test/handshake_util.cc

@@ -64,17 +64,11 @@ bool RetryAsync(SSL *ssl, int ret) {
 
   if (test_state->packeted_bio != nullptr &&
       PacketedBioAdvanceClock(test_state->packeted_bio)) {
-    // The DTLS retransmit logic silently ignores write failures. So the test
-    // may progress, allow writes through synchronously.
-    AsyncBioEnforceWriteQuota(test_state->async_bio, false);
     int timeout_ret = DTLSv1_handle_timeout(ssl);
-    AsyncBioEnforceWriteQuota(test_state->async_bio, true);
-
-    if (timeout_ret < 0) {
-      fprintf(stderr, "Error retransmitting.\n");
-      return false;
+    if (timeout_ret >= 0) {
+      return true;
     }
-    return true;
+    ssl_err = SSL_get_error(ssl, timeout_ret);
   }
 
   // See if we needed to read or write more. If so, allow one byte through on
@@ -96,8 +90,9 @@ bool RetryAsync(SSL *ssl, int ret) {
       test_state->early_callback_ready = true;
       return true;
     case SSL_ERROR_WANT_PRIVATE_KEY_OPERATION:
+      test_state->private_key_retries++;
       if (config->private_key_delay_ms != 0 &&
-          test_state->private_key_retries == 0) {
+          test_state->private_key_retries == 1) {
         // The first time around, simulate the private key operation taking a
         // long time to run.
         if (test_state->packeted_bio == nullptr) {
@@ -111,15 +106,15 @@ bool RetryAsync(SSL *ssl, int ret) {
           clock->tv_usec -= 1000000;
           clock->tv_sec++;
         }
-        AsyncBioEnforceWriteQuota(test_state->async_bio, false);
         int timeout_ret = DTLSv1_handle_timeout(ssl);
-        AsyncBioEnforceWriteQuota(test_state->async_bio, true);
         if (timeout_ret < 0) {
-          fprintf(stderr, "Error retransmitting.\n");
+          if (SSL_get_error(ssl, timeout_ret) == SSL_ERROR_WANT_WRITE) {
+            AsyncBioAllowWrite(test_state->async_bio, 1);
+            return true;
+          }
           return false;
         }
       }
-      test_state->private_key_retries++;
       return true;
     case SSL_ERROR_WANT_CERTIFICATE_VERIFY:
       test_state->custom_verify_ready = true;

ssl/tls13_both.cc

@@ -633,7 +633,7 @@ bool tls13_add_key_update(SSL *ssl, int update_requested) {
   // wire. This prevents us from accumulating write obligations when read and
   // write progress at different rates. See RFC 8446, section 4.6.3.
   ssl->s3->key_update_pending = true;
-
+  ssl->method->finish_flight(ssl);
   return true;
 }
 

ssl/tls13_server.cc

@@ -1274,17 +1274,29 @@ static enum ssl_hs_wait_t do_read_client_finished(SSL_HANDSHAKE *hs) {
   }
 
   ssl->method->next_message(ssl);
-  return ssl_hs_ack;
+  if (SSL_is_dtls(ssl)) {
+    ssl->method->schedule_ack(ssl);
+    return ssl_hs_flush;
+  }
+  return ssl_hs_ok;
 }
 
 static enum ssl_hs_wait_t do_send_new_session_ticket(SSL_HANDSHAKE *hs) {
+  SSL *const ssl = hs->ssl;
   bool sent_tickets;
   if (!add_new_session_tickets(hs, &sent_tickets)) {
     return ssl_hs_error;
   }
 
   hs->tls13_state = state13_done;
-  return sent_tickets ? ssl_hs_flush_post_handshake : ssl_hs_ok;
+  // In QUIC and DTLS, we can flush the ticket to the transport immediately. In
+  // TLS over TCP-like transports, we defer until the server performs a write.
+  // This prevents a non-reading client from causing the server to hang in the
+  // case of a small server write buffer. Consumers which don't write data to
+  // the client will need to do a zero-byte write if they wish to flush the
+  // tickets.
+  bool should_flush = sent_tickets && (SSL_is_dtls(ssl) || SSL_is_quic(ssl));
+  return should_flush ? ssl_hs_flush : ssl_hs_ok;
 }
 
 enum ssl_hs_wait_t tls13_server_handshake(SSL_HANDSHAKE *hs) {

ssl/tls_method.cc

@@ -143,6 +143,15 @@ static bool tls_set_write_state(SSL *ssl, ssl_encryption_level_t level,
   return true;
 }
 
+static void tls_finish_flight(SSL *ssl) {
+  // We don't track whether a flight is complete in TLS and instead always flush
+  // every queued message in |tls_flush|, whether the flight is complete or not.
+}
+
+static void tls_schedule_ack(SSL *ssl) {
+  // TLS does not use ACKs.
+}
+
 static const SSL_PROTOCOL_METHOD kTLSProtocolMethod = {
     false /* is_dtls */,
     tls_new,
@@ -159,8 +168,9 @@ static const SSL_PROTOCOL_METHOD kTLSProtocolMethod = {
     tls_finish_message,
     tls_add_message,
     tls_add_change_cipher_spec,
-    tls_flush_flight,
-    /*send_ack=*/nullptr,
+    tls_finish_flight,
+    tls_schedule_ack,
+    tls_flush,
     tls_on_handshake_complete,
     tls_set_read_state,
     tls_set_write_state,