diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index d3efb735b..2f8541095 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -59,6 +59,7 @@ SSL,264,DUPLICATE_KEY_SHARE SSL,296,DUPLICATE_SIGNATURE_ALGORITHM SSL,283,EARLY_DATA_NOT_IN_USE SSL,144,ECC_CERT_NOT_FOR_SIGNING +SSL,319,ECH_REJECTED SSL,310,ECH_SERVER_CONFIG_AND_PRIVATE_KEY_MISMATCH SSL,311,ECH_SERVER_CONFIG_UNSUPPORTED_EXTENSION SSL,313,ECH_SERVER_WOULD_HAVE_NO_RETRY_CONFIGS diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index dc3a79dfb..ed5e64f73 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -3557,6 +3557,11 @@ OPENSSL_EXPORT const char *SSL_early_data_reason_string( // This can prevent observers from seeing cleartext information about the // connection, such as the server_name extension. // +// By default, BoringSSL will treat the server name, session ticket, and client +// certificate as secret, but most other parameters, such as the ALPN protocol +// list will be treated as public and sent in the cleartext ClientHello. Other +// APIs may be added for applications with different secrecy requirements. +// // ECH support in BoringSSL is still experimental and under development. // // See https://tools.ietf.org/html/draft-ietf-tls-esni-10. @@ -3573,16 +3578,57 @@ OPENSSL_EXPORT void SSL_set_enable_ech_grease(SSL *ssl, int enable); // valid but none of the ECHConfigs implement supported parameters, it will // return success and proceed without ECH. // -// WARNING: Client ECH support is still incomplete and does not yet implement -// the recovery flow. It currently treats ECH rejection as a fatal error. Do not -// use this API yet. -// -// TODO(https://crbug.com/boringssl/275): When the recovery flow is implemented, -// fill in the remaining docs. +// If a supported ECHConfig is found, |ssl| will encrypt the true ClientHello +// parameters. If the server cannot decrypt it, e.g. due to a key mismatch, ECH +// has a recovery flow. |ssl| will handshake using the cleartext parameters, +// including a public name in the ECHConfig. If using +// |SSL_CTX_set_custom_verify|, callers should use |SSL_get0_ech_name_override| +// to verify the certificate with the public name. If using the built-in +// verifier, the |X509_STORE_CTX| will be configured automatically. +// +// If no other errors are found in this handshake, it will fail with +// |SSL_R_ECH_REJECTED|. Since it didn't use the true parameters, the connection +// cannot be used for application data. Instead, callers should handle this +// error by calling |SSL_get0_ech_retry_configs| and retrying the connection +// with updated ECH parameters. If the retry also fails with +// |SSL_R_ECH_REJECTED|, the caller should report a connection failure. OPENSSL_EXPORT int SSL_set1_ech_config_list(SSL *ssl, const uint8_t *ech_config_list, size_t ech_config_list_len); +// SSL_get0_ech_name_override sets |*out_name| and |*out_name_len| to point to a +// buffer containing the ECH public name, if the server rejected ECH, or the +// empty string otherwise. +// +// This function should be called during the certificate verification callback +// (see |SSL_CTX_set_custom_verify|) if |ssl| is a client offering ECH. If +// |*out_name_len| is non-zero, the caller should verify the certificate against +// the result, interpreted as a DNS name, rather than the true server name. In +// this case, the handshake will never succeed and is only used to authenticate +// retry configs. See also |SSL_get0_ech_retry_configs|. +OPENSSL_EXPORT void SSL_get0_ech_name_override(const SSL *ssl, + const char **out_name, + size_t *out_name_len); + +// SSL_get0_ech_retry_configs sets |*out_retry_configs| and +// |*out_retry_configs_len| to a buffer containing a serialized ECHConfigList. +// If the server did not provide an ECHConfigList, |*out_retry_configs_len| will +// be zero. +// +// When handling an |SSL_R_ECH_REJECTED| error code as a client, callers should +// use this function to recover from potential key mismatches. If the result is +// non-empty, the caller should retry the connection, passing this buffer to +// |SSL_set1_ech_config_list|. If the result is empty, the server has rolled +// back ECH support, and the caller should retry without ECH. +// +// This function must only be called in response to an |SSL_R_ECH_REJECTED| +// error code. Calling this function on |ssl|s that have not authenticated the +// rejection handshake will assert in debug builds and otherwise return an +// unparsable list. +OPENSSL_EXPORT void SSL_get0_ech_retry_configs( + const SSL *ssl, const uint8_t **out_retry_configs, + size_t *out_retry_configs_len); + // SSL_marshal_ech_config constructs a new serialized ECHConfig. On success, it // sets |*out| to a newly-allocated buffer containing the result and |*out_len| // to the size of the buffer. The caller must call |OPENSSL_free| on |*out| to @@ -5502,6 +5548,7 @@ BSSL_NAMESPACE_END #define SSL_R_COULD_NOT_PARSE_HINTS 316 #define SSL_R_INVALID_ECH_PUBLIC_NAME 317 #define SSL_R_INVALID_ECH_CONFIG_LIST 318 +#define SSL_R_ECH_REJECTED 319 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020 diff --git a/include/openssl/x509.h b/include/openssl/x509.h index ac334625e..beeed9138 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -1820,6 +1820,7 @@ BORINGSSL_MAKE_DELETER(X509_REQ, X509_REQ_free) BORINGSSL_MAKE_DELETER(X509_REVOKED, X509_REVOKED_free) BORINGSSL_MAKE_DELETER(X509_SIG, X509_SIG_free) BORINGSSL_MAKE_DELETER(X509_STORE, X509_STORE_free) +BORINGSSL_MAKE_UP_REF(X509_STORE, X509_STORE_up_ref) BORINGSSL_MAKE_DELETER(X509_STORE_CTX, X509_STORE_CTX_free) BORINGSSL_MAKE_DELETER(X509_VERIFY_PARAM, X509_VERIFY_PARAM_free) diff --git a/ssl/encrypted_client_hello.cc b/ssl/encrypted_client_hello.cc index 041c8de02..e5fabd903 100644 --- a/ssl/encrypted_client_hello.cc +++ b/ssl/encrypted_client_hello.cc @@ -40,7 +40,8 @@ BSSL_NAMESPACE_BEGIN // ECH reuses the extension code point for the version number. -static const uint16_t kECHConfigVersion = TLSEXT_TYPE_encrypted_client_hello; +static constexpr uint16_t kECHConfigVersion = + TLSEXT_TYPE_encrypted_client_hello; static const decltype(&EVP_hpke_aes_128_gcm) kSupportedAEADs[] = { &EVP_hpke_aes_128_gcm, @@ -993,6 +994,47 @@ int SSL_set1_ech_config_list(SSL *ssl, const uint8_t *ech_config_list, return ssl->config->client_ech_config_list.CopyFrom(span); } +void SSL_get0_ech_name_override(const SSL *ssl, const char **out_name, + size_t *out_name_len) { + // When ECH is rejected, we use the public name. Note that, if + // |SSL_CTX_set_reverify_on_resume| is enabled, we reverify the certificate + // before the 0-RTT point. If also offering ECH, we verify as if + // ClientHelloInner was accepted and do not override. This works because, at + // this point, |ech_status| will be |ssl_ech_none|. See the + // ECH-Client-Reject-EarlyDataReject-OverrideNameOnRetry tests in runner.go. + const SSL_HANDSHAKE *hs = ssl->s3->hs.get(); + if (hs && ssl->s3->ech_status == ssl_ech_rejected) { + *out_name = reinterpret_cast( + hs->selected_ech_config->public_name.data()); + *out_name_len = hs->selected_ech_config->public_name.size(); + } else { + *out_name = nullptr; + *out_name_len = 0; + } +} + +void SSL_get0_ech_retry_configs( + const SSL *ssl, const uint8_t **out_retry_configs, + size_t *out_retry_configs_len) { + const SSL_HANDSHAKE *hs = ssl->s3->hs.get(); + if (!hs || !hs->ech_authenticated_reject) { + // It is an error to call this function except in response to + // |SSL_R_ECH_REJECTED|. Returning an empty string risks the caller + // mistakenly believing the server has disabled ECH. Instead, return a + // non-empty ECHConfigList with a syntax error, so the subsequent + // |SSL_set1_ech_config_list| call will fail. + assert(0); + static const uint8_t kPlaceholder[] = { + kECHConfigVersion >> 8, kECHConfigVersion & 0xff, 0xff, 0xff, 0xff}; + *out_retry_configs = kPlaceholder; + *out_retry_configs_len = sizeof(kPlaceholder); + return; + } + + *out_retry_configs = hs->ech_retry_configs.data(); + *out_retry_configs_len = hs->ech_retry_configs.size(); +} + int SSL_marshal_ech_config(uint8_t **out, size_t *out_len, uint8_t config_id, const EVP_HPKE_KEY *key, const char *public_name, size_t max_name_len) { @@ -1129,5 +1171,5 @@ int SSL_ech_accepted(const SSL *ssl) { return ssl->s3->hs->selected_ech_config != nullptr; } - return ssl->s3->ech_accept; + return ssl->s3->ech_status == ssl_ech_accepted; } diff --git a/ssl/extensions.cc b/ssl/extensions.cc index 69a2bdc1a..4950d2663 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc @@ -654,6 +654,11 @@ static bool ext_ech_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert, return false; } + if (!ssl_is_valid_ech_config_list(*contents)) { + *out_alert = SSL_AD_DECODE_ERROR; + return false; + } + // The server may only send retry configs in response to ClientHelloOuter (or // ECH GREASE), not ClientHelloInner. The unsolicited extension rule checks // this implicitly because the ClientHelloInner has no encrypted_client_hello @@ -663,14 +668,13 @@ static bool ext_ech_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert, // https://github.com/tlswg/draft-ietf-tls-esni/pull/422 is merged, a later // draft will fold encrypted_client_hello and ech_is_inner together. Then this // assert should become a runtime check. - assert(!ssl->s3->ech_accept); - - // TODO(https://crbug.com/boringssl/275): When the implementing the - // ClientHelloOuter flow, save the retry configs. - if (!ssl_is_valid_ech_config_list(*contents)) { - *out_alert = SSL_AD_DECODE_ERROR; + assert(ssl->s3->ech_status != ssl_ech_accepted); + if (hs->selected_ech_config && + !hs->ech_retry_configs.CopyFrom(*contents)) { + *out_alert = SSL_AD_INTERNAL_ERROR; return false; } + return true; } @@ -685,8 +689,8 @@ static bool ext_ech_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert, static bool ext_ech_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) { SSL *const ssl = hs->ssl; - if (ssl_protocol_version(ssl) < TLS1_3_VERSION || // - ssl->s3->ech_accept || // + if (ssl_protocol_version(ssl) < TLS1_3_VERSION || + ssl->s3->ech_status == ssl_ech_accepted || // hs->ech_keys == nullptr) { return true; } @@ -1634,12 +1638,21 @@ static bool ext_channel_id_add_clienthello(const SSL_HANDSHAKE *hs, CBB *out, CBB *out_compressible, ssl_client_hello_type_t type) { const SSL *const ssl = hs->ssl; - if (!hs->config->channel_id_private || SSL_is_dtls(ssl)) { + if (!hs->config->channel_id_private || SSL_is_dtls(ssl) || + // Don't offer Channel ID in ClientHelloOuter. ClientHelloOuter handshakes + // are not authenticated for the name that can learn the Channel ID. + // + // We could alternatively offer the extension but sign with a random key. + // For other extensions, we try to align |ssl_client_hello_outer| and + // |ssl_client_hello_unencrypted|, to improve the effectiveness of ECH + // GREASE. However, Channel ID is deprecated and unlikely to be used with + // ECH, so do the simplest thing. + type == ssl_client_hello_outer) { return true; } - if (!CBB_add_u16(out_compressible, TLSEXT_TYPE_channel_id) || - !CBB_add_u16(out_compressible, 0 /* length */)) { + if (!CBB_add_u16(out, TLSEXT_TYPE_channel_id) || + !CBB_add_u16(out, 0 /* length */)) { return false; } diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 07c9e3d9a..db4ee710c 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc @@ -128,6 +128,7 @@ SSL_HANDSHAKE::SSL_HANDSHAKE(SSL *ssl_arg) : ssl(ssl_arg), ech_present(false), ech_is_inner_present(false), + ech_authenticated_reject(false), scts_requested(false), handshake_finalized(false), accept_psk_mode(false), @@ -715,6 +716,10 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) { return -1; case ssl_hs_early_return: + if (!ssl->server) { + // On ECH reject, the handshake should never complete. + assert(ssl->s3->ech_status != ssl_ech_rejected); + } *out_early_return = true; hs->wait = ssl_hs_ok; return 1; @@ -734,6 +739,10 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) { return -1; } if (hs->wait == ssl_hs_ok) { + if (!ssl->server) { + // On ECH reject, the handshake should never complete. + assert(ssl->s3->ech_status != ssl_ech_rejected); + } // The handshake has completed. *out_early_return = false; return 1; diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index d5ccafc26..ba8f4b748 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -731,13 +731,9 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - // TODO(https://crbug.com/boringssl/275): If the server negotiates TLS 1.2 and - // we offer ECH, we handshake with ClientHelloOuter instead of - // ClientHelloInner. That path is not yet implemented. For now, terminate the - // handshake with a distinguishable error for testing. + // TLS 1.2 handshakes cannot accept ECH. if (hs->selected_ech_config) { - OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_REJECTED); - return ssl_hs_error; + ssl->s3->ech_status = ssl_ech_rejected; } // Copy over the server random. @@ -764,44 +760,13 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { } } - if (hs->session_id_len != 0 && - CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len)) { - // Echoing the ClientHello session ID in TLS 1.2, whether from the session - // or a synthetic one, indicates resumption. If there was no session, this - // was the TLS 1.3 compatibility mode session ID. As we know this is not a - // session the server knows about, any server resuming it is in error. - // Reject the first connection deterministicly, rather than installing an - // invalid session into the session cache. https://crbug.com/796910 - if (ssl->session == nullptr) { - OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_ECHOED_INVALID_SESSION_ID); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); - return ssl_hs_error; - } - // We never offer sessions on renegotiation. - assert(!ssl->s3->initial_handshake_complete); - ssl->s3->session_reused = true; - // Note |ssl->session| may be a TLS 1.3 session, offered in a separate - // extension altogether. In that case, the version check below will fail the - // connection. - } else { - // The session wasn't resumed. Create a fresh SSL_SESSION to fill out. - ssl_set_session(ssl, NULL); - if (!ssl_get_new_session(hs)) { - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); - return ssl_hs_error; - } - // Note: session_id could be empty. - hs->new_session->session_id_length = CBS_len(&session_id); - OPENSSL_memcpy(hs->new_session->session_id, CBS_data(&session_id), - CBS_len(&session_id)); - } - const SSL_CIPHER *cipher = SSL_get_cipher_by_value(cipher_suite); if (cipher == NULL) { OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CIPHER_RETURNED); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error; } + hs->new_cipher = cipher; // The cipher must be allowed in the selected version and enabled. uint32_t mask_a, mask_k; @@ -815,7 +780,20 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - if (ssl->session != NULL) { + if (hs->session_id_len != 0 && + CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len)) { + // Echoing the ClientHello session ID in TLS 1.2, whether from the session + // or a synthetic one, indicates resumption. If there was no session (or if + // the session was only offered in ECH ClientHelloInner), this was the + // TLS 1.3 compatibility mode session ID. As we know this is not a session + // the server knows about, any server resuming it is in error. Reject the + // first connection deterministicly, rather than installing an invalid + // session into the session cache. https://crbug.com/796910 + if (ssl->session == nullptr || ssl->s3->ech_status == ssl_ech_rejected) { + OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_ECHOED_INVALID_SESSION_ID); + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); + return ssl_hs_error; + } if (ssl->session->ssl_version != ssl->version) { OPENSSL_PUT_ERROR(SSL, SSL_R_OLD_SESSION_VERSION_NOT_RETURNED); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); @@ -833,10 +811,22 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error; } + // We never offer sessions on renegotiation. + assert(!ssl->s3->initial_handshake_complete); + ssl->s3->session_reused = true; } else { + // The session wasn't resumed. Create a fresh SSL_SESSION to fill out. + ssl_set_session(ssl, NULL); + if (!ssl_get_new_session(hs)) { + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); + return ssl_hs_error; + } + // Note: session_id could be empty. + hs->new_session->session_id_length = CBS_len(&session_id); + OPENSSL_memcpy(hs->new_session->session_id, CBS_data(&session_id), + CBS_len(&session_id)); hs->new_session->cipher = cipher; } - hs->new_cipher = cipher; // Now that the cipher is known, initialize the handshake hash and hash the // ServerHello. @@ -1334,8 +1324,12 @@ static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) { return ssl_hs_ok; } - // Call cert_cb to update the certificate. - if (hs->config->cert->cert_cb != NULL) { + if (ssl->s3->ech_status == ssl_ech_rejected) { + // Do not send client certificates on ECH reject. We have not authenticated + // the server for the name that can learn the certificate. + SSL_certs_clear(ssl); + } else if (hs->config->cert->cert_cb != nullptr) { + // Call cert_cb to update the certificate. int rv = hs->config->cert->cert_cb(ssl, hs->config->cert->cert_cb_arg); if (rv == 0) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); @@ -1642,7 +1636,7 @@ static enum ssl_hs_wait_t do_send_client_finished(SSL_HANDSHAKE *hs) { } static bool can_false_start(const SSL_HANDSHAKE *hs) { - SSL *const ssl = hs->ssl; + const SSL *const ssl = hs->ssl; // False Start bypasses the Finished check's downgrade protection. This can // enable attacks where we send data under weaker settings than supported @@ -1660,6 +1654,13 @@ static bool can_false_start(const SSL_HANDSHAKE *hs) { return false; } + // If ECH was rejected, disable False Start. We run the handshake to + // completion, including the Finished downgrade check, to authenticate the + // recovery flow. + if (ssl->s3->ech_status == ssl_ech_rejected) { + return false; + } + // Additionally require ALPN or NPN by default. // // TODO(davidben): Can this constraint be relaxed globally now that cipher @@ -1796,6 +1797,13 @@ static enum ssl_hs_wait_t do_read_server_finished(SSL_HANDSHAKE *hs) { static enum ssl_hs_wait_t do_finish_client_handshake(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; + if (ssl->s3->ech_status == ssl_ech_rejected) { + // Release the retry configs. + hs->ech_authenticated_reject = true; + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ECH_REQUIRED); + OPENSSL_PUT_ERROR(SSL, SSL_R_ECH_REJECTED); + return ssl_hs_error; + } ssl->method->on_handshake_complete(ssl); diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 74ac1338e..c8a23a165 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -661,14 +661,17 @@ static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) { } hs->ech_config_id = config_id; - ssl->s3->ech_accept = true; + ssl->s3->ech_status = ssl_ech_accepted; break; } } - // If we did not accept ECH, we will send the current ECHConfigs as - // retry_configs in the ServerHello's encrypted extensions. Proceed with the - // ClientHelloOuter. + // If we did not accept ECH, proceed with the ClientHelloOuter. Note this + // could be key mismatch or ECH GREASE, so we most complete the handshake + // as usual, except EncryptedExtensions will contain retry configs. + if (ssl->s3->ech_status != ssl_ech_accepted) { + ssl->s3->ech_status = ssl_ech_rejected; + } } uint8_t alert = SSL_AD_DECODE_ERROR; @@ -803,7 +806,7 @@ static enum ssl_hs_wait_t do_select_certificate(SSL_HANDSHAKE *hs) { // It should not be possible to negotiate TLS 1.2 with ECH. The // ClientHelloInner decoding function rejects ClientHellos which offer TLS 1.2 // or below. - assert(!ssl->s3->ech_accept); + assert(ssl->s3->ech_status != ssl_ech_accepted); // TODO(davidben): Also compute hints for TLS 1.2. When doing so, update the // check in bssl_shim.cc to test this. diff --git a/ssl/internal.h b/ssl/internal.h index ba1dde351..3b7326ae9 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1833,6 +1833,10 @@ struct SSL_HANDSHAKE { // ech_client_bytes contains the ECH extension to send in the ClientHello. Array ech_client_bytes; + // ech_retry_configs, on the client, contains the retry configs from the + // server as a serialized ECHConfigList. + Array ech_retry_configs; + // ech_client_hello_buf, on the server, contains the bytes of the // reconstructed ClientHelloInner message. Array ech_client_hello_buf; @@ -1872,8 +1876,8 @@ struct SSL_HANDSHAKE { uint16_t cert_compression_alg_id; // ech_hpke_ctx is the HPKE context used in ECH. On the server, it is - // initialized if |ech_accept| is true. On the client, it is initialized if - // |selected_ech_config| is not nullptr. + // initialized if |ech_status| is |ssl_ech_accepted|. On the client, it is + // initialized if |selected_ech_config| is not nullptr. ScopedEVP_HPKE_CTX ech_hpke_ctx; // server_params, in a TLS 1.2 server, stores the ServerKeyExchange @@ -1943,6 +1947,10 @@ struct SSL_HANDSHAKE { // contained an ech_is_inner extension. bool ech_is_inner_present : 1; + // ech_authenticated_reject, on the client, indicates whether an ECH rejection + // handshake has been authenticated. + bool ech_authenticated_reject : 1; + // scts_requested is true if the SCT extension is in the ClientHello. bool scts_requested : 1; @@ -2574,6 +2582,16 @@ enum ssl_shutdown_t { ssl_shutdown_error = 2, }; +enum ssl_ech_status_t { + // ssl_ech_none indicates ECH was not offered, or we have not gotten far + // enough in the handshake to determine the status. + ssl_ech_none, + // ssl_ech_accepted indicates the server accepted ECH. + ssl_ech_accepted, + // ssl_ech_rejected indicates the server was offered ECH but rejected it. + ssl_ech_rejected, +}; + struct SSL3_STATE { static constexpr bool kAllowUniquePtr = true; @@ -2636,8 +2654,8 @@ struct SSL3_STATE { // key_update_count is the number of consecutive KeyUpdates received. uint8_t key_update_count = 0; - // ech_accept indicates whether ECH was accepted by the server. - bool ech_accept : 1; + // ech_status indicates whether ECH was accepted by the server. + ssl_ech_status_t ech_status = ssl_ech_none; // skip_early_data instructs the record layer to skip unexpected early data // messages when 0RTT is rejected. diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index 454daf853..fa73d34a7 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc @@ -164,8 +164,7 @@ BSSL_NAMESPACE_BEGIN SSL3_STATE::SSL3_STATE() - : ech_accept(false), - skip_early_data(false), + : skip_early_data(false), have_version(false), v2_hello_done(false), is_v2_hello(false), diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 4bb9c3216..76f88c7a9 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1691,14 +1691,13 @@ bool MakeECHConfig(std::vector *out, return true; } -static bssl::UniquePtr MakeTestECHKeys() { +static bssl::UniquePtr MakeTestECHKeys(uint8_t config_id = 1) { bssl::ScopedEVP_HPKE_KEY key; uint8_t *ech_config; size_t ech_config_len; if (!EVP_HPKE_KEY_generate(key.get(), EVP_hpke_x25519_hkdf_sha256()) || - !SSL_marshal_ech_config(&ech_config, &ech_config_len, - /*config_id=*/1, key.get(), "public.example", - 16)) { + !SSL_marshal_ech_config(&ech_config, &ech_config_len, config_id, + key.get(), "public.example", 16)) { return nullptr; } bssl::UniquePtr free_ech_config(ech_config); @@ -2154,6 +2153,147 @@ TEST(SSLTest, ECHPublicName) { EXPECT_TRUE(ssl_is_valid_ech_public_name(str_to_span("4294967296"))); } +// When using the built-in verifier, test that |SSL_get0_ech_name_override| is +// applied automatically. +TEST(SSLTest, ECHBuiltinVerifier) { + // These test certificates generated with the following Go program. + /* clang-format off +func main() { + notBefore := time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC) + notAfter := time.Date(2099, time.January, 1, 0, 0, 0, 0, time.UTC) + rootKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + rootTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "Test CA"}, + NotBefore: notBefore, + NotAfter: notAfter, + BasicConstraintsValid: true, + IsCA: true, + } + rootDER, _ := x509.CreateCertificate(rand.Reader, rootTemplate, rootTemplate, &rootKey.PublicKey, rootKey) + root, _ := x509.ParseCertificate(rootDER) + pem.Encode(os.Stdout, &pem.Block{Type: "CERTIFICATE", Bytes: rootDER}) + leafKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + leafKeyDER, _ := x509.MarshalPKCS8PrivateKey(leafKey) + pem.Encode(os.Stdout, &pem.Block{Type: "PRIVATE KEY", Bytes: leafKeyDER}) + for i, name := range []string{"public.example", "secret.example"} { + leafTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(int64(i) + 2), + Subject: pkix.Name{CommonName: name}, + NotBefore: notBefore, + NotAfter: notAfter, + BasicConstraintsValid: true, + DNSNames: []string{name}, + } + leafDER, _ := x509.CreateCertificate(rand.Reader, leafTemplate, root, &leafKey.PublicKey, rootKey) + pem.Encode(os.Stdout, &pem.Block{Type: "CERTIFICATE", Bytes: leafDER}) + } +} +clang-format on */ + bssl::UniquePtr root = CertFromPEM(R"( +-----BEGIN CERTIFICATE----- +MIIBRzCB7aADAgECAgEBMAoGCCqGSM49BAMCMBIxEDAOBgNVBAMTB1Rlc3QgQ0Ew +IBcNMDAwMTAxMDAwMDAwWhgPMjA5OTAxMDEwMDAwMDBaMBIxEDAOBgNVBAMTB1Rl +c3QgQ0EwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAT5JUjrI1DAxSpEl88UkmJw +tAJqxo/YrSFo9V3MkcNkfTixi5p6MUtO8DazhEgekBcd2+tBAWtl7dy0qpvTqx92 +ozIwMDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTw6ftkexAI6o4r5FntJIfL +GU5F4zAKBggqhkjOPQQDAgNJADBGAiEAiiNowddQeHZaZFIygwe6RW5/WG4sUXWC +dkyl9CQzRaYCIQCFS1EvwZbZtMny27fYm1eeYciY0TkJTEi34H1KwyzzIA== +-----END CERTIFICATE----- +)"); + ASSERT_TRUE(root); + bssl::UniquePtr leaf_key = KeyFromPEM(R"( +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgj5WKHwHnziiyPauf +7QukxTwtTyGZkk8qNdms4puJfxqhRANCAARNrkhxabALDlJrHtvkuDwvCWUF/oVC +hr6PDITHi1lDlJzvVT4aXBH87sH2n2UV5zpx13NHkq1bIC8eRT8eOIe0 +-----END PRIVATE KEY----- +)"); + ASSERT_TRUE(leaf_key); + bssl::UniquePtr leaf_public = CertFromPEM(R"( +-----BEGIN CERTIFICATE----- +MIIBaDCCAQ6gAwIBAgIBAjAKBggqhkjOPQQDAjASMRAwDgYDVQQDEwdUZXN0IENB +MCAXDTAwMDEwMTAwMDAwMFoYDzIwOTkwMTAxMDAwMDAwWjAZMRcwFQYDVQQDEw5w +dWJsaWMuZXhhbXBsZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABE2uSHFpsAsO +Umse2+S4PC8JZQX+hUKGvo8MhMeLWUOUnO9VPhpcEfzuwfafZRXnOnHXc0eSrVsg +Lx5FPx44h7SjTDBKMAwGA1UdEwEB/wQCMAAwHwYDVR0jBBgwFoAU8On7ZHsQCOqO +K+RZ7SSHyxlOReMwGQYDVR0RBBIwEIIOcHVibGljLmV4YW1wbGUwCgYIKoZIzj0E +AwIDSAAwRQIhANqZRhDR/+QL05hsWXMYEwaiHifd9iakKoFEhKFchcF3AiBRAeXw +wRGGT6+iPmTYM6N5/IDyAb5B9Ke38O6lLEsUwA== +-----END CERTIFICATE----- +)"); + ASSERT_TRUE(leaf_public); + bssl::UniquePtr leaf_secret = CertFromPEM(R"( +-----BEGIN CERTIFICATE----- +MIIBaTCCAQ6gAwIBAgIBAzAKBggqhkjOPQQDAjASMRAwDgYDVQQDEwdUZXN0IENB +MCAXDTAwMDEwMTAwMDAwMFoYDzIwOTkwMTAxMDAwMDAwWjAZMRcwFQYDVQQDEw5z +ZWNyZXQuZXhhbXBsZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABE2uSHFpsAsO +Umse2+S4PC8JZQX+hUKGvo8MhMeLWUOUnO9VPhpcEfzuwfafZRXnOnHXc0eSrVsg +Lx5FPx44h7SjTDBKMAwGA1UdEwEB/wQCMAAwHwYDVR0jBBgwFoAU8On7ZHsQCOqO +K+RZ7SSHyxlOReMwGQYDVR0RBBIwEIIOc2VjcmV0LmV4YW1wbGUwCgYIKoZIzj0E +AwIDSQAwRgIhAPQdIz1xCFkc9WuSkxOxJDpywZiEp9SnKcxJ9nwrlRp3AiEA+O3+ +XRqE7XFhHL+7TNC2a9OOAjQsEF137YPWo+rhgko= +-----END CERTIFICATE----- +)"); + ASSERT_TRUE(leaf_secret); + + // Use different config IDs so that fuzzer mode, which breaks trial + // decryption, will observe the key mismatch. + bssl::UniquePtr keys = MakeTestECHKeys(/*config_id=*/1); + ASSERT_TRUE(keys); + bssl::UniquePtr wrong_keys = MakeTestECHKeys(/*config_id=*/2); + ASSERT_TRUE(wrong_keys); + bssl::UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(server_ctx); + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(client_ctx); + + // Configure the client to verify certificates and expect the secret name. + // This is the name the client is trying to connect to. If ECH is rejected, + // BoringSSL will internally override this setting with the public name. + bssl::UniquePtr store(X509_STORE_new()); + ASSERT_TRUE(store); + ASSERT_TRUE(X509_STORE_add_cert(store.get(), root.get())); + SSL_CTX_set_cert_store(client_ctx.get(), store.release()); + SSL_CTX_set_verify(client_ctx.get(), SSL_VERIFY_PEER, nullptr); + static const char kSecretName[] = "secret.example"; + ASSERT_TRUE(X509_VERIFY_PARAM_set1_host(SSL_CTX_get0_param(client_ctx.get()), + kSecretName, strlen(kSecretName))); + + // For simplicity, we only run through a pair of representative scenarios here + // and rely on runner.go to verify that |SSL_get0_ech_name_override| behaves + // correctly. + for (bool accept_ech : {false, true}) { + SCOPED_TRACE(accept_ech); + for (bool use_leaf_secret : {false, true}) { + SCOPED_TRACE(use_leaf_secret); + + // The server will reject ECH when configured with the wrong keys. + ASSERT_TRUE(SSL_CTX_set1_ech_keys( + server_ctx.get(), accept_ech ? keys.get() : wrong_keys.get())); + + bssl::UniquePtr client, server; + ASSERT_TRUE(CreateClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + ASSERT_TRUE(InstallECHConfigList(client.get(), keys.get())); + + // Configure the server with the selected certificate. + ASSERT_TRUE(SSL_use_certificate(server.get(), use_leaf_secret + ? leaf_secret.get() + : leaf_public.get())); + ASSERT_TRUE(SSL_use_PrivateKey(server.get(), leaf_key.get())); + + // The handshake may fail due to name mismatch or ECH reject. We check + // |SSL_get_verify_result| to confirm the handshake got far enough. + CompleteHandshakes(client.get(), server.get()); + EXPECT_EQ(accept_ech == use_leaf_secret ? X509_V_OK + : X509_V_ERR_HOSTNAME_MISMATCH, + SSL_get_verify_result(client.get())); + } + } +} + #if defined(OPENSSL_THREADS) // Test that the server ECH config can be swapped out while the |SSL_CTX| is // in use on other threads. This test is intended to be run with TSan. diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index cda761171..98f1f6a87 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -368,25 +368,33 @@ static bool ssl_crypto_x509_session_verify_cert_chain(SSL_SESSION *session, return false; } - SSL_CTX *ssl_ctx = hs->ssl->ctx.get(); + SSL *const ssl = hs->ssl; + SSL_CTX *ssl_ctx = ssl->ctx.get(); X509_STORE *verify_store = ssl_ctx->cert_store; if (hs->config->cert->verify_store != nullptr) { verify_store = hs->config->cert->verify_store; } X509 *leaf = sk_X509_value(cert_chain, 0); + const char *name; + size_t name_len; + SSL_get0_ech_name_override(ssl, &name, &name_len); ScopedX509_STORE_CTX ctx; if (!X509_STORE_CTX_init(ctx.get(), verify_store, leaf, cert_chain) || - !X509_STORE_CTX_set_ex_data( - ctx.get(), SSL_get_ex_data_X509_STORE_CTX_idx(), hs->ssl) || + !X509_STORE_CTX_set_ex_data(ctx.get(), + SSL_get_ex_data_X509_STORE_CTX_idx(), ssl) || // We need to inherit the verify parameters. These can be determined by // the context: if its a server it will verify SSL client certificates or // vice versa. - !X509_STORE_CTX_set_default( - ctx.get(), hs->ssl->server ? "ssl_client" : "ssl_server") || + !X509_STORE_CTX_set_default(ctx.get(), + ssl->server ? "ssl_client" : "ssl_server") || // Anything non-default in "param" should overwrite anything in the ctx. !X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()), - hs->config->param)) { + hs->config->param) || + // ClientHelloOuter connections use a different name. + (name_len != 0 && + !X509_VERIFY_PARAM_set1_host(X509_STORE_CTX_get0_param(ctx.get()), name, + name_len))) { OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB); return false; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index ef46540d1..f4e7fffd6 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -809,9 +809,44 @@ static bool DoConnection(bssl::UniquePtr *out_session, } assert(!config->handoff); + config = retry_config; ret = DoExchange(out_session, &ssl, retry_config, is_resume, true, writer); } + // An ECH rejection appears as a failed connection. Note |ssl| may use a + // different config on ECH rejection. + if (config->expect_no_ech_retry_configs || + !config->expect_ech_retry_configs.empty()) { + bssl::Span expected = + config->expect_no_ech_retry_configs + ? bssl::Span() + : bssl::MakeConstSpan(reinterpret_cast( + config->expect_ech_retry_configs.data()), + config->expect_ech_retry_configs.size()); + if (ret) { + fprintf(stderr, "Expected ECH rejection, but connection succeeded.\n"); + return false; + } + uint32_t err = ERR_peek_error(); + if (SSL_get_error(ssl.get(), -1) != SSL_ERROR_SSL || + ERR_GET_LIB(err) != ERR_LIB_SSL || + ERR_GET_REASON(err) != SSL_R_ECH_REJECTED) { + fprintf(stderr, "Expected ECH rejection, but connection succeeded.\n"); + return false; + } + const uint8_t *retry_configs; + size_t retry_configs_len; + SSL_get0_ech_retry_configs(ssl.get(), &retry_configs, &retry_configs_len); + if (bssl::MakeConstSpan(retry_configs, retry_configs_len) != expected) { + fprintf(stderr, "ECH retry configs did not match expectations.\n"); + // Clear the error queue. Otherwise |SSL_R_ECH_REJECTED| will be printed + // to stderr and the test framework will think the test had the expected + // expectations. + ERR_clear_error(); + return false; + } + } + if (!ret) { // Print the |SSL_get_error| code. Otherwise, some failures are silent and // hard to debug. diff --git a/ssl/test/runner/alert.go b/ssl/test/runner/alert.go index fbfa6ab1a..561d0c6eb 100644 --- a/ssl/test/runner/alert.go +++ b/ssl/test/runner/alert.go @@ -45,6 +45,7 @@ const ( alertUnknownPSKIdentity alert = 115 alertCertificateRequired alert = 116 alertNoApplicationProtocol alert = 120 + alertECHRequired alert = 121 ) var alertText = map[alert]string{ @@ -78,6 +79,7 @@ var alertText = map[alert]string{ alertUnknownPSKIdentity: "unknown PSK identity", alertCertificateRequired: "certificate required", alertNoApplicationProtocol: "no application protocol", + alertECHRequired: "ECH required", } func (e alert) String() string { diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 767f8da29..782eb36a0 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1677,6 +1677,10 @@ type ProtocolBugs struct { // invalid Channel ID signature. InvalidChannelIDSignature bool + // AlwaysNegotiateChannelID, if true, causes the server to negotiate Channel + // ID, even whenn the client does not offer it. + AlwaysNegotiateChannelID bool + // ExpectGREASE, if true, causes messages without GREASE values to be // rejected. See RFC 8701. ExpectGREASE bool diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 1bfc584d6..b9d766717 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -1650,7 +1650,7 @@ func (hs *serverHandshakeState) processClientExtensions(serverExtensions *server serverExtensions.extendedMasterSecret = hs.clientHello.extendedMasterSecret && !disableEMS } - if hs.clientHello.channelIDSupported && config.RequestChannelID { + if config.Bugs.AlwaysNegotiateChannelID || (hs.clientHello.channelIDSupported && config.RequestChannelID) { serverExtensions.channelIDRequested = true } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 5bdd50cee..4838eecce 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -17708,28 +17708,30 @@ func addEncryptedClientHelloTests() { }) // Test the client can recognize when ECH is rejected. - // TODO(https://crbug.com/boringssl/275): Once implemented, this - // handshake should complete. testCases = append(testCases, testCase{ testType: clientTest, protocol: protocol, name: prefix + "ECH-Client-Reject", config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig2, echConfig3}, Bugs: ProtocolBugs{ ExpectServerName: "public.example", }, }, flags: []string{ "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-retry-configs", base64FlagValue(CreateECHConfigList(echConfig2.ECHConfig.Raw, echConfig3.ECHConfig.Raw)), }, - shouldFail: true, - expectedError: ":CONNECTION_REJECTED:", + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", }) testCases = append(testCases, testCase{ testType: clientTest, protocol: protocol, name: prefix + "ECH-Client-Reject-HelloRetryRequest", config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig2, echConfig3}, CurvePreferences: []CurveID{CurveP384}, Bugs: ProtocolBugs{ ExpectServerName: "public.example", @@ -17738,10 +17740,29 @@ func addEncryptedClientHelloTests() { }, flags: []string{ "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-retry-configs", base64FlagValue(CreateECHConfigList(echConfig2.ECHConfig.Raw, echConfig3.ECHConfig.Raw)), "-expect-hrr", // Check we triggered HRR. }, - shouldFail: true, - expectedError: ":CONNECTION_REJECTED:", + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", + }) + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-NoRetryConfigs", + config: Config{ + Bugs: ProtocolBugs{ + ExpectServerName: "public.example", + }, + }, + flags: []string{ + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-no-ech-retry-configs", + }, + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", }) if protocol != quic { testCases = append(testCases, testCase{ @@ -17751,19 +17772,79 @@ func addEncryptedClientHelloTests() { config: Config{ MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ - ExpectServerName: "public.example", - ExpectMissingKeyShare: true, // Check we triggered HRR. + ExpectServerName: "public.example", }, }, flags: []string{ "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), - "-expect-hrr", // Check we triggered HRR. + // TLS 1.2 cannot provide retry configs. + "-expect-no-ech-retry-configs", }, - shouldFail: true, - expectedError: ":CONNECTION_REJECTED:", + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", + }) + + // Test that the client disables False Start when ECH is rejected. + testCases = append(testCases, testCase{ + name: prefix + "ECH-Client-Reject-TLS12-NoFalseStart", + config: Config{ + MaxVersion: VersionTLS12, + CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, + NextProtos: []string{"foo"}, + Bugs: ProtocolBugs{ + // The options below cause the server to, immediately + // after client Finished, send an alert and try to read + // application data without sending server Finished. + ExpectFalseStart: true, + AlertBeforeFalseStartTest: alertAccessDenied, + }, + }, + flags: []string{ + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-false-start", + "-advertise-alpn", "\x03foo", + "-expect-alpn", "foo", + }, + shimWritesFirst: true, + shouldFail: true, + // Ensure the client does not send application data at the False + // Start point. EOF comes from the client closing the connection + // in response ot the alert. + expectedLocalError: "tls: peer did not false start: EOF", + // Ensures the client picks up the alert before reporting an + // authenticated |SSL_R_ECH_REJECTED|. + expectedError: ":TLSV1_ALERT_ACCESS_DENIED:", }) } + // Test that unsupported retry configs in a valid ECHConfigList are + // allowed. They will be skipped when configured in the retry. + retryConfigs := CreateECHConfigList( + unsupportedVersion, + unsupportedKEM.Raw, + unsupportedCipherSuites.Raw, + unsupportedMandatoryExtension.Raw, + echConfig2.ECHConfig.Raw) + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-UnsupportedRetryConfigs", + config: Config{ + Bugs: ProtocolBugs{ + SendECHRetryConfigs: retryConfigs, + ExpectServerName: "public.example", + }, + }, + flags: []string{ + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-retry-configs", base64FlagValue(retryConfigs), + }, + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", + }) + // Test that the client rejects ClientHelloOuter handshakes that attempt // to resume the ClientHelloInner's ticket. In draft-ietf-tls-esni-10, // the confirmation signal is computed in an odd order, so this requires @@ -17771,7 +17852,7 @@ func addEncryptedClientHelloTests() { testCases = append(testCases, testCase{ testType: clientTest, protocol: protocol, - name: prefix + "ECH-Client-Reject-ResumeInnerSession", + name: prefix + "ECH-Client-Reject-ResumeInnerSession-TLS13", config: Config{ ServerECHConfigs: []ServerECHConfig{echConfig}, Bugs: ProtocolBugs{ @@ -17779,6 +17860,7 @@ func addEncryptedClientHelloTests() { }, }, resumeConfig: &Config{ + MaxVersion: VersionTLS13, ServerECHConfigs: []ServerECHConfig{echConfig}, Bugs: ProtocolBugs{ ExpectServerName: "public.example", @@ -17797,11 +17879,54 @@ func addEncryptedClientHelloTests() { resumeExpectations: &connectionExpectations{echAccepted: false}, }) + // Test the above, but the server now attempts to resume the + // ClientHelloInner's ticket at TLS 1.2. + if protocol != quic { + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-ResumeInnerSession-TLS12", + config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig}, + Bugs: ProtocolBugs{ + ExpectServerName: "secret.example", + }, + }, + resumeConfig: &Config{ + MinVersion: VersionTLS12, + MaxVersion: VersionTLS12, + ServerECHConfigs: []ServerECHConfig{echConfig}, + Bugs: ProtocolBugs{ + ExpectServerName: "public.example", + UseInnerSessionWithClientHelloOuter: true, + // The client only ever offers TLS 1.3 sessions in + // ClientHelloInner. AcceptAnySession allows them to be + // resumed at TLS 1.2. + AcceptAnySession: true, + }, + }, + resumeSession: true, + flags: []string{ + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-host-name", "secret.example", + "-on-initial-expect-ech-accept", + }, + // From the client's perspective, the server echoed a session ID to + // signal resumption, but the selected ClientHello had nothing to + // resume. + shouldFail: true, + expectedError: ":SERVER_ECHOED_INVALID_SESSION_ID:", + expectedLocalError: "remote error: illegal parameter", + expectations: connectionExpectations{echAccepted: true}, + resumeExpectations: &connectionExpectations{echAccepted: false}, + }) + } + // Test that the client can process ECH rejects after an early data reject. testCases = append(testCases, testCase{ testType: clientTest, protocol: protocol, - name: prefix + "ECH-Client-Reject-EarlyDataReject", + name: prefix + "ECH-Client-Reject-EarlyDataRejected", config: Config{ ServerECHConfigs: []ServerECHConfig{echConfig}, Bugs: ProtocolBugs{ @@ -17809,6 +17934,7 @@ func addEncryptedClientHelloTests() { }, }, resumeConfig: &Config{ + ServerECHConfigs: []ServerECHConfig{echConfig2}, Bugs: ProtocolBugs{ ExpectServerName: "public.example", }, @@ -17819,6 +17945,10 @@ func addEncryptedClientHelloTests() { // Although the resumption connection does not accept ECH, the // API will report ECH was accepted at the 0-RTT point. "-expect-ech-accept", + // -on-retry refers to the retried handshake after 0-RTT reject, + // while ech-retry-configs refers to the ECHConfigs to use in + // the next connection attempt. + "-on-retry-expect-ech-retry-configs", base64FlagValue(CreateECHConfigList(echConfig2.ECHConfig.Raw)), }, resumeSession: true, expectResumeRejected: true, @@ -17826,16 +17956,15 @@ func addEncryptedClientHelloTests() { expectEarlyDataRejected: true, expectations: connectionExpectations{echAccepted: true}, resumeExpectations: &connectionExpectations{echAccepted: false}, - // TODO(https://crbug.com/boringssl/275): Once implemented, this - // should complete the handshake. - shouldFail: true, - expectedError: ":CONNECTION_REJECTED:", + shouldFail: true, + expectedLocalError: "remote error: ECH required", + expectedError: ":ECH_REJECTED:", }) if protocol != quic { testCases = append(testCases, testCase{ testType: clientTest, protocol: protocol, - name: prefix + "ECH-Client-Reject-EarlyDataReject-TLS12", + name: prefix + "ECH-Client-Reject-EarlyDataRejected-TLS12", config: Config{ ServerECHConfigs: []ServerECHConfig{echConfig}, Bugs: ProtocolBugs{ @@ -17911,6 +18040,266 @@ func addEncryptedClientHelloTests() { }, expectations: connectionExpectations{echAccepted: true}, }) + + // Test both sync and async mode, to test both with and without the + // client certificate callback. + for _, async := range []bool{false, true} { + var flags []string + var suffix string + if async { + flags = []string{"-async"} + suffix = "-Async" + } + + // Test that ECH and client certificates can be used together. + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-ClientCertificate" + suffix, + config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig}, + ClientAuth: RequireAnyClientCert, + }, + flags: append([]string{ + "-cert-file", path.Join(*resourceDir, rsaCertificateFile), + "-key-file", path.Join(*resourceDir, rsaKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-accept", + }, flags...), + expectations: connectionExpectations{echAccepted: true}, + }) + + // Test that, when ECH is rejected, the client does not send a client + // certificate. + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-NoClientCertificate-TLS13" + suffix, + config: Config{ + MinVersion: VersionTLS13, + MaxVersion: VersionTLS13, + ClientAuth: RequireAnyClientCert, + }, + flags: append([]string{ + "-cert-file", path.Join(*resourceDir, rsaCertificateFile), + "-key-file", path.Join(*resourceDir, rsaKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + }, flags...), + shouldFail: true, + expectedLocalError: "tls: client didn't provide a certificate", + }) + if protocol != quic { + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-NoClientCertificate-TLS12" + suffix, + config: Config{ + MinVersion: VersionTLS12, + MaxVersion: VersionTLS12, + ClientAuth: RequireAnyClientCert, + }, + flags: append([]string{ + "-cert-file", path.Join(*resourceDir, rsaCertificateFile), + "-key-file", path.Join(*resourceDir, rsaKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + }, flags...), + shouldFail: true, + expectedLocalError: "tls: client didn't provide a certificate", + }) + } + } + + // Test that ECH and Channel ID can be used together. + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-ChannelID", + config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig}, + RequestChannelID: true, + }, + flags: []string{ + "-send-channel-id", path.Join(*resourceDir, channelIDKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-accept", + }, + resumeSession: true, + expectations: connectionExpectations{ + channelID: true, + echAccepted: true, + }, + }) + + // Handshakes where ECH is rejected do not offer or accept Channel ID. + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-NoChannelID-TLS13", + config: Config{ + MinVersion: VersionTLS13, + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + AlwaysNegotiateChannelID: true, + }, + }, + flags: []string{ + "-send-channel-id", path.Join(*resourceDir, channelIDKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + }, + shouldFail: true, + expectedLocalError: "remote error: unsupported extension", + expectedError: ":UNEXPECTED_EXTENSION:", + }) + if protocol != quic { + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-NoChannelID-TLS12", + config: Config{ + MinVersion: VersionTLS12, + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + AlwaysNegotiateChannelID: true, + }, + }, + flags: []string{ + "-send-channel-id", path.Join(*resourceDir, channelIDKeyFile), + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + }, + shouldFail: true, + expectedLocalError: "remote error: unsupported extension", + expectedError: ":UNEXPECTED_EXTENSION:", + }) + } + + // Test that ECH correctly overrides the host name for certificate + // verification. + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-NotOffered-NoOverrideName", + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + // When not offering ECH, verify the usual name in both full + // and resumption handshakes. + "-reverify-on-resume", + "-expect-no-ech-name-override", + }, + resumeSession: true, + }) + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-GREASE-NoOverrideName", + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + "-enable-ech-grease", + // When offering ECH GREASE, verify the usual name in both full + // and resumption handshakes. + "-reverify-on-resume", + "-expect-no-ech-name-override", + }, + resumeSession: true, + }) + if protocol != quic { + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Rejected-OverrideName-TLS12", + config: Config{ + MinVersion: VersionTLS12, + MaxVersion: VersionTLS12, + }, + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + // When ECH is rejected, verify the public name. This can + // only happen in full handshakes. + "-expect-ech-name-override", "public.example", + }, + shouldFail: true, + expectedError: ":ECH_REJECTED:", + expectedLocalError: "remote error: ECH required", + }) + } + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-OverrideName-TLS13", + config: Config{ + MinVersion: VersionTLS13, + MaxVersion: VersionTLS13, + }, + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + // When ECH is rejected, verify the public name. This can + // only happen in full handshakes. + "-expect-ech-name-override", "public.example", + }, + shouldFail: true, + expectedError: ":ECH_REJECTED:", + expectedLocalError: "remote error: ECH required", + }) + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Accept-NoOverrideName", + config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig}, + }, + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + "-expect-ech-accept", + // When ECH is accepted, verify the usual name in both full and + // resumption handshakes. + "-reverify-on-resume", + "-expect-no-ech-name-override", + }, + resumeSession: true, + expectations: connectionExpectations{echAccepted: true}, + }) + testCases = append(testCases, testCase{ + testType: clientTest, + protocol: protocol, + name: prefix + "ECH-Client-Reject-EarlyDataRejected-OverrideNameOnRetry", + config: Config{ + ServerECHConfigs: []ServerECHConfig{echConfig}, + }, + resumeConfig: &Config{}, + flags: []string{ + "-verify-peer", + "-use-custom-verify-callback", + "-ech-config-list", base64FlagValue(CreateECHConfigList(echConfig.ECHConfig.Raw)), + // Although the resumption connection does not accept ECH, the + // API will report ECH was accepted at the 0-RTT point. + "-expect-ech-accept", + // The resumption connection verifies certificates twice. First, + // if reverification is enabled, we verify the 0-RTT certificate + // as if ECH as accepted. There should be no name override. + // Next, on the post-0-RTT-rejection retry, we verify the new + // server certificate. This picks up the ECH reject, so it + // should use public.example. + "-reverify-on-resume", + "-on-resume-expect-no-ech-name-override", + "-on-retry-expect-ech-name-override", "public.example", + }, + resumeSession: true, + expectResumeRejected: true, + earlyData: true, + expectEarlyDataRejected: true, + expectations: connectionExpectations{echAccepted: true}, + resumeExpectations: &connectionExpectations{echAccepted: false}, + shouldFail: true, + expectedError: ":ECH_REJECTED:", + expectedLocalError: "remote error: ECH required", + }) } } diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 6288da461..12a9f7abd 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -60,6 +60,8 @@ const Flag kBoolFlags[] = { {"-fallback-scsv", &TestConfig::fallback_scsv}, {"-enable-ech-grease", &TestConfig::enable_ech_grease}, {"-expect-ech-accept", &TestConfig::expect_ech_accept}, + {"-expect-no-ech-name-override", &TestConfig::expect_no_ech_name_override}, + {"-expect-no-ech-retry-configs", &TestConfig::expect_no_ech_retry_configs}, {"-require-any-client-certificate", &TestConfig::require_any_client_certificate}, {"-false-start", &TestConfig::false_start}, @@ -167,6 +169,7 @@ const Flag kStringFlags[] = { {"-key-file", &TestConfig::key_file}, {"-cert-file", &TestConfig::cert_file}, {"-expect-server-name", &TestConfig::expect_server_name}, + {"-expect-ech-name-override", &TestConfig::expect_ech_name_override}, {"-advertise-npn", &TestConfig::advertise_npn}, {"-expect-next-proto", &TestConfig::expect_next_proto}, {"-select-next-proto", &TestConfig::select_next_proto}, @@ -201,6 +204,7 @@ const Flag> kOptionalStringFlags[] = { }; const Flag kBase64Flags[] = { + {"-expect-ech-retry-configs", &TestConfig::expect_ech_retry_configs}, {"-ech-config-list", &TestConfig::ech_config_list}, {"-expect-certificate-types", &TestConfig::expect_certificate_types}, {"-expect-channel-id", &TestConfig::expect_channel_id}, @@ -774,6 +778,20 @@ static bool CheckVerifyCallback(SSL *ssl) { } } + const char *name_override; + size_t name_override_len; + SSL_get0_ech_name_override(ssl, &name_override, &name_override_len); + if (config->expect_no_ech_name_override && name_override_len != 0) { + fprintf(stderr, "Unexpected ECH name override.\n"); + return false; + } + if (!config->expect_ech_name_override.empty() && + config->expect_ech_name_override != + std::string(name_override, name_override_len)) { + fprintf(stderr, "ECH name did not match expected value.\n"); + return false; + } + if (GetTestState(ssl)->cert_verified) { fprintf(stderr, "Certificate verified twice.\n"); return false; diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 377a75758..c9f2a254b 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -44,6 +44,10 @@ struct TestConfig { std::vector ech_server_keys; std::vector ech_is_retry_config; bool expect_ech_accept = false; + std::string expect_ech_name_override; + bool expect_no_ech_name_override = false; + std::string expect_ech_retry_configs; + bool expect_no_ech_retry_configs = false; std::string ech_config_list; std::string expect_certificate_types; bool require_any_client_certificate = false; diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 02922919c..bff7fb918 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -476,7 +476,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { ssl->s3->server_random + sizeof(ssl->s3->server_random) - sizeof(ech_confirmation), sizeof(ech_confirmation)) == 0) { - ssl->s3->ech_accept = true; + ssl->s3->ech_status = ssl_ech_accepted; hs->transcript = std::move(hs->inner_transcript); hs->extensions.sent = hs->inner_extensions_sent; // Report the inner random value through |SSL_get_client_random|. @@ -489,13 +489,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); return ssl_hs_error; } - - // TODO(https://crbug.com/boringssl/275): If the server declines ECH, we - // handshake with ClientHelloOuter instead of ClientHelloInner. That path - // is not yet implemented. For now, terminate the handshake with a - // distiguisable error for testing. - OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_REJECTED); - return ssl_hs_error; + ssl->s3->ech_status = ssl_ech_rejected; } } @@ -555,7 +549,7 @@ static enum ssl_hs_wait_t do_read_encrypted_extensions(SSL_HANDSHAKE *hs) { // If offering ECH, the server may not accept early data with // ClientHelloOuter. We do not offer sessions with ClientHelloOuter, so this // this should be implied by checking |session_reused|. - assert(hs->selected_ech_config == nullptr || ssl->s3->ech_accept); + assert(ssl->s3->ech_status != ssl_ech_rejected); if (hs->early_session->cipher != hs->new_session->cipher) { OPENSSL_PUT_ERROR(SSL, SSL_R_CIPHER_MISMATCH_ON_EARLY_DATA); @@ -835,8 +829,12 @@ static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) { return ssl_hs_ok; } - // Call cert_cb to update the certificate. - if (hs->config->cert->cert_cb != NULL) { + if (ssl->s3->ech_status == ssl_ech_rejected) { + // Do not send client certificates on ECH reject. We have not authenticated + // the server for the name that can learn the certificate. + SSL_certs_clear(ssl); + } else if (hs->config->cert->cert_cb != nullptr) { + // Call cert_cb to update the certificate. int rv = hs->config->cert->cert_cb(ssl, hs->config->cert->cert_cb_arg); if (rv == 0) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 7f32b6cd1..501d01fcb 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -601,7 +601,7 @@ static enum ssl_hs_wait_t do_read_second_client_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - if (ssl->s3->ech_accept) { + if (ssl->s3->ech_status == ssl_ech_accepted) { // If we previously accepted the ClientHelloInner, check that the second // ClientHello contains an encrypted_client_hello extension. CBS ech_body; @@ -761,7 +761,7 @@ static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - assert(!ssl->s3->ech_accept || hs->ech_is_inner_present); + assert(ssl->s3->ech_status != ssl_ech_accepted || hs->ech_is_inner_present); if (hs->ech_is_inner_present) { // Fill in the ECH confirmation signal. Span random_suffix =