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>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 17b5d0e..e7c57c7 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -623,7 +623,8 @@ // 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 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 @@ // // 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);
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index 838c21b..db51c9a 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc
@@ -446,7 +446,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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; }
diff --git a/ssl/d1_lib.cc b/ssl/d1_lib.cc index 520bde3..5cca499 100644 --- a/ssl/d1_lib.cc +++ b/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 @@ 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 @@ 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); }
diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index 7bdeca1..b4d4c67 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc
@@ -261,6 +261,10 @@ // 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 @@ 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; }
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc index 54bac2f..de600bb 100644 --- a/ssl/dtls_method.cc +++ b/ssl/dtls_method.cc
@@ -195,8 +195,9 @@ 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,
diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 968fe1b..0b5db42 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc
@@ -589,6 +589,16 @@ 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 @@ 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 @@ 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 @@ 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 @@ *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. } }
diff --git a/ssl/internal.h b/ssl/internal.h index 9721cb1..463852b 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2128,7 +2128,6 @@ 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 @@ 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 @@ // 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 @@ 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_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_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);
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc index f456ad7..82671c0 100644 --- a/ssl/s3_both.cc +++ b/ssl/s3_both.cc
@@ -280,7 +280,7 @@ 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 @@ 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;
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index f7d0263..379e628 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -963,6 +963,15 @@ 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.
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index a47660d..055015f 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -9782,5 +9782,71 @@ 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
diff --git a/ssl/test/async_bio.cc b/ssl/test/async_bio.cc index 1c9859a..a296715 100644 --- a/ssl/test/async_bio.cc +++ b/ssl/test/async_bio.cc
@@ -29,7 +29,6 @@ struct AsyncBio { bool datagram; - bool enforce_write_quota; size_t read_quota; size_t write_quota; }; @@ -47,10 +46,6 @@ 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 @@ if (a == NULL) { return 0; } - a->enforce_write_quota = true; bio->init = 1; bio->ptr = (char *)a; return 1; @@ -180,11 +174,3 @@ } a->write_quota += count; } - -void AsyncBioEnforceWriteQuota(BIO *bio, bool enforce) { - AsyncBio *a = GetData(bio); - if (a == NULL) { - return; - } - a->enforce_write_quota = enforce; -}
diff --git a/ssl/test/async_bio.h b/ssl/test/async_bio.h index 9974139..7ab764e 100644 --- a/ssl/test/async_bio.h +++ b/ssl/test/async_bio.h
@@ -36,8 +36,5 @@ // 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
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 5a17c5c..aada04f 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -221,19 +221,10 @@ } 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.
diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc index d5e43e7..f2c58a6 100644 --- a/ssl/test/handshake_util.cc +++ b/ssl/test/handshake_util.cc
@@ -64,17 +64,11 @@ 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 @@ 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 @@ 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;
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 54bb20c..e0d5ad0 100644 --- a/ssl/tls13_both.cc +++ b/ssl/tls13_both.cc
@@ -633,7 +633,7 @@ // 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; }
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index b783b0a..8b6683e 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -1274,17 +1274,29 @@ } 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) {
diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc index 0861d86..33c3a7a 100644 --- a/ssl/tls_method.cc +++ b/ssl/tls_method.cc
@@ -143,6 +143,15 @@ 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 @@ 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,