Implement SSL_get_client_ciphers (#1159)

# Notes

This commit implements `SSL_get_client_ciphers`, which returns a "stack" (really just a list) of `SSL_CIPHER` structs [presented by the client](https://github.com/openssl/openssl/blob/master/ssl/statem/statem_srvr.c#L2002) during the `|ssl|`'s most recent handshake. [Like OpenSSL](https://github.com/openssl/openssl/blob/e2972982c64f3f1ac10b3ebe1086d99ec67631bd/ssl/ssl_lib.c#L1433), we free this list when the connection is freed. OpenSSL [obtains this list](https://github.com/openssl/openssl/blob/7a5f58b2cf0d7b2fa0451603a88c3976c657dae9/ssl/ssl_lib.c#L3205-L3212) from from an `SSL_CONNECTION` object, something that we or BoringSSL appear to have removed.

So, we're left with the choice of where to store client ciphers. Options include internal/opaque structs `SSL_CTX`, `SSL_HANDSHAKE`, `SSL_SESSION`, `SSL_CONFIG`, and `SSL3_STATE`. Of these, `SSL_HANDSHAKE` or `SSL_SESSION` seemed most appropriate, but `SSL_HANDSHAKE` is [reset](https://github.com/aws/aws-lc/blob/main/ssl/ssl_lib.cc#L871) immediately after the handshake phase completes and `SSL_SESSION` incurs complications around caching. `SSL_CTX` appears to hold more configuration than mutable state, so we went with storing the client ciphers on `SSL3_STATE`. One important drawback to note here is that `SSL3_STATE` has "experimental" support for ASN.1 serialization. To avoid introducing irrevocable changes to that format, we do not include the client ciphers in that serialized object. When deserialized, client ciphers will be populated as "null". We can add support for this as an optional field in the future if required.

# Commit History

* Implement SSL_get_client_ciphers

* ExpectHandshakeSuccess includes 0RTT w/o cipher exchange

* Assign to ctx directly

* Set client cipher suites for tls13

* Factor out common logic and rely on ssl->ctx->peer_ciphers

* Adjust test case, TLS 1.3 handshake still fails

```
[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (30 ms total)
[  PASSED  ] 6 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] WithVersion/SSLVersionTest.GetPeerCertificate/TLS1_3, where GetParam() = 32-byte object <04-03 00-00 00-00 00-00 54-4C 53-31 5F-33 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00>
[  FAILED  ] WithVersion/SSLVersionTest.GetPeerCertificate/TLS1_3_SSL_TRANSFER, where GetParam() = 32-byte object <04-03 00-00 00-00 00-00 54-4C 53-31 5F-33 5F-53 53-4C 5F-54 52-41 4E-53 46-45 52-00 01-00 00-00>
```

* Revert "Factor out common logic and rely on ssl->ctx->peer_ciphers"

This reverts commit 64de7d6.

* Revert "Revert "Factor out common logic and rely on ssl->ctx->peer_ciphers""

This reverts commit 450c8d25510522d90aa5c38a7797b9670a1c67ab.

* Remove outdated CBS evenness check

* Move parsing up, store ciphers on handshake object

* Tests passing, skip transfer tests

* Only set peer_ciphers on ssl->s3

* Reset peer_ciphers on clear, address TODOs

* Rename peer_ciphers to client_ciphers

* Send handshake failure alert on parsing failure

* Rename client_ciphers to client_cipher_suites

* Move client cipher stack to SSL struct

* Preserve SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST behavior, push SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST on parse error

* Address @andhop docs + comments feedback

* Add SSL_get_client_ciphers for QUIC

include/openssl/ssl.h

@@ -1771,6 +1771,13 @@ 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.
+OPENSSL_EXPORT STACK_OF(SSL_CIPHER) *SSL_get_client_ciphers(const SSL *ssl);
+
 // SSL_session_reused returns one if |ssl| performed an abbreviated handshake
 // and zero otherwise.
 //
@@ -2595,7 +2602,9 @@ OPENSSL_EXPORT const char *SSL_get_group_name(uint16_t group_id);
 // finished.
 // WARNING: Currently only supports |SSL| as server.
 // WARNING: CRYPTO_EX_DATA |ssl->ex_data| is not encoded. Remember set |ex_data|
-// back after decode. WARNING: BIO |ssl->rbio| and |ssl->wbio| are not encoded.
+// back after decode.
+// WARNING: BIO |ssl->rbio| and |ssl->wbio| are not encoded.
+// WARNING: STACK_OF(SSL_CIPHER) |ssl->client_cipher_suites| is not encoded.
 //
 // Initial implementation of this API is made by Evgeny Potemkin.
 OPENSSL_EXPORT int SSL_to_bytes(const SSL *in, uint8_t **out_data,
@@ -2611,6 +2620,8 @@ OPENSSL_EXPORT int SSL_to_bytes(const SSL *in, uint8_t **out_data,
 //          Otherwise, the connections use the same key material.
 // WARNING: Remember set |ssl->rbio| and |ssl->wbio| before using |ssl|.
 // WARNING: Remember set callback functions and |ex_data| back if needed.
+// WARNING: STACK_OF(SSL_CIPHER) |ssl->client_cipher_suites| is not encoded and
+//          will be repopulated on next handshake.
 // WARNING: To ensure behavior unchange, |ctx| setting should be the same.
 //
 // Initial implementation of this API is made by Evgeny Potemkin.

ssl/handshake_server.cc

@@ -264,32 +264,43 @@ static bool negotiate_version(SSL_HANDSHAKE *hs, uint8_t *out_alert,
   return true;
 }
 
-static UniquePtr<STACK_OF(SSL_CIPHER)> ssl_parse_client_cipher_list(
-    const SSL_CLIENT_HELLO *client_hello) {
+bool ssl_parse_client_cipher_list(
+    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);
 
+  // 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);
+    return false;
+  }
+
   UniquePtr<STACK_OF(SSL_CIPHER)> sk(sk_SSL_CIPHER_new_null());
   if (!sk) {
-    return nullptr;
+    return false;
   }
 
   while (CBS_len(&cipher_suites) > 0) {
     uint16_t cipher_suite;
 
     if (!CBS_get_u16(&cipher_suites, &cipher_suite)) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
-      return nullptr;
+      return false;
     }
 
     const SSL_CIPHER *c = SSL_get_cipher_by_value(cipher_suite);
     if (c != NULL && !sk_SSL_CIPHER_push(sk.get(), c)) {
-      return nullptr;
+      OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
+      return false;
     }
   }
 
-  return sk;
+  *ciphers_out = std::move(sk);
+
+  return true;
 }
 
 // ssl_get_compatible_server_ciphers determines the key exchange and
@@ -338,9 +349,8 @@ static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs,
   *out_mask_a = mask_a;
 }
 
-static const SSL_CIPHER *choose_cipher(
-    SSL_HANDSHAKE *hs, const SSL_CLIENT_HELLO *client_hello,
-    const SSLCipherPreferenceList *server_pref) {
+static const SSL_CIPHER *choose_cipher(SSL_HANDSHAKE *hs,
+        const SSLCipherPreferenceList *server_pref) {
   SSL *const ssl = hs->ssl;
   const STACK_OF(SSL_CIPHER) *prio, *allow;
   // in_group_flags will either be NULL, or will point to an array of bytes
@@ -352,18 +362,12 @@ static const SSL_CIPHER *choose_cipher(
   // such value exists yet.
   int group_min = -1;
 
-  UniquePtr<STACK_OF(SSL_CIPHER)> client_pref =
-      ssl_parse_client_cipher_list(client_hello);
-  if (!client_pref) {
-    return nullptr;
-  }
-
   if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
     prio = server_pref->ciphers.get();
     in_group_flags = server_pref->in_group_flags;
-    allow = client_pref.get();
+    allow = ssl->client_cipher_suites.get();
   } else {
-    prio = client_pref.get();
+    prio = ssl->client_cipher_suites.get();
     in_group_flags = NULL;
     allow = server_pref->ciphers.get();
   }
@@ -810,12 +814,18 @@ 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)) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+    return ssl_hs_error;
+  }
+
   // Negotiate the cipher suite. This must be done after |cert_cb| so the
   // certificate is finalized.
   SSLCipherPreferenceList *prefs = hs->config->cipher_list
                                        ? hs->config->cipher_list.get()
                                        : ssl->ctx->cipher_list.get();
