From 364d28bb38b68c0009e3e94aa085de5a46756f12 Mon Sep 17 00:00:00 2001 From: Shubham Mittal <107728331+smittals2@users.noreply.github.com> Date: Wed, 1 May 2024 16:41:41 -0700 Subject: [PATCH] Changed SSL_client_hello_get0_ciphers to align with OpenSSL behavior (#1542) ### Description of changes: This PR aligns SSL_client_hello_get0_ciphers with OpenSSL's behavior. Previously, it returned only the ciphers supported by our library. Now, it offers all ciphersuites from the client hello, including unsupported ones. 1. New fields added to SSL struct, |all_client_cipher_suites| and |all_client_cipher_suites_len| 2. |ssl_parse_client_cipher_list| modified to also populate |ssl->all_client_cipher_suites|. Function signature also modified to take in an SSL object 3. Rewrote | SSL_client_hello_get0_ciphers| 4. Changed documentation to better callout the differences between which ciphersuites are returned (supported or unsupported) 5. Added tests for TLS 1.2 and 1.3 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- include/openssl/ssl.h | 12 ++-- ssl/handshake_server.cc | 12 +++- ssl/internal.h | 24 ++++--- ssl/ssl_lib.cc | 31 ++------ ssl/ssl_test.cc | 154 ++++++++++++++++++++++++++++++++++++---- ssl/tls13_server.cc | 2 +- 6 files changed, 179 insertions(+), 56 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 2dcb53fd96..7e1f4987b6 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1849,16 +1849,18 @@ OPENSSL_EXPORT int SSL_get_extms_support(const SSL *ssl); // not been negotiated yet. OPENSSL_EXPORT const SSL_CIPHER *SSL_get_current_cipher(const SSL *ssl); -// SSL_get_client_ciphers returns stack of ciphers offered by the client during -// a handshake. If the |ssl| is a client or the handshake hasn't occurred yet, -// NULL is returned. The stack of ciphers IS NOT de/serialized, so NULL will -// also be returned for deserialized or transported |ssl|'s that haven't yet -// performed a new handshake. +// SSL_get_client_ciphers returns the stack of ciphers offered by the client +// during a handshake and that are supported by this library. If |ssl| is a +// client or the handshake hasn't occurred yet, NULL is returned. The stack of +// ciphers IS NOT de/serialized, so NULL will also be returned for deserialized +// or transported |ssl|'s that haven't yet performed a new handshake. OPENSSL_EXPORT STACK_OF(SSL_CIPHER) *SSL_get_client_ciphers(const SSL *ssl); // SSL_client_hello_get0_ciphers provides access to the client ciphers field from the // Client Hello, optionally writing the result to an out pointer. It returns the field // length if successful, or 0 if |ssl| is a client or the handshake hasn't occurred yet. +// |out| points to the raw bytes from the client hello message so it may contain invalid +// or unsupported Cipher IDs. OPENSSL_EXPORT size_t SSL_client_hello_get0_ciphers(SSL *ssl, const unsigned char **out); // SSL_session_reused returns one if |ssl| performed an abbreviated handshake diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 9d185d6f95..7dc241aed4 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -265,13 +265,18 @@ static bool negotiate_version(SSL_HANDSHAKE *hs, uint8_t *out_alert, } bool ssl_parse_client_cipher_list( - const SSL_CLIENT_HELLO *client_hello, UniquePtr *ciphers_out) { + SSL *ssl, const SSL_CLIENT_HELLO *client_hello, UniquePtr *ciphers_out) { ciphers_out->reset(); CBS cipher_suites; CBS_init(&cipher_suites, client_hello->cipher_suites, client_hello->cipher_suites_len); + // Store raw bytes for cipher suites offered + ssl->all_client_cipher_suites.reset(static_cast(OPENSSL_memdup( + client_hello->cipher_suites, client_hello->cipher_suites_len))); + ssl->all_client_cipher_suites_len = client_hello->cipher_suites_len; + // Cipher suites are encoded as 2-byte unsigned integers if (CBS_len(&cipher_suites) % 2 != 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST); @@ -282,7 +287,7 @@ bool ssl_parse_client_cipher_list( if (!sk) { return false; } - + while (CBS_len(&cipher_suites) > 0) { uint16_t cipher_suite; @@ -291,6 +296,7 @@ bool ssl_parse_client_cipher_list( return false; } + // Storing only supported ciphers const SSL_CIPHER *c = SSL_get_cipher_by_value(cipher_suite); if (c != NULL && !sk_SSL_CIPHER_push(sk.get(), c)) { OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST); @@ -814,7 +820,7 @@ static enum ssl_hs_wait_t do_select_certificate(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - if (!ssl_parse_client_cipher_list(&client_hello, &ssl->client_cipher_suites)) { + if (!ssl_parse_client_cipher_list(ssl, &client_hello, &ssl->client_cipher_suites)) { OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); return ssl_hs_error; diff --git a/ssl/internal.h b/ssl/internal.h index 426cca81b4..b4fde1f4e1 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -2451,9 +2451,10 @@ bool ssl_client_cipher_list_contains_cipher( const SSL_CLIENT_HELLO *client_hello, uint16_t id); // ssl_parse_client_cipher_list returns the ciphers offered by the client -// during handshake, or null if the handshake hasn't occurred or there was an -// error. -bool ssl_parse_client_cipher_list(const SSL_CLIENT_HELLO *client_hello, +// during handshake that are supported by this library, or null if the handshake hasn't +// occurred or there was an error. It also stores the unparsed raw bytes of cipher suites offered in +// the client hello into |ssl->all_client_cipher_suites|. +bool ssl_parse_client_cipher_list(SSL *ssl, const SSL_CLIENT_HELLO *client_hello, UniquePtr *ciphers_out); @@ -4064,14 +4065,19 @@ struct ssl_st { bssl::UniquePtr session; // client_cipher_suites contains cipher suites offered by the client during - // the handshake, with preference order maintained. This field is NOT - // serialized and is only populated if used in a server context. + // the handshake and that are supported by this library, with preference order + // maintained. This field is NOT serialized and is only populated if used in + // a server context. bssl::UniquePtr client_cipher_suites; - // client_cipher_suites_arr is initialized when |SSL_client_hello_get0_ciphers| - // is called with a valid |out| param. It holds the cipher suites offered by the - // client from |client_cipher_suites| in an array. - bssl::Array client_cipher_suites_arr; + // all_client_cipher_suites contains the raw bytes for cipher suites offered + // by the client during the handshake (including those not supported by this + // library), with preference order maintained. This field may contain + // cipher IDs that are invalid. + bssl::UniquePtr all_client_cipher_suites; + + // Field length of ciphers in client hello. Maximum allowed size is 2^16 + uint16_t all_client_cipher_suites_len = 0; void (*info_callback)(const SSL *ssl, int type, int value) = nullptr; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 0effa208ee..b292a9b557 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -3085,7 +3085,7 @@ int SSL_clear(SSL *ssl) { } ssl->client_cipher_suites.reset(); - ssl->client_cipher_suites_arr.Reset(); + ssl->all_client_cipher_suites.reset(); // In OpenSSL, reusing a client |SSL| with |SSL_clear| causes the previously // established session to be offered the next time around. wpa_supplicant @@ -3311,33 +3311,14 @@ int SSL_set1_curves_list(SSL *ssl, const char *curves) { } size_t SSL_client_hello_get0_ciphers(SSL *ssl, const unsigned char **out) { - STACK_OF(SSL_CIPHER) *client_cipher_suites = SSL_get_client_ciphers(ssl); - if (client_cipher_suites == nullptr) { + if (SSL_get_client_ciphers(ssl) == nullptr) { return 0; } + const char * ciphers = ssl->all_client_cipher_suites.get(); + assert(ciphers != nullptr); - size_t num_ciphers = sk_SSL_CIPHER_num(client_cipher_suites); if (out != nullptr) { - // Hasn't been called before - if(ssl->client_cipher_suites_arr.empty()) { - if (!ssl->client_cipher_suites_arr.Init(num_ciphers)) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); - return 0; - } - - // Construct list of cipherIDs - for (size_t i = 0; i < num_ciphers; i++) { - const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(client_cipher_suites, i); - uint16_t iana_id = SSL_CIPHER_get_protocol_id(cipher); - - CRYPTO_store_u16_be(&ssl->client_cipher_suites_arr[i], iana_id); - } - } - - assert(ssl->client_cipher_suites_arr.size() == num_ciphers); - *out = reinterpret_cast(ssl->client_cipher_suites_arr.data()); + *out = reinterpret_cast(ciphers); } - - // Return the size - return num_ciphers; + return ssl->all_client_cipher_suites_len; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index c8335de560..2a8f8698b9 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -2411,16 +2411,79 @@ TEST(SSLTest, FindingCipher) { ASSERT_FALSE(cipher3); } -TEST(SSLTest, GetClientCiphers) { +TEST(SSLTest, GetClientCiphersAfterHandshakeFailure1_3) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); - bssl::UniquePtr server_ctx = - CreateContextWithTestCertificate(TLS_method()); + bssl::UniquePtr server_ctx = CreateContextWithTestCertificate(TLS_method()); + + // configure client to add fake ciphersuite + SSL_CTX_set_grease_enabled(client_ctx.get(), 1); + + // There will be no cipher match, handshake will not succeed. + ASSERT_TRUE(SSL_CTX_set_ciphersuites(client_ctx.get(), + "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256")); + ASSERT_TRUE(SSL_CTX_set_ciphersuites(server_ctx.get(), + "TLS_AES_128_GCM_SHA256")); + + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + + ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION)); + + bssl::UniquePtr client, server; + ASSERT_TRUE(CreateClientAndServer(&client, &server, + client_ctx.get(), server_ctx.get())); + + const unsigned char *tmp = nullptr; + // Handshake not completed, getting ciphers should fail + ASSERT_FALSE(SSL_client_hello_get0_ciphers(client.get(), &tmp)); + ASSERT_FALSE(SSL_client_hello_get0_ciphers(server.get(), &tmp)); + ASSERT_FALSE(tmp); + + // This should fail, but should be able to inspect client ciphers still + ASSERT_FALSE(CompleteHandshakes(client.get(), server.get())); + + ASSERT_EQ(SSL_client_hello_get0_ciphers(client.get(), nullptr), (size_t) 0); + + const unsigned char expected_cipher_bytes[] = {0x13, 0x02, 0x13, 0x03}; + const unsigned char *p = nullptr; + + // Expected size is 2 bytes more than |expected_cipher_bytes| to account for + // grease value + ASSERT_EQ(SSL_client_hello_get0_ciphers(server.get(), &p), + sizeof(expected_cipher_bytes) + 2); + + // Grab the first 2 bytes and check grease value + uint16_t grease_val = CRYPTO_load_u16_be(p); + ASSERT_FALSE(SSL_get_cipher_by_value(grease_val)); + + // Sanity check for first cipher ID after grease value + uint16_t cipher_val = CRYPTO_load_u16_be(p+2); + ASSERT_TRUE(SSL_get_cipher_by_value((cipher_val))); + + // Check order and validity of the rest of the client cipher suites, + // excluding the grease value (2nd byte onwards) + ASSERT_EQ(Bytes(expected_cipher_bytes, sizeof(expected_cipher_bytes)), + Bytes(p+2, sizeof(expected_cipher_bytes))); + + // Parsed ciphersuite list should only have 2 valid ciphersuites as configured + // (grease value should not be included). Even though the handshake fails, + // client cipher data should be available through the server SSL object. + ASSERT_TRUE(sk_SSL_CIPHER_num(server.get()->client_cipher_suites.get()) == 2); +} + +TEST(SSLTest, GetClientCiphers1_3) { + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr server_ctx = CreateContextWithTestCertificate(TLS_method()); + + // configure client to add fake ciphersuite + SSL_CTX_set_grease_enabled(client_ctx.get(), 1); ASSERT_TRUE(SSL_CTX_set_ciphersuites(client_ctx.get(), "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256")); + ASSERT_TRUE(client_ctx); ASSERT_TRUE(server_ctx); - // Configure only TLS 1.3. ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION)); ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION)); @@ -2436,22 +2499,87 @@ TEST(SSLTest, GetClientCiphers) { ASSERT_TRUE(CompleteHandshakes(client.get(), server.get())); - // Client calling, should return 0 ASSERT_EQ(SSL_client_hello_get0_ciphers(client.get(), nullptr), (size_t) 0); const unsigned char expected_cipher_bytes[] = {0x13, 0x01, 0x13, 0x02, 0x13, 0x03}; + const unsigned char *p = nullptr; + + // Expected size is 2 bytes more than |expected_cipher_bytes| to account for + // grease value + ASSERT_EQ(SSL_client_hello_get0_ciphers(server.get(), &p), + sizeof(expected_cipher_bytes) + 2); + + // Grab the first 2 bytes and check grease value + uint16_t grease_val = CRYPTO_load_u16_be(p); + ASSERT_FALSE(SSL_get_cipher_by_value(grease_val)); + + // Sanity check for first cipher ID after grease value + uint16_t cipher_val = CRYPTO_load_u16_be(p+2); + ASSERT_TRUE(SSL_get_cipher_by_value((cipher_val))); - const unsigned char *p; - // Get client ciphers and ensure written to out in appropriate format - ASSERT_EQ(SSL_client_hello_get0_ciphers(server.get(), &p), (size_t) 3); + // Check order and validity of the rest of the client cipher suites, + // excluding the grease value (2nd byte onwards) ASSERT_EQ(Bytes(expected_cipher_bytes, sizeof(expected_cipher_bytes)), - Bytes(p, sizeof(expected_cipher_bytes))); + Bytes(p+2, sizeof(expected_cipher_bytes))); + + // Parsed ciphersuite list should only have 3 valid ciphersuites as configured + // (grease value should not be included) + ASSERT_TRUE(sk_SSL_CIPHER_num(server.get()->client_cipher_suites.get()) == 3); +} + +TEST(SSLTest, GetClientCiphers1_2) { + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + + // configure client to add fake ciphersuite + SSL_CTX_set_grease_enabled(client_ctx.get(), 1); + + ASSERT_TRUE(SSL_CTX_set_cipher_list(client_ctx.get(), + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384:TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA")); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_2_VERSION)); + ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_2_VERSION)); + + bssl::UniquePtr client, server; + ASSERT_TRUE(CreateClientAndServer(&client, &server, + client_ctx.get(), server_ctx.get())); - // When calling again, should reuse value from ssl_st - ASSERT_FALSE(server.get()->client_cipher_suites_arr.empty()); - ASSERT_EQ(SSL_client_hello_get0_ciphers(server.get(), &p), (size_t) 3); + const unsigned char *tmp = nullptr; + // Handshake not completed, getting ciphers should fail + ASSERT_FALSE(SSL_client_hello_get0_ciphers(client.get(), &tmp)); + ASSERT_FALSE(SSL_client_hello_get0_ciphers(server.get(), &tmp)); + ASSERT_FALSE(tmp); + + ASSERT_TRUE(CompleteHandshakes(client.get(), server.get())); + + ASSERT_EQ(SSL_client_hello_get0_ciphers(client.get(), nullptr), (size_t) 0); + + const unsigned char expected_cipher_bytes[] = {0xC0, 0x2C, 0xC0, 0x13}; + const unsigned char *p = nullptr; + + // Expected size is 2 bytes more than |expected_cipher_bytes| to account for + // grease value + ASSERT_EQ(SSL_client_hello_get0_ciphers(server.get(), &p), + sizeof(expected_cipher_bytes) + 2); + + // Grab the first 2 bytes and check grease value + uint16_t grease_val = CRYPTO_load_u16_be(p); + ASSERT_FALSE(SSL_get_cipher_by_value(grease_val)); + + // Sanity check for first cipher ID after grease value + uint16_t cipher_val = CRYPTO_load_u16_be(p+2); + ASSERT_TRUE(SSL_get_cipher_by_value((cipher_val))); + + // Check order and validity of the rest of the client cipher suites, + // excluding the grease value (2nd byte onwards) ASSERT_EQ(Bytes(expected_cipher_bytes, sizeof(expected_cipher_bytes)), - Bytes(p, sizeof(expected_cipher_bytes))); + Bytes(p+2, sizeof(expected_cipher_bytes))); + + // Parsed ciphersuite list should only have 2 valid ciphersuites as configured + // (grease value should not be included) + ASSERT_TRUE(sk_SSL_CIPHER_num(server.get()->client_cipher_suites.get()) == 2); } static bssl::UniquePtr g_last_session; diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 4d8e1e14ac..4ed3b91424 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -232,7 +232,7 @@ static enum ssl_hs_wait_t do_select_parameters(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - if (!ssl_parse_client_cipher_list(&client_hello, &ssl->client_cipher_suites)) { + if (!ssl_parse_client_cipher_list(ssl, &client_hello, &ssl->client_cipher_suites)) { OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); return ssl_hs_error;