Changed SSL_client_hello_get0_ciphers to align with OpenSSL behavior

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;