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

@@ -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

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<STACK_OF(SSL_CIPHER)> *ciphers_out) {
+    SSL *ssl, const SSL_CLIENT_HELLO *client_hello, UniquePtr<STACK_OF(SSL_CIPHER)> *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<char *>(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;

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<STACK_OF(SSL_CIPHER)> *ciphers_out);
 
 
@@ -4064,14 +4065,19 @@ struct ssl_st {
   bssl::UniquePtr<SSL_SESSION> 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<STACK_OF(SSL_CIPHER)> 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<uint16_t> 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<char> 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;
 

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<unsigned char*>(ssl->client_cipher_suites_arr.data());
+    *out = reinterpret_cast<const unsigned char*>(ciphers);
   }
-
-  // Return the size
-  return num_ciphers;
+  return ssl->all_client_cipher_suites_len;
 }

ssl/ssl_test.cc

@@ -2411,16 +2411,79 @@ TEST(SSLTest, FindingCipher) {
   ASSERT_FALSE(cipher3);
 }
 
-TEST(SSLTest, GetClientCiphers) {
+TEST(SSLTest, GetClientCiphersAfterHandshakeFailure1_3) {
   bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
-  bssl::UniquePtr<SSL_CTX> server_ctx =
-          CreateContextWithTestCertificate(TLS_method());
+  bssl::UniquePtr<SSL_CTX> 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<SSL> 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<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+  bssl::UniquePtr<SSL_CTX> 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<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+  bssl::UniquePtr<SSL_CTX> 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<SSL> 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<SSL_SESSION> g_last_session;

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;