From 5c7508f641f48872c8094ba5b80892922c49c79a Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Fri, 18 Jul 2025 13:33:20 -0400 Subject: [PATCH 1/9] Impl SSL_client_hello_get1_extensions_present --- include/openssl/ssl.h | 12 +++++ ssl/ssl_client_hello_test.cc | 100 +++++++++++++++++++++++++++++++++++ ssl/ssl_lib.cc | 71 ++++++++++++++++++++++++- 3 files changed, 182 insertions(+), 1 deletion(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 5cc0142a2b..a14203c0e6 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1124,6 +1124,18 @@ OPENSSL_EXPORT size_t SSL_get0_peer_delegation_algorithms( // - SSL_CLIENT_HELLO_RETRY (not supported) is handled like SSL_CLIENT_HELLO_ERROR typedef int (*SSL_client_hello_cb_fn)(SSL *s, int *al, void *arg); +/* SSL_client_hello_get1_extensions_present searches the extensions in the + * ClientHello for all present extensions. If found, it allocates an array of + * int and sets |*out| to point to the array and |*outlen| to the number of + * extensions. The caller is responsible for releasing the array with + * OPENSSL_free. If no extensions are found, it sets |*out| to NULL and + * |*outlen| to 0. + * + * This function can only be called from within a client hello callback (see + * |SSL_CTX_set_client_hello_cb|) or during server certificate selection (see + * |SSL_CTX_set_select_certificate_cb|). */ +OPENSSL_EXPORT int SSL_client_hello_get1_extensions_present(SSL *s, int **out, size_t *outlen); + // SSL_CTX_set_client_hello_cb configures a callback that is called when a // ClientHello message is received. This can be used to select certificates, // adjust settings, or otherwise make decisions about the connection before diff --git a/ssl/ssl_client_hello_test.cc b/ssl/ssl_client_hello_test.cc index 37459e5248..1173bf0c7e 100644 --- a/ssl/ssl_client_hello_test.cc +++ b/ssl/ssl_client_hello_test.cc @@ -16,9 +16,12 @@ #include #include +#include #include "ssl_common_test.h" +#include + BSSL_NAMESPACE_BEGIN namespace { @@ -345,6 +348,103 @@ TEST(SSLClientHelloTest, ClientHelloKnownExtensions) { EXPECT_GT(results.supported_groups_len, 0u); } +int callback_SSL_client_hello_get1_extensions_present_impl( + SSL *ssl, int *al, void *arg, bool expect_session_ticket) { + auto *called = static_cast(arg); + *called = true; + + int *extensions = nullptr; + size_t extensions_len = 0; + if (!SSL_client_hello_get1_extensions_present(ssl, &extensions, + &extensions_len)) { + ADD_FAILURE() << "SSL_client_hello_get1_extensions_present failed"; + return SSL_CLIENT_HELLO_ERROR; + } + + EXPECT_GT(extensions_len, 0u); + EXPECT_NE(nullptr, extensions); + + // Verify a few common extensions are present + bool found_supported_groups = false; + bool found_session_ticket = false; + for (size_t i = 0; i < extensions_len; i++) { + if (extensions[i] == TLSEXT_TYPE_supported_groups) { + found_supported_groups = true; + } + if (extensions[i] == TLSEXT_TYPE_session_ticket) { + found_session_ticket = true; + } + } + EXPECT_TRUE(found_supported_groups); + EXPECT_TRUE(found_session_ticket == expect_session_ticket); + + OPENSSL_free(extensions); + + return SSL_CLIENT_HELLO_SUCCESS; +} + +// Global variable to store the expect_session_ticket value +bool g_expect_session_ticket = false; +int callback_wrapper(SSL *ssl, int *al, void *arg) { + return callback_SSL_client_hello_get1_extensions_present_impl( + ssl, al, arg, g_expect_session_ticket); +} + + +// Test SSL_client_hello_get1_extensions_present with a client hello that has +// extensions. +TEST(SSLClientHelloTest, ExtensionsPresent) { + UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + + g_expect_session_ticket = true; + + SSL_CTX_set_info_callback( + client_ctx.get(), [](const SSL *ssl, int type, int val) { + if (type == SSL_CB_HANDSHAKE_START) { + ASSERT_TRUE( + SSL_set_tlsext_host_name(const_cast(ssl), "example.com")); + } + }); + + bool callback_called = false; + SSL_CTX_set_client_hello_cb(server_ctx.get(), callback_wrapper, + &callback_called); + + UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + EXPECT_TRUE(callback_called); +} + +// Test SSL_client_hello_get1_extensions_present with a client hello that has +// no session ticket extension. +TEST(SSLClientHelloTest, NoTicketExtensionPresent) { + UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + + g_expect_session_ticket = false; + // Disable all extensions on the client to simulate a "no extensions" scenario + // Note: This is a bit artificial as the library might add some extensions + // by default. We rely on the callback to check the result. + SSL_CTX_set_options(client_ctx.get(), SSL_OP_NO_TICKET); + + bool callback_called = false; + SSL_CTX_set_client_hello_cb(server_ctx.get(), callback_wrapper, + &callback_called); + + UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + EXPECT_TRUE(callback_called); +} + } // namespace BSSL_NAMESPACE_END diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 3326bfea23..d2015df1ed 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3081,6 +3081,76 @@ int SSL_client_hello_get0_ext(SSL *s, unsigned int type, const unsigned char **o return 1; // Success } +int SSL_client_hello_get1_extensions_present(SSL *s, int **out, + size_t *outlen) { + GUARD_PTR(s); + GUARD_PTR(s->s3); + SSL_HANDSHAKE *hs = s->s3->hs.get(); + GUARD_PTR(hs); + + if (out == nullptr || outlen == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER); + return 0; + } + + *out = nullptr; + *outlen = 0; + + SSLMessage msg_unused; + SSL_CLIENT_HELLO client_hello; + if (!hs->GetClientHello(&msg_unused, &client_hello)) { + return 0; + } + + CBS extensions; + CBS_init(&extensions, client_hello.extensions, client_hello.extensions_len); + size_t num_extensions = 0; + CBS extensions_copy = extensions; + + // Count the number of extensions so we can allocate + while (CBS_len(&extensions_copy) > 0) { + uint16_t type; + CBS body; + if (!CBS_get_u16(&extensions_copy, &type) || + !CBS_get_u16_length_prefixed(&extensions_copy, &body)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + return 0; + } + num_extensions++; + } + + if (num_extensions == 0) { + return 1; + } + + // Allocate an int for each extension + int *ext_types = + static_cast(OPENSSL_malloc(sizeof(int) * num_extensions)); + if (ext_types == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + return 0; + } + + // Store the type for each extension + size_t i = 0; + while (CBS_len(&extensions) > 0) { + uint16_t type; + CBS body; + if (!CBS_get_u16(&extensions, &type) || + !CBS_get_u16_length_prefixed(&extensions, &body)) { + OPENSSL_free(ext_types); + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + return 0; + } + ext_types[i++] = type; + } + + *out = ext_types; + *outlen = num_extensions; + + return 1; +} + void SSL_CTX_set_keylog_callback(SSL_CTX *ctx, void (*cb)(const SSL *ssl, const char *line)) { ctx->keylog_callback = cb; @@ -3655,4 +3725,3 @@ OPENSSL_EXPORT int SSL_get_write_traffic_secret( int SSL_verify_client_post_handshake(SSL *ssl) { return 0; } - From 19e3a2a11c8aab94e3236cc4ec66b820264d6164 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Mon, 21 Jul 2025 11:14:54 -0400 Subject: [PATCH 2/9] Impl SSL_client_hello_get_extension_order --- include/openssl/ssl.h | 36 ++++++++++++----- ssl/ssl_lib.cc | 92 +++++++++++++++++++++++++------------------ 2 files changed, 80 insertions(+), 48 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index a14203c0e6..5d056d713a 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1124,18 +1124,34 @@ OPENSSL_EXPORT size_t SSL_get0_peer_delegation_algorithms( // - SSL_CLIENT_HELLO_RETRY (not supported) is handled like SSL_CLIENT_HELLO_ERROR typedef int (*SSL_client_hello_cb_fn)(SSL *s, int *al, void *arg); -/* SSL_client_hello_get1_extensions_present searches the extensions in the - * ClientHello for all present extensions. If found, it allocates an array of - * int and sets |*out| to point to the array and |*outlen| to the number of - * extensions. The caller is responsible for releasing the array with - * OPENSSL_free. If no extensions are found, it sets |*out| to NULL and - * |*outlen| to 0. - * - * This function can only be called from within a client hello callback (see - * |SSL_CTX_set_client_hello_cb|) or during server certificate selection (see - * |SSL_CTX_set_select_certificate_cb|). */ +// SSL_client_hello_get1_extensions_present iterates over the extensions in the +// ClientHello. If any are found, it allocates an array of int and sets |*out| +// to point to this array and |*outlen| to the number of extensions. The ints +// in the array correspond to the type of each extension. The caller is +// responsible for releasing the array with OPENSSL_free. If no extensions are +// found, it sets |*out| to NULL and |*outlen| to 0. The function returns 1 on +// success and returns 0 on error. +// +// This function can only be called from within a client hello callback (see +// |SSL_CTX_set_client_hello_cb|) or during server certificate selection (see +// |SSL_CTX_set_select_certificate_cb|). OPENSSL_EXPORT int SSL_client_hello_get1_extensions_present(SSL *s, int **out, size_t *outlen); +// SSL_client_hello_get_extension_order iterates over the extensions in the +// ClientHello. If |exts| is not null, the type for each extension will be +// stored in |exts| and |*num_exts| should be the size of storage +// allocated for |exts|; the function will return an error if |*num_exts| is +// too small. On success, the function will return 1 and will set |*num_exts| to +// the number of extensions. The caller may pass |exts| as null to obtain the +// number of extensions. If no ClientHello extensions are found, the +// function returns 1 and sets |*num_exts| to 0. The functions returns 0 on +// error. +// +// This function can only be called from within a client hello callback (see +// |SSL_CTX_set_client_hello_cb|) or during server certificate selection (see +// |SSL_CTX_set_select_certificate_cb|). +OPENSSL_EXPORT int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_exts); + // SSL_CTX_set_client_hello_cb configures a callback that is called when a // ClientHello message is received. This can be used to select certificates, // adjust settings, or otherwise make decisions about the connection before diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index d2015df1ed..41c4eeba46 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3083,70 +3083,86 @@ int SSL_client_hello_get0_ext(SSL *s, unsigned int type, const unsigned char **o int SSL_client_hello_get1_extensions_present(SSL *s, int **out, size_t *outlen) { - GUARD_PTR(s); - GUARD_PTR(s->s3); - SSL_HANDSHAKE *hs = s->s3->hs.get(); - GUARD_PTR(hs); + size_t num_extensions = 0; - if (out == nullptr || outlen == nullptr) { - OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER); + // Count the number of extensions so we can allocate + if (1 != SSL_client_hello_get_extension_order(s, nullptr, &num_extensions)) { return 0; } - *out = nullptr; - *outlen = 0; + if (num_extensions == 0) { + return 1; + } - SSLMessage msg_unused; - SSL_CLIENT_HELLO client_hello; - if (!hs->GetClientHello(&msg_unused, &client_hello)) { + // Allocate a uint16_t for each extension + uint16_t *exts = + static_cast(OPENSSL_zalloc(sizeof(uint16_t) * num_extensions)); + if (exts == nullptr) { return 0; } - CBS extensions; - CBS_init(&extensions, client_hello.extensions, client_hello.extensions_len); - size_t num_extensions = 0; - CBS extensions_copy = extensions; + // Collect the type for each extension + if (1 != SSL_client_hello_get_extension_order(s, exts, &num_extensions)) { + OPENSSL_free(exts); + return 0; + } - // Count the number of extensions so we can allocate - while (CBS_len(&extensions_copy) > 0) { - uint16_t type; - CBS body; - if (!CBS_get_u16(&extensions_copy, &type) || - !CBS_get_u16_length_prefixed(&extensions_copy, &body)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); - return 0; - } - num_extensions++; + // Allocate the int array needed by caller. + int *ext_types = + static_cast(OPENSSL_zalloc(sizeof(int) * num_extensions)); + if (ext_types == nullptr) { + OPENSSL_free(exts); + return 0; } - if (num_extensions == 0) { - return 1; + // Cast each uint16_t type to an int + for (size_t i = 0; i < num_extensions; i++) { + ext_types[i] = exts[i]; } + OPENSSL_free(exts); - // Allocate an int for each extension - int *ext_types = - static_cast(OPENSSL_malloc(sizeof(int) * num_extensions)); - if (ext_types == nullptr) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + *out = ext_types; + *outlen = num_extensions; + + return 1; +} + +int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_exts) { + GUARD_PTR(s); + GUARD_PTR(s->s3); + SSL_HANDSHAKE *hs = s->s3->hs.get(); + GUARD_PTR(hs); + + SSLMessage msg_unused; + SSL_CLIENT_HELLO client_hello; + if (!hs->GetClientHello(&msg_unused, &client_hello)) { return 0; } - // Store the type for each extension + CBS extensions; + CBS_init(&extensions, client_hello.extensions, client_hello.extensions_len); + + size_t num_extensions = 0; size_t i = 0; while (CBS_len(&extensions) > 0) { uint16_t type; CBS body; if (!CBS_get_u16(&extensions, &type) || !CBS_get_u16_length_prefixed(&extensions, &body)) { - OPENSSL_free(ext_types); OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); return 0; } - ext_types[i++] = type; + if (exts != nullptr) { + // num_exts is an in/out param. Return error if insufficient size. + if (i >= *num_exts) { + return 0; + } + // Store the type for each extension + exts[i++] = type; + } + num_extensions++; } - - *out = ext_types; - *outlen = num_extensions; + *num_exts = num_extensions; return 1; } From 69a6733e6b9bd1c63fa689105b186fbc150f23fd Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Mon, 21 Jul 2025 14:16:28 -0400 Subject: [PATCH 3/9] Improve tests --- ssl/ssl_client_hello_test.cc | 104 ++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 19 deletions(-) diff --git a/ssl/ssl_client_hello_test.cc b/ssl/ssl_client_hello_test.cc index 1173bf0c7e..fa4349ae09 100644 --- a/ssl/ssl_client_hello_test.cc +++ b/ssl/ssl_client_hello_test.cc @@ -348,10 +348,15 @@ TEST(SSLClientHelloTest, ClientHelloKnownExtensions) { EXPECT_GT(results.supported_groups_len, 0u); } +struct ExtensionsPresentTestArgs { + bool *called; + bool expect_session_ticket; +}; + int callback_SSL_client_hello_get1_extensions_present_impl( - SSL *ssl, int *al, void *arg, bool expect_session_ticket) { - auto *called = static_cast(arg); - *called = true; + SSL *ssl, int *al, void *arg) { + auto *args = static_cast(arg); + *(args->called) = true; int *extensions = nullptr; size_t extensions_len = 0; @@ -376,21 +381,13 @@ int callback_SSL_client_hello_get1_extensions_present_impl( } } EXPECT_TRUE(found_supported_groups); - EXPECT_TRUE(found_session_ticket == expect_session_ticket); + EXPECT_EQ(found_session_ticket, args->expect_session_ticket); OPENSSL_free(extensions); return SSL_CLIENT_HELLO_SUCCESS; } -// Global variable to store the expect_session_ticket value -bool g_expect_session_ticket = false; -int callback_wrapper(SSL *ssl, int *al, void *arg) { - return callback_SSL_client_hello_get1_extensions_present_impl( - ssl, al, arg, g_expect_session_ticket); -} - - // Test SSL_client_hello_get1_extensions_present with a client hello that has // extensions. TEST(SSLClientHelloTest, ExtensionsPresent) { @@ -400,8 +397,6 @@ TEST(SSLClientHelloTest, ExtensionsPresent) { ASSERT_TRUE(client_ctx); ASSERT_TRUE(server_ctx); - g_expect_session_ticket = true; - SSL_CTX_set_info_callback( client_ctx.get(), [](const SSL *ssl, int type, int val) { if (type == SSL_CB_HANDSHAKE_START) { @@ -411,8 +406,11 @@ TEST(SSLClientHelloTest, ExtensionsPresent) { }); bool callback_called = false; - SSL_CTX_set_client_hello_cb(server_ctx.get(), callback_wrapper, - &callback_called); + ExtensionsPresentTestArgs args = {&callback_called, + true /* expect_session_ticket */}; + SSL_CTX_set_client_hello_cb( + server_ctx.get(), callback_SSL_client_hello_get1_extensions_present_impl, + &args); UniquePtr client, server; ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), @@ -429,15 +427,83 @@ TEST(SSLClientHelloTest, NoTicketExtensionPresent) { ASSERT_TRUE(client_ctx); ASSERT_TRUE(server_ctx); - g_expect_session_ticket = false; // Disable all extensions on the client to simulate a "no extensions" scenario // Note: This is a bit artificial as the library might add some extensions // by default. We rely on the callback to check the result. SSL_CTX_set_options(client_ctx.get(), SSL_OP_NO_TICKET); bool callback_called = false; - SSL_CTX_set_client_hello_cb(server_ctx.get(), callback_wrapper, - &callback_called); + ExtensionsPresentTestArgs args = {&callback_called, + false /* expect_session_ticket */}; + SSL_CTX_set_client_hello_cb( + server_ctx.get(), callback_SSL_client_hello_get1_extensions_present_impl, + &args); + + UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + EXPECT_TRUE(callback_called); +} + +// Test SSL_client_hello_get_extension_order to verify its behavior with +// different buffer sizes and to ensure it correctly reports the number of +// extensions. +TEST(SSLClientHelloTest, GetExtensionOrder) { + UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + + bool callback_called = false; + SSL_CTX_set_client_hello_cb( + server_ctx.get(), + [](SSL *ssl, int *al, void *arg) -> int { + bool *called = static_cast(arg); + *called = true; + + size_t num_extensions = 0; + // First, call with a null buffer to get the count of extensions. + if (SSL_client_hello_get_extension_order(ssl, nullptr, + &num_extensions) != 1) { + return SSL_CLIENT_HELLO_ERROR; + } + EXPECT_GT(num_extensions, 0u); + + // Allocate a buffer of the correct size and get the extensions. + uint16_t* exts = + (uint16_t *)OPENSSL_zalloc(sizeof(uint16_t) * num_extensions); + if (exts == nullptr) { + return SSL_CLIENT_HELLO_ERROR; + } + if (SSL_client_hello_get_extension_order( + ssl, exts, &num_extensions) != 1) { + OPENSSL_free(exts); + return SSL_CLIENT_HELLO_ERROR; + } + + // Call with a buffer that is too small and confirm it fails. + size_t too_small_num_extensions = num_extensions - 1; + uint16_t* too_small_exts = + (uint16_t *)OPENSSL_zalloc(sizeof(uint16_t) * + too_small_num_extensions); + if (!too_small_exts) { + OPENSSL_free(exts); + return SSL_CLIENT_HELLO_ERROR; + } + // Expect failure + if (SSL_client_hello_get_extension_order( + ssl, too_small_exts, &too_small_num_extensions) != 0) { + OPENSSL_free(exts); + OPENSSL_free(too_small_exts); + return SSL_CLIENT_HELLO_ERROR; + } + OPENSSL_free(exts); + OPENSSL_free(too_small_exts); + + return SSL_CLIENT_HELLO_SUCCESS; + }, + &callback_called); UniquePtr client, server; ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), From 3789314bcddf5f14d564baec634599187e9104c7 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Mon, 21 Jul 2025 14:17:25 -0400 Subject: [PATCH 4/9] Satisfay clang-tidy --- ssl/ssl_lib.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 41c4eeba46..9683d4c904 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3145,7 +3145,7 @@ int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_ext size_t num_extensions = 0; size_t i = 0; while (CBS_len(&extensions) > 0) { - uint16_t type; + uint16_t type = 0; CBS body; if (!CBS_get_u16(&extensions, &type) || !CBS_get_u16_length_prefixed(&extensions, &body)) { From b1bb5d827f0fe04675e724ade0607d42371193e9 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Tue, 22 Jul 2025 09:35:11 -0400 Subject: [PATCH 5/9] Impl SSL_client_hello_get0_legacy_version --- include/openssl/ssl.h | 9 +++++++++ ssl/ssl_client_hello_test.cc | 6 ++++++ ssl/ssl_lib.cc | 14 ++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 5d056d713a..015e8c981d 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1172,6 +1172,15 @@ OPENSSL_EXPORT void SSL_CTX_set_client_hello_cb(SSL_CTX *c, SSL_client_hello_cb_ // SSL_client_hello_isv2 always returns zero as SSLv2 is not supported. OPENSSL_EXPORT int SSL_client_hello_isv2(SSL *s); + +// SSL_client_hello_get0_legacy_version provides the value of the +// "legacy_version" field in the client hello. +// +// This function can only be called from within a client hello callback (see +// |SSL_CTX_set_client_hello_cb|) or during server certificate selection (see +// |SSL_CTX_set_select_certificate_cb|). +OPENSSL_EXPORT unsigned int SSL_client_hello_get0_legacy_version(SSL *s); + // SSL_client_hello_get0_ext searches the extensions in the ClientHello for an // extension of the given type. If found, it sets |*out| to point to the // extension contents (not including the type and length bytes) and |*outlen| diff --git a/ssl/ssl_client_hello_test.cc b/ssl/ssl_client_hello_test.cc index fa4349ae09..4c7aa71312 100644 --- a/ssl/ssl_client_hello_test.cc +++ b/ssl/ssl_client_hello_test.cc @@ -369,6 +369,9 @@ int callback_SSL_client_hello_get1_extensions_present_impl( EXPECT_GT(extensions_len, 0u); EXPECT_NE(nullptr, extensions); + unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); + EXPECT_NE(legacy_version, 0u); + // Verify a few common extensions are present bool found_supported_groups = false; bool found_session_ticket = false; @@ -482,6 +485,9 @@ TEST(SSLClientHelloTest, GetExtensionOrder) { return SSL_CLIENT_HELLO_ERROR; } + unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); + EXPECT_NE(legacy_version, 0u); + // Call with a buffer that is too small and confirm it fails. size_t too_small_num_extensions = num_extensions - 1; uint16_t* too_small_exts = diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 9683d4c904..7c980d18bb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3167,6 +3167,20 @@ int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_ext return 1; } +unsigned int SSL_client_hello_get0_legacy_version(SSL *s) { + GUARD_PTR(s); + GUARD_PTR(s->s3); + SSL_HANDSHAKE *hs = s->s3->hs.get(); + GUARD_PTR(hs); + + SSLMessage msg_unused; + SSL_CLIENT_HELLO client_hello; + if (!hs->GetClientHello(&msg_unused, &client_hello)) { + return 0; + } + return client_hello.version; +} + void SSL_CTX_set_keylog_callback(SSL_CTX *ctx, void (*cb)(const SSL *ssl, const char *line)) { ctx->keylog_callback = cb; From e5307ec30b749e5037b6f3cdbfa4de4bc2a872ad Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Tue, 22 Jul 2025 10:16:27 -0400 Subject: [PATCH 6/9] mod_ssl needs hmac include --- include/openssl/evp.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/openssl/evp.h b/include/openssl/evp.h index 162aa464c0..83a9130e9d 100644 --- a/include/openssl/evp.h +++ b/include/openssl/evp.h @@ -70,6 +70,7 @@ #include #include #include +#include // Needed by Apache mod_ssl #if defined(__cplusplus) extern "C" { From 3c4fcc83b87567da1d83e8be0e15cfce345022f5 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Tue, 22 Jul 2025 10:24:25 -0400 Subject: [PATCH 7/9] Handle null params --- ssl/ssl_lib.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 7c980d18bb..5e6f993271 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3083,6 +3083,9 @@ int SSL_client_hello_get0_ext(SSL *s, unsigned int type, const unsigned char **o int SSL_client_hello_get1_extensions_present(SSL *s, int **out, size_t *outlen) { + GUARD_PTR(s); + GUARD_PTR(out); + GUARD_PTR(outlen); size_t num_extensions = 0; // Count the number of extensions so we can allocate @@ -3091,6 +3094,8 @@ int SSL_client_hello_get1_extensions_present(SSL *s, int **out, } if (num_extensions == 0) { + *out = nullptr; + *outlen = 0; return 1; } From dc2eb4a58ba98925d935fd6ab775437ca056d6b0 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Fri, 8 Aug 2025 11:23:09 -0400 Subject: [PATCH 8/9] Improve testing of SSL_client_hello_get0_legacy_version --- ssl/ssl_client_hello_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ssl/ssl_client_hello_test.cc b/ssl/ssl_client_hello_test.cc index 4c7aa71312..5b40087dd1 100644 --- a/ssl/ssl_client_hello_test.cc +++ b/ssl/ssl_client_hello_test.cc @@ -370,7 +370,7 @@ int callback_SSL_client_hello_get1_extensions_present_impl( EXPECT_NE(nullptr, extensions); unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); - EXPECT_NE(legacy_version, 0u); + EXPECT_EQ(legacy_version, (unsigned)TLS1_2_VERSION); // Verify a few common extensions are present bool found_supported_groups = false; @@ -486,7 +486,7 @@ TEST(SSLClientHelloTest, GetExtensionOrder) { } unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); - EXPECT_NE(legacy_version, 0u); + EXPECT_EQ(legacy_version, (unsigned)TLS1_2_VERSION); // Call with a buffer that is too small and confirm it fails. size_t too_small_num_extensions = num_extensions - 1; From a6c2375bafbdf1f98d51e80c336d3a5e342209f7 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Mon, 11 Aug 2025 07:22:57 -0400 Subject: [PATCH 9/9] PR feedback --- ssl/ssl_client_hello_test.cc | 25 ++++++++++++++++--------- ssl/ssl_lib.cc | 5 ++--- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/ssl/ssl_client_hello_test.cc b/ssl/ssl_client_hello_test.cc index 5b40087dd1..58f2b44399 100644 --- a/ssl/ssl_client_hello_test.cc +++ b/ssl/ssl_client_hello_test.cc @@ -367,7 +367,7 @@ int callback_SSL_client_hello_get1_extensions_present_impl( } EXPECT_GT(extensions_len, 0u); - EXPECT_NE(nullptr, extensions); + EXPECT_TRUE(extensions); unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); EXPECT_EQ(legacy_version, (unsigned)TLS1_2_VERSION); @@ -469,32 +469,37 @@ TEST(SSLClientHelloTest, GetExtensionOrder) { // First, call with a null buffer to get the count of extensions. if (SSL_client_hello_get_extension_order(ssl, nullptr, &num_extensions) != 1) { + ADD_FAILURE() + << "Failed initial call to SSL_client_hello_get_extension_order"; return SSL_CLIENT_HELLO_ERROR; } EXPECT_GT(num_extensions, 0u); // Allocate a buffer of the correct size and get the extensions. - uint16_t* exts = - (uint16_t *)OPENSSL_zalloc(sizeof(uint16_t) * num_extensions); + uint16_t *exts = static_cast( + OPENSSL_zalloc(sizeof(uint16_t) * num_extensions)); if (exts == nullptr) { + ADD_FAILURE() << "Failed to allocate extensions"; return SSL_CLIENT_HELLO_ERROR; } - if (SSL_client_hello_get_extension_order( - ssl, exts, &num_extensions) != 1) { + if (SSL_client_hello_get_extension_order(ssl, exts, &num_extensions) != + 1) { + ADD_FAILURE() + << "Failed call to SSL_client_hello_get_extension_order"; OPENSSL_free(exts); return SSL_CLIENT_HELLO_ERROR; } unsigned legacy_version = SSL_client_hello_get0_legacy_version(ssl); - EXPECT_EQ(legacy_version, (unsigned)TLS1_2_VERSION); + EXPECT_EQ(legacy_version, static_cast(TLS1_2_VERSION)); // Call with a buffer that is too small and confirm it fails. size_t too_small_num_extensions = num_extensions - 1; - uint16_t* too_small_exts = - (uint16_t *)OPENSSL_zalloc(sizeof(uint16_t) * - too_small_num_extensions); + uint16_t *too_small_exts = static_cast( + OPENSSL_zalloc(sizeof(uint16_t) * too_small_num_extensions)); if (!too_small_exts) { OPENSSL_free(exts); + ADD_FAILURE() << "Failed to allocate too small buffer"; return SSL_CLIENT_HELLO_ERROR; } // Expect failure @@ -502,6 +507,8 @@ TEST(SSLClientHelloTest, GetExtensionOrder) { ssl, too_small_exts, &too_small_num_extensions) != 0) { OPENSSL_free(exts); OPENSSL_free(too_small_exts); + ADD_FAILURE() + << "Failed call to SSL_client_hello_get_extension_order"; return SSL_CLIENT_HELLO_ERROR; } OPENSSL_free(exts); diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 5e6f993271..91872f805a 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3148,7 +3148,6 @@ int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_ext CBS_init(&extensions, client_hello.extensions, client_hello.extensions_len); size_t num_extensions = 0; - size_t i = 0; while (CBS_len(&extensions) > 0) { uint16_t type = 0; CBS body; @@ -3159,11 +3158,11 @@ int SSL_client_hello_get_extension_order(SSL *s, uint16_t *exts, size_t *num_ext } if (exts != nullptr) { // num_exts is an in/out param. Return error if insufficient size. - if (i >= *num_exts) { + if (num_extensions >= *num_exts) { return 0; } // Store the type for each extension - exts[i++] = type; + exts[num_extensions] = type; } num_extensions++; }