-  hs->new_cipher = choose_cipher(hs, &client_hello, prefs);
+  hs->new_cipher = choose_cipher(hs, prefs);
   if (hs->new_cipher == NULL) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);

ssl/internal.h

@@ -703,13 +703,13 @@ bool ssl_cipher_requires_server_key_exchange(const SSL_CIPHER *cipher);
 size_t ssl_cipher_get_record_split_len(const SSL_CIPHER *cipher);
 
 // ssl_choose_tls13_cipher returns an |SSL_CIPHER| corresponding with the best
-// available from |cipher_suites| compatible with |version|, |group_id| and
+// available from |client_cipher_suites| compatible with |version|, |group_id| and
 // configured |tls13_ciphers|. It returns NULL if there isn't a compatible
 // cipher. |has_aes_hw| indicates if the choice should be made as if support for
 // AES in hardware is available.
 const SSL_CIPHER *ssl_choose_tls13_cipher(
-    CBS cipher_suites, bool has_aes_hw, uint16_t version, uint16_t group_id,
-    const STACK_OF(SSL_CIPHER) *tls13_ciphers);
+    const STACK_OF(SSL_CIPHER) *client_cipher_suites, bool has_aes_hw, uint16_t version,
+    uint16_t group_id, const STACK_OF(SSL_CIPHER) *tls13_ciphers);
 
 
 // Transcript layer.
