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;