HelloRetryRequest getter Adds getter indicating whether HelloRetryRequest was triggered during TLSv1.3 handshake. Change-Id: I84922188ded81ec89259b5f333c80494426759f8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37304 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index be83edf..b0ee69a 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3946,6 +3946,11 @@ // mechanism would have aborted |ssl|'s handshake and zero otherwise. OPENSSL_EXPORT int SSL_is_tls13_downgrade(const SSL *ssl); +// SSL_used_hello_retry_request returns one if the TLS 1.3 HelloRetryRequest +// message has been either sent by the server or received by the client. It +// returns zero otherwise. +OPENSSL_EXPORT int SSL_used_hello_retry_request(const SSL *ssl); + // SSL_set_jdk11_workaround configures whether to workaround various bugs in // JDK 11's TLS 1.3 implementation by disabling TLS 1.3 for such clients. //
diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 6791d2a..707c6d2 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc
@@ -128,8 +128,6 @@ : ssl(ssl_arg), scts_requested(false), needs_psk_binder(false), - received_hello_retry_request(false), - sent_hello_retry_request(false), handshake_finalized(false), accept_psk_mode(false), cert_request(false),
diff --git a/ssl/internal.h b/ssl/internal.h index 7f163a4..5f81b22 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1693,9 +1693,6 @@ // be filled in. bool needs_psk_binder : 1; - bool received_hello_retry_request : 1; - bool sent_hello_retry_request : 1; - // handshake_finalized is true once the handshake has completed, at which // point accessors should use the established state. bool handshake_finalized : 1; @@ -2387,6 +2384,10 @@ // HelloRequest. bool renegotiate_pending : 1; + // used_hello_retry_request is whether the handshake used a TLS 1.3 + // HelloRetryRequest message. + bool used_hello_retry_request : 1; + // hs_buf is the buffer of handshake data to process. UniquePtr<BUF_MEM> hs_buf;
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index f1f0ec7..6b0635b 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc
@@ -181,7 +181,8 @@ token_binding_negotiated(false), pq_experiment_signal_seen(false), alert_dispatch(false), - renegotiate_pending(false) {} + renegotiate_pending(false), + used_hello_retry_request(false) {} SSL3_STATE::~SSL3_STATE() {}
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 1af8506..1daf03f 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -2859,6 +2859,10 @@ int SSL_is_tls13_downgrade(const SSL *ssl) { return ssl->s3->tls13_downgrade; } +int SSL_used_hello_retry_request(const SSL *ssl) { + return ssl->s3->used_hello_retry_request; +} + void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, int ignore) { ctx->ignore_tls13_downgrade = !!ignore; }
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index e5a33dd..298dc9b 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc
@@ -1845,7 +1845,7 @@ // Per RFC 8446 section 4.1.4, skip offering the session if the selected // cipher in HelloRetryRequest does not match. This avoids performing the // transcript hash transformation for multiple hashes. - if (hs->received_hello_retry_request && + if (ssl->s3 && ssl->s3->used_hello_retry_request && ssl->session->cipher->algorithm_prf != hs->new_cipher->algorithm_prf) { return true; } @@ -2035,7 +2035,7 @@ SSL *const ssl = hs->ssl; // The second ClientHello never offers early data, and we must have already // filled in |early_data_reason| by this point. - if (hs->received_hello_retry_request) { + if (ssl->s3->used_hello_retry_request) { assert(ssl->s3->early_data_reason != ssl_early_data_unknown); return true; } @@ -2089,7 +2089,7 @@ CBS *contents) { SSL *const ssl = hs->ssl; if (contents == NULL) { - if (hs->early_data_offered && !hs->received_hello_retry_request) { + if (hs->early_data_offered && !ssl->s3->used_hello_retry_request) { ssl->s3->early_data_reason = ssl->s3->session_reused ? ssl_early_data_peer_declined : ssl_early_data_session_not_resumed; @@ -2104,7 +2104,7 @@ // If we received an HRR, the second ClientHello never offers early data, so // the extensions logic will automatically reject early data extensions as // unsolicited. This covered by the ServerAcceptsEarlyDataOnHRR test. - assert(!hs->received_hello_retry_request); + assert(!ssl->s3->used_hello_retry_request); if (CBS_len(contents) != 0) { *out_alert = SSL_AD_DECODE_ERROR; @@ -2173,7 +2173,7 @@ uint16_t group_id = hs->retry_group; uint16_t second_group_id = 0; - if (hs->received_hello_retry_request) { + if (ssl->s3 && ssl->s3->used_hello_retry_request) { // We received a HelloRetryRequest without a new curve, so there is no new // share to append. Leave |hs->key_share| as-is. if (group_id == 0 && @@ -2235,7 +2235,7 @@ // Save the contents of the extension to repeat it in the second // ClientHello. - if (!hs->received_hello_retry_request && + if (ssl->s3 && !ssl->s3->used_hello_retry_request && !hs->key_share_bytes.CopyFrom( MakeConstSpan(CBB_data(&kse_bytes), CBB_len(&kse_bytes)))) { return false;
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 61de136..5748fb9 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -676,6 +676,12 @@ return false; } + if ((config->expect_hrr && !SSL_used_hello_retry_request(ssl)) || + (config->expect_no_hrr && SSL_used_hello_retry_request(ssl))) { + fprintf(stderr, "Got %sHRR, but wanted opposite.\n", + SSL_used_hello_retry_request(ssl) ? "" : "no "); + return false; + } return true; }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index a4f787a..e78e9a2 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -145,12 +145,12 @@ type CurveID uint16 const ( - CurveP224 CurveID = 21 - CurveP256 CurveID = 23 - CurveP384 CurveID = 24 - CurveP521 CurveID = 25 - CurveX25519 CurveID = 29 - CurveCECPQ2 CurveID = 16696 + CurveP224 CurveID = 21 + CurveP256 CurveID = 23 + CurveP384 CurveID = 24 + CurveP521 CurveID = 25 + CurveX25519 CurveID = 29 + CurveCECPQ2 CurveID = 16696 ) // TLS Elliptic Curve Point Formats
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 18b01aa..660be0a 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -4383,6 +4383,7 @@ resumeSession: true, // Ensure session tickets are used, not session IDs. noSessionCache: true, + flags: []string{"-expect-no-hrr"}, }) tests = append(tests, testCase{ name: "Basic-Client-RenewTicket", @@ -4414,7 +4415,10 @@ }, }, resumeSession: true, - flags: []string{"-expect-no-session-id"}, + flags: []string{ + "-expect-no-session-id", + "-expect-no-hrr", + }, }) tests = append(tests, testCase{ testType: serverTest, @@ -4482,6 +4486,7 @@ }, // Cover HelloRetryRequest during an ECDHE-PSK resumption. resumeSession: true, + flags: []string{"-expect-hrr"}, }) tests = append(tests, testCase{ @@ -4495,6 +4500,7 @@ }, // Cover HelloRetryRequest during an ECDHE-PSK resumption. resumeSession: true, + flags: []string{"-expect-hrr"}, }) tests = append(tests, testCase{ @@ -12538,6 +12544,7 @@ CurvePreferences: []CurveID{CurveX25519}, }, expectedCurveID: CurveX25519, + flags: []string{"-expect-hrr"}, }) testCases = append(testCases, testCase{ @@ -12551,6 +12558,7 @@ // Although the ClientHello did not predict our preferred curve, // we always select it whether it is predicted or not. expectedCurveID: CurveX25519, + flags: []string{"-expect-hrr"}, }) testCases = append(testCases, testCase{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 23de5e9..a4a37e6 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -153,6 +153,8 @@ &TestConfig::expect_delegated_credential_used}, {"-enable-pq-experiment-signal", &TestConfig::enable_pq_experiment_signal}, {"-expect-pq-experiment-signal", &TestConfig::expect_pq_experiment_signal}, + {"-expect-hrr", &TestConfig::expect_hrr}, + {"-expect-no-hrr", &TestConfig::expect_no_hrr}, }; const Flag<std::string> kStringFlags[] = {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 8c25ed2..b94ca10 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -178,6 +178,8 @@ std::string expect_early_data_reason; bool enable_pq_experiment_signal = false; bool expect_pq_experiment_signal = false; + bool expect_hrr = false; + bool expect_no_hrr = false; int argc; char **argv;
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 15c18c2..fa3f3a6 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -184,7 +184,7 @@ } ssl->method->next_message(ssl); - hs->received_hello_retry_request = true; + ssl->s3->used_hello_retry_request = true; hs->tls13_state = state_send_second_client_hello; // 0-RTT is rejected if we receive a HelloRetryRequest. if (hs->in_early_data) { @@ -269,8 +269,7 @@ } // Check that the cipher matches the one in the HelloRetryRequest. - if (hs->received_hello_retry_request && - hs->new_cipher != cipher) { + if (ssl->s3->used_hello_retry_request && hs->new_cipher != cipher) { OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CIPHER_RETURNED); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index abacefc..d8115f5 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -515,7 +515,7 @@ return ssl_hs_error; } - hs->sent_hello_retry_request = true; + ssl->s3->used_hello_retry_request = true; hs->tls13_state = state_read_second_client_hello; return ssl_hs_flush; } @@ -612,7 +612,7 @@ return ssl_hs_error; } - if (!hs->sent_hello_retry_request && + if (!ssl->s3->used_hello_retry_request && !ssl->method->add_change_cipher_spec(ssl)) { return ssl_hs_error; }