Make SSL_get_servername work in the early callback. This avoids early callback users writing their own SNI parser and gives us a place to surface the server name from ESNI in the future. Update-Note: This isn't a breaking change, but users of SSL_CTX_set_select_certificate_cb can likely drop a bit of code after this CL. Bug: 275 Change-Id: I9685ae5cca8e0483de76229d12dac45ff8e9ec32 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36784 Reviewed-by: Steven Valdez <svaldez@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 9285b3f..c3735cd 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3714,6 +3714,8 @@ // high-level operation on |ssl| to be retried at a later time, which will // result in another call to |cb|. // +// |SSL_get_servername| may be used during this callback. +// // Note: The |SSL_CLIENT_HELLO| is only valid for the duration of the callback // and is not valid while the handshake is paused. OPENSSL_EXPORT void SSL_CTX_set_select_certificate_cb(
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 8ee39a2..36aa560 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc
@@ -503,6 +503,54 @@ return true; } +static bool extract_sni(SSL_HANDSHAKE *hs, uint8_t *out_alert, + const SSL_CLIENT_HELLO *client_hello) { + SSL *const ssl = hs->ssl; + CBS sni; + if (!ssl_client_hello_get_extension(client_hello, &sni, + TLSEXT_TYPE_server_name)) { + // No SNI extension to parse. + return true; + } + + CBS server_name_list, host_name; + uint8_t name_type; + if (!CBS_get_u16_length_prefixed(&sni, &server_name_list) || + !CBS_get_u8(&server_name_list, &name_type) || + // Although the server_name extension was intended to be extensible to + // new name types and multiple names, OpenSSL 1.0.x had a bug which meant + // different name types will cause an error. Further, RFC 4366 originally + // defined syntax inextensibly. RFC 6066 corrected this mistake, but + // adding new name types is no longer feasible. + // + // Act as if the extensibility does not exist to simplify parsing. + !CBS_get_u16_length_prefixed(&server_name_list, &host_name) || + CBS_len(&server_name_list) != 0 || + CBS_len(&sni) != 0) { + *out_alert = SSL_AD_DECODE_ERROR; + return false; + } + + if (name_type != TLSEXT_NAMETYPE_host_name || + CBS_len(&host_name) == 0 || + CBS_len(&host_name) > TLSEXT_MAXLEN_host_name || + CBS_contains_zero_byte(&host_name)) { + *out_alert = SSL_AD_UNRECOGNIZED_NAME; + return false; + } + + // Copy the hostname as a string. + char *raw = nullptr; + if (!CBS_strdup(&host_name, &raw)) { + *out_alert = SSL_AD_INTERNAL_ERROR; + return false; + } + ssl->s3->hostname.reset(raw); + + hs->should_ack_sni = true; + return true; +} + static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; @@ -526,6 +574,12 @@ return ssl_hs_handoff; } + uint8_t alert = SSL_AD_DECODE_ERROR; + if (!extract_sni(hs, &alert, &client_hello)) { + ssl_send_alert(ssl, SSL3_AL_FATAL, alert); + return ssl_hs_error; + } + // Run the early callback. if (ssl->ctx->select_certificate_cb != NULL) { switch (ssl->ctx->select_certificate_cb(&client_hello)) { @@ -553,7 +607,6 @@ hs->apply_jdk11_workaround = true; } - uint8_t alert = SSL_AD_DECODE_ERROR; if (!negotiate_version(hs, &alert, &client_hello)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error;
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 88685c8..c1c41a8 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc
@@ -633,45 +633,7 @@ static bool ext_sni_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert, CBS *contents) { - SSL *const ssl = hs->ssl; - if (contents == NULL) { - return true; - } - - CBS server_name_list, host_name; - uint8_t name_type; - if (!CBS_get_u16_length_prefixed(contents, &server_name_list) || - !CBS_get_u8(&server_name_list, &name_type) || - // Although the server_name extension was intended to be extensible to - // new name types and multiple names, OpenSSL 1.0.x had a bug which meant - // different name types will cause an error. Further, RFC 4366 originally - // defined syntax inextensibly. RFC 6066 corrected this mistake, but - // adding new name types is no longer feasible. - // - // Act as if the extensibility does not exist to simplify parsing. - !CBS_get_u16_length_prefixed(&server_name_list, &host_name) || - CBS_len(&server_name_list) != 0 || - CBS_len(contents) != 0) { - return false; - } - - if (name_type != TLSEXT_NAMETYPE_host_name || - CBS_len(&host_name) == 0 || - CBS_len(&host_name) > TLSEXT_MAXLEN_host_name || - CBS_contains_zero_byte(&host_name)) { - *out_alert = SSL_AD_UNRECOGNIZED_NAME; - return false; - } - - // Copy the hostname as a string. - char *raw = nullptr; - if (!CBS_strdup(&host_name, &raw)) { - *out_alert = SSL_AD_INTERNAL_ERROR; - return false; - } - ssl->s3->hostname.reset(raw); - - hs->should_ack_sni = true; + // SNI has already been parsed earlier in the handshake. See |extract_sni|. return true; }
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 19f94ba..a53aed2 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -1102,34 +1102,15 @@ GetTestState(client_hello->ssl)->early_callback_called = true; if (!config->expect_server_name.empty()) { - const uint8_t *extension_data; - size_t extension_len; - CBS extension, server_name_list, host_name; - uint8_t name_type; - - if (!SSL_early_callback_ctx_extension_get( - client_hello, TLSEXT_TYPE_server_name, &extension_data, - &extension_len)) { - fprintf(stderr, "Could not find server_name extension.\n"); + const char *server_name = + SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + if (server_name == nullptr || + std::string(server_name) != config->expect_server_name) { + fprintf(stderr, + "Server name mismatch in early callback (got %s; want %s).\n", + server_name, config->expect_server_name.c_str()); return ssl_select_cert_error; } - - CBS_init(&extension, extension_data, extension_len); - if (!CBS_get_u16_length_prefixed(&extension, &server_name_list) || - CBS_len(&extension) != 0 || - !CBS_get_u8(&server_name_list, &name_type) || - name_type != TLSEXT_NAMETYPE_host_name || - !CBS_get_u16_length_prefixed(&server_name_list, &host_name) || - CBS_len(&server_name_list) != 0) { - fprintf(stderr, "Could not decode server_name extension.\n"); - return ssl_select_cert_error; - } - - if (!CBS_mem_equal(&host_name, - (const uint8_t *)config->expect_server_name.data(), - config->expect_server_name.size())) { - fprintf(stderr, "Server name mismatch.\n"); - } } if (config->fail_early_callback) {