From d274b1bacdca36f3941bf78e43dc38acf676a1a8 Mon Sep 17 00:00:00 2001 From: Rushil Mehra Date: Thu, 6 Jun 2024 10:07:43 -0700 Subject: [PATCH] Add ECH fallback API This commit solves https://bugs.chromium.org/p/boringssl/issues/detail?id=714. To summarize, there are cases where servers will advertise ECH on hostnames that may, in practice, be unable to actually negotiate e.g. TLS 1.3. To gracefully handle this case, this commit adds a new return value for the select_cert_cb that signals to the server that ECH must be disabled. To accomplish this, we slightly rewind the state machine to instead handshake with ClientHelloOuter, and clear ech_keys on the handshake state such that the server hello does not include any retry_configs in EncryptedExtensions. Clients will take this as a signal that ECH is disabled on the hostname, and that they should instead handshake without ECH. Bug: 42290593 Change-Id: I1806ba052ffbc3e5c46161a1596d125cc5e5a8fc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69087 Reviewed-by: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck --- include/openssl/ssl.h | 10 ++++++ ssl/handshake_server.cc | 16 ++++++++- ssl/test/runner/common.go | 3 ++ ssl/test/runner/handshake_client.go | 4 +++ ssl/test/runner/runner.go | 50 +++++++++++++++++++++++++++++ ssl/test/test_config.cc | 21 ++++++++++-- ssl/test/test_config.h | 2 ++ 7 files changed, 103 insertions(+), 3 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 9f95a06bb..10898a9c3 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4617,6 +4617,16 @@ enum ssl_select_cert_result_t BORINGSSL_ENUM_INT { // ssl_select_cert_error indicates that a fatal error occured and the // handshake should be terminated. ssl_select_cert_error = -1, + // ssl_select_cert_disable_ech indicates that, although an encrypted + // ClientHelloInner was decrypted, it should be discarded. The certificate + // selection callback will then be called again, passing in the + // ClientHelloOuter instead. From there, the handshake will proceed + // without retry_configs, to signal to the client to disable ECH. + // + // This value may only be returned when |SSL_ech_accepted| returnes one. It + // may be useful if the ClientHelloInner indicated a service which does not + // support ECH, e.g. if it is a TLS-1.2 only service. + ssl_select_cert_disable_ech = -2, }; // SSL_early_callback_ctx_extension_get searches the extensions in diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index afa0927f3..508f9bf6f 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -613,6 +613,10 @@ static bool extract_sni(SSL_HANDSHAKE *hs, uint8_t *out_alert, if (!ssl_client_hello_get_extension(client_hello, &sni, TLSEXT_TYPE_server_name)) { // No SNI extension to parse. + // + // Clear state in case we previously extracted SNI from ClientHelloOuter. + ssl->s3->hostname.reset(); + hs->should_ack_sni = false; return true; } @@ -686,7 +690,10 @@ static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) { } uint8_t alert = SSL_AD_DECODE_ERROR; - if (!decrypt_ech(hs, &alert, &client_hello)) { + // We check for rejection status in case we've rewound the state machine after + // determining `ClientHelloInner` is invalid. + if (ssl->s3->ech_status != ssl_ech_rejected && + !decrypt_ech(hs, &alert, &client_hello)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -722,6 +729,13 @@ static enum ssl_hs_wait_t do_read_client_hello_after_ech(SSL_HANDSHAKE *hs) { case ssl_select_cert_retry: return ssl_hs_certificate_selection_pending; + case ssl_select_cert_disable_ech: + hs->ech_client_hello_buf.Reset(); + hs->ech_keys = nullptr; + hs->state = state12_read_client_hello; + ssl->s3->ech_status = ssl_ech_rejected; + return ssl_hs_ok; + case ssl_select_cert_error: // Connection rejected. OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_REJECTED); diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 3a8caa1e7..6bdd664dc 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1965,6 +1965,9 @@ type ProtocolBugs struct { // EncryptSessionTicketKey, if non-nil, is the ticket key to use when // encrypting tickets. EncryptSessionTicketKey *[32]byte + + // OmitPublicName omits the server name extension from ClientHelloOuter. + OmitPublicName bool } func (c *Config) serverInit() { diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index a0a81f999..a7ab0e2c8 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -583,6 +583,10 @@ func (hs *clientHandshakeState) createClientHello(innerHello *clientHelloMsg, ec hello.serverName = c.config.ServerName } + if !isInner && c.config.Bugs.OmitPublicName { + hello.serverName = "" + } + disableEMS := c.config.Bugs.NoExtendedMasterSecret if c.cipherSuite != nil { disableEMS = c.config.Bugs.NoExtendedMasterSecretOnRenegotiation diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 3aab605c7..18ed78ba5 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -17343,6 +17343,56 @@ write hs 4 }, }) + // Test that we successfully rewind the TLS state machine and disable ECH in the + // case that the select_cert_cb signals that ECH is not possible for the SNI in + // ClientHelloInner. + testCases = append(testCases, testCase{ + testType: serverTest, + protocol: protocol, + name: prefix + "ECH-Server-FailCallbackNeedRewind", + config: Config{ + ServerName: "secret.example", + ClientECHConfig: echConfig.ECHConfig, + }, + flags: []string{ + "-async", + "-fail-early-callback-ech-rewind", + "-ech-server-config", base64FlagValue(echConfig.ECHConfig.Raw), + "-ech-server-key", base64FlagValue(echConfig.Key), + "-ech-is-retry-config", "1", + "-expect-server-name", "public.example", + }, + expectations: connectionExpectations{ + echAccepted: false, + }, + }) + + // Test that we correctly handle falling back to a ClientHelloOuter with + // no SNI (public name). + testCases = append(testCases, testCase{ + testType: serverTest, + protocol: protocol, + name: prefix + "ECH-Server-RewindWithNoPublicName", + config: Config{ + ServerName: "secret.example", + ClientECHConfig: echConfig.ECHConfig, + Bugs: ProtocolBugs{ + OmitPublicName: true, + }, + }, + flags: []string{ + "-async", + "-fail-early-callback-ech-rewind", + "-ech-server-config", base64FlagValue(echConfig.ECHConfig.Raw), + "-ech-server-key", base64FlagValue(echConfig.Key), + "-ech-is-retry-config", "1", + "-expect-no-server-name", + }, + expectations: connectionExpectations{ + echAccepted: false, + }, + }) + // Test ECH-enabled server with two ECHConfigs can decrypt client's ECH when // it uses the second ECHConfig. testCases = append(testCases, testCase{ diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 24b1bdd4d..dfe1cb462 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -354,6 +354,7 @@ const Flag *FindFlag(const char *name) { BoolFlag("-implicit-handshake", &TestConfig::implicit_handshake), BoolFlag("-use-early-callback", &TestConfig::use_early_callback), BoolFlag("-fail-early-callback", &TestConfig::fail_early_callback), + BoolFlag("-fail-early-callback-ech-rewind", &TestConfig::fail_early_callback_ech_rewind), BoolFlag("-install-ddos-callback", &TestConfig::install_ddos_callback), BoolFlag("-fail-ddos-callback", &TestConfig::fail_ddos_callback), BoolFlag("-fail-cert-callback", &TestConfig::fail_cert_callback), @@ -374,6 +375,8 @@ const Flag *FindFlag(const char *name) { &TestConfig::expect_reject_early_data), BoolFlag("-expect-no-offer-early-data", &TestConfig::expect_no_offer_early_data), + BoolFlag("-expect-no-server-name", + &TestConfig::expect_no_server_name), BoolFlag("-use-ticket-callback", &TestConfig::use_ticket_callback), BoolFlag("-renew-ticket", &TestConfig::renew_ticket), BoolFlag("-enable-early-data", &TestConfig::enable_early_data), @@ -1581,9 +1584,23 @@ static enum ssl_select_cert_result_t SelectCertificateCallback( TestState *test_state = GetTestState(ssl); test_state->early_callback_called = true; + // Invoke the rewind before we sanity check SNI because we will + // end up calling the select_cert_cb twice with two different SNIs. + if (SSL_ech_accepted(ssl) && config->fail_early_callback_ech_rewind) { + return ssl_select_cert_disable_ech; + } + + const char *server_name = + SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); + + if (config->expect_no_server_name && server_name != nullptr) { + fprintf(stderr, + "Expected no server name but got %s.\n", + server_name); + return ssl_select_cert_error; + } + if (!config->expect_server_name.empty()) { - const char *server_name = - SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); if (server_name == nullptr || std::string(server_name) != config->expect_server_name) { fprintf(stderr, diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 89656d19e..607f58dc1 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -118,6 +118,7 @@ struct TestConfig { bool implicit_handshake = false; bool use_early_callback = false; bool fail_early_callback = false; + bool fail_early_callback_ech_rewind = false; bool install_ddos_callback = false; bool fail_ddos_callback = false; bool fail_cert_callback = false; @@ -134,6 +135,7 @@ struct TestConfig { bool expect_accept_early_data = false; bool expect_reject_early_data = false; bool expect_no_offer_early_data = false; + bool expect_no_server_name = false; bool use_ticket_callback = false; bool renew_ticket = false; bool enable_early_data = false;