@@ -2425,6 +2425,12 @@ bool ssl_client_hello_get_extension(const SSL_CLIENT_HELLO *client_hello,
 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,
+                                  UniquePtr<STACK_OF(SSL_CIPHER)> *ciphers_out);
+
 
 // GREASE.
 
@@ -4000,6 +4006,11 @@ struct ssl_st {
   // is immutable.
   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.
+  bssl::UniquePtr<STACK_OF(SSL_CIPHER)> client_cipher_suites;
+
   void (*info_callback)(const SSL *ssl, int type, int value) = nullptr;
 
   bssl::UniquePtr<SSL_CTX> ctx;

ssl/s3_both.cc

@@ -686,36 +686,28 @@ class CipherScorer {
 };
 
 const SSL_CIPHER *ssl_choose_tls13_cipher(
-    CBS cipher_suites, bool has_aes_hw, uint16_t version, uint16_t group_id,
-    const STACK_OF(SSL_CIPHER) *tls13_ciphers) {
-  if (CBS_len(&cipher_suites) % 2 != 0) {
-    return nullptr;
-  }
+    const STACK_OF(SSL_CIPHER) *client_cipher_suites, bool has_aes_hw, uint16_t version,
+    uint16_t group_id, const STACK_OF(SSL_CIPHER) *tls13_ciphers) {
 
   const SSL_CIPHER *best = nullptr;
   CipherScorer scorer(has_aes_hw);
   CipherScorer::Score best_score = scorer.MinScore();
 
-  while (CBS_len(&cipher_suites) > 0) {
-    uint16_t cipher_suite;
-    if (!CBS_get_u16(&cipher_suites, &cipher_suite)) {
-      return nullptr;
-    }
-
-    SSL_CIPHER *candidate = nullptr;
+  for (size_t i = 0; i < sk_SSL_CIPHER_num(client_cipher_suites); i++) {
+    const SSL_CIPHER *client_cipher = sk_SSL_CIPHER_value(client_cipher_suites, i);
+    const SSL_CIPHER *candidate = nullptr;
     if (tls13_ciphers != nullptr) {
       // Limit to customized TLS 1.3 ciphers if configured.
-      uint32_t cipher_suite_id = 0x03000000L | cipher_suite;
-      for (size_t i = 0; i < sk_SSL_CIPHER_num(tls13_ciphers); i++) {
-        const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(tls13_ciphers, i);
-        if (cipher != nullptr && cipher->id == cipher_suite_id) {
+      for (size_t j = 0; j < sk_SSL_CIPHER_num(tls13_ciphers); j++) {
+        const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(tls13_ciphers, j);
+        if (cipher != nullptr && cipher->id == client_cipher->id) {
           candidate = (SSL_CIPHER *)cipher;
           break;
         }
       }
     } else {
       // Limit to TLS 1.3 ciphers we know about.
-      candidate = (SSL_CIPHER *)SSL_get_cipher_by_value(cipher_suite);
+      candidate = client_cipher;
     }
     if (candidate == nullptr ||
         SSL_CIPHER_get_min_version(candidate) > version ||

ssl/ssl_lib.cc

@@ -2545,6 +2545,13 @@ const SSL_CIPHER *SSL_get_current_cipher(const SSL *ssl) {
   return session == nullptr ? nullptr : session->cipher;
 }
 
+STACK_OF(SSL_CIPHER) *SSL_get_client_ciphers(const SSL *ssl) {
+  if (ssl == NULL || ssl->s3 == NULL) {
+    return NULL;
+  }
+  return ssl->client_cipher_suites.get();
+}
+
 int SSL_session_reused(const SSL *ssl) {
   return ssl->s3->session_reused || SSL_in_early_data(ssl);
 }
@@ -3062,6 +3069,8 @@ int SSL_clear(SSL *ssl) {
     return 0;  // SSL_clear may not be used after shedding config.
   }
 
+  ssl->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
   // depends on this behavior, so emulate it.

ssl/ssl_test.cc

@@ -4328,6 +4328,37 @@ TEST_P(SSLVersionTest, SSLClearFailsWithShedding) {
   ASSERT_FALSE(SSL_clear(server_.get()));
 }
 
+TEST_P(SSLVersionTest, SSLClientCiphers) {
+  // Client ciphers ARE NOT SERIALIZED, so skip tests that rely on transfer or
+  // serialization of |ssl| and accompanying objects under test.
+  if (GetParam().transfer_ssl) {
+      return;
+  }
+
+  EXPECT_FALSE(SSL_get_client_ciphers(client_.get()));
+  EXPECT_FALSE(SSL_get_client_ciphers(server_.get()));
+
+  shed_handshake_config_ = false;
+  ASSERT_TRUE(Connect());
+
+  // The client should still have no view of the server's preferences, but the
+  // server should have seen at least one cipher from the client.
+  EXPECT_FALSE(SSL_get_client_ciphers(client_.get()));
+  EXPECT_GT(sk_SSL_CIPHER_num(SSL_get_client_ciphers(server_.get())), (size_t) 0);
+
+  // With config shedding disabled, clearing |server| shouldn't error and
+  // should reset server's client ciphers
+  ASSERT_TRUE(SSL_clear(server_.get()));
+  EXPECT_FALSE(SSL_get_client_ciphers(server_.get()));
+
+  shed_handshake_config_ = true;
+  ASSERT_TRUE(Connect());
+
+  // These should be unaffected by config shedding
+  EXPECT_FALSE(SSL_get_client_ciphers(client_.get()));
+  EXPECT_GT(sk_SSL_CIPHER_num(SSL_get_client_ciphers(server_.get())), (size_t) 0);
+}
+
 static bool ChainsEqual(STACK_OF(X509) *chain,
                         const std::vector<X509 *> &expected) {
   if (sk_X509_num(chain) != expected.size()) {
@@ -7730,6 +7761,9 @@ TEST_F(QUICMethodTest, ZeroRTTAccept) {
   ASSERT_TRUE(CreateClientAndServer());
   SSL_set_session(client_.get(), session.get());
 
+  EXPECT_FALSE(SSL_get_client_ciphers(client_.get()));
+  EXPECT_FALSE(SSL_get_client_ciphers(server_.get()));
+
   // The client handshake should return immediately into the early data state.
   ASSERT_EQ(SSL_do_handshake(client_.get()), 1);
   EXPECT_TRUE(SSL_in_early_data(client_.get()));
@@ -7746,6 +7780,10 @@ TEST_F(QUICMethodTest, ZeroRTTAccept) {
   // 1-RTT read keys until client Finished.
   EXPECT_TRUE(transport_->server()->HasWriteSecret(ssl_encryption_application));
   EXPECT_FALSE(transport_->server()->HasReadSecret(ssl_encryption_application));
+  // The client should still have no view of the server's preferences, but the
+  // server should have seen at least one cipher from the client.
+  EXPECT_FALSE(SSL_get_client_ciphers(client_.get()));
+  EXPECT_GT(sk_SSL_CIPHER_num(SSL_get_client_ciphers(server_.get())), (size_t) 0);
 
   // Finish up the client and server handshakes.
   ASSERT_TRUE(CompleteHandshakesForQUIC());

ssl/tls13_server.cc

@@ -108,26 +108,19 @@ static int ssl_ext_supported_versions_add_serverhello(SSL_HANDSHAKE *hs,
   return 1;
 }
 
-static const SSL_CIPHER *choose_tls13_cipher(
-    const SSL *ssl, const SSL_CLIENT_HELLO *client_hello, uint16_t group_id) {
-  CBS cipher_suites;
-  CBS_init(&cipher_suites, client_hello->cipher_suites,
-           client_hello->cipher_suites_len);
-
-  const uint16_t version = ssl_protocol_version(ssl);
-
+static const SSL_CIPHER *choose_tls13_cipher(const SSL *ssl, uint16_t group_id) {
   STACK_OF(SSL_CIPHER) *tls13_ciphers = nullptr;
   if (ssl->ctx->tls13_cipher_list &&
       ssl->ctx->tls13_cipher_list.get()->ciphers &&
       sk_SSL_CIPHER_num(ssl->ctx->tls13_cipher_list.get()->ciphers.get()) > 0) {
     tls13_ciphers = ssl->ctx->tls13_cipher_list.get()->ciphers.get();
   }
 
-  return ssl_choose_tls13_cipher(cipher_suites,
+  return ssl_choose_tls13_cipher(ssl->client_cipher_suites.get(),
                                  ssl->config->aes_hw_override
                                      ? ssl->config->aes_hw_override_value
                                      : EVP_has_aes_hardware(),
-                                 version, group_id, tls13_ciphers);
+                                 ssl_protocol_version(ssl), group_id, tls13_ciphers);
 }
 
 static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) {
@@ -239,8 +232,14 @@ 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)) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+    return ssl_hs_error;
+  }
+
   // Negotiate the cipher suite.
-  hs->new_cipher = choose_tls13_cipher(ssl, &client_hello, group_id);
+  hs->new_cipher = choose_tls13_cipher(ssl, group_id);
   if (hs->new_cipher == NULL) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);