diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 17b5d0ee1..e7c57c708 100644 --- a/include/openssl/ssl.h +++ b/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); diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index 838c21b53..db51c9a36 100644 --- a/ssl/d1_both.cc +++ b/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 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; } diff --git a/ssl/d1_lib.cc b/ssl/d1_lib.cc index 520bde3e0..5cca499bb 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 @@ 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); } diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index 7bdeca18c..b4d4c6782 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc @@ -261,6 +261,10 @@ ssl_open_record_t dtls1_open_app_data(SSL *ssl, Span *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 *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; } diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc index 54bac2fc3..de600bb08 100644 --- a/ssl/dtls_method.cc +++ b/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, diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 968fe1b66..0b5db42ac 100644 --- a/ssl/handshake.cc +++ b/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. } } diff --git a/ssl/internal.h b/ssl/internal.h index 9721cb1fb..463852b48 100644 --- a/ssl/internal.h +++ b/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 *out_msg); bool tls_add_message(SSL *ssl, Array 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 *out_msg); bool dtls1_add_message(SSL *ssl, Array 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 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 f456ad781..82671c029 100644 --- a/ssl/s3_both.cc +++ b/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; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index f7d0263f9..379e628da 100644 --- a/ssl/ssl_lib.cc +++ b/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. diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index a47660d61..055015fd6 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -9782,5 +9782,71 @@ TEST(SSLTest, IDOnlyTLS13Session) { EXPECT_FALSE(SSL_SESSION_is_resumable(session.get())); } +TEST(SSLTest, DTLSReadTimeoutExpired) { + bssl::UniquePtr 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 client(SSL_new(ctx.get())); + ASSERT_TRUE(client); + SSL_set_connect_state(client.get()); + bssl::UniquePtr rbio(BIO_new(BIO_s_mem())); + ASSERT_TRUE(rbio); + SSL_set0_rbio(client.get(), rbio.release()); + bssl::UniquePtr 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 1c9859afe..a2967159c 100644 --- a/ssl/test/async_bio.cc +++ b/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; -} diff --git a/ssl/test/async_bio.h b/ssl/test/async_bio.h index 9974139d9..7ab764e38 100644 --- a/ssl/test/async_bio.h +++ b/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 diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 5a17c5cc7..aada04fcc 100644 --- a/ssl/test/bssl_shim.cc +++ b/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. diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc index d5e43e7fd..f2c58a646 100644 --- a/ssl/test/handshake_util.cc +++ b/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; diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 54bb20c69..e0d5ad0ba 100644 --- a/ssl/tls13_both.cc +++ b/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; } diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index b783b0aef..8b6683eeb 100644 --- a/ssl/tls13_server.cc +++ b/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) { diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc index 0861d8687..33c3a7a08 100644 --- a/ssl/tls_method.cc +++ b/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,