Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers

include/openssl/ssl.h

@@ -1690,10 +1690,9 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out,
 // Once an equal-preference group is used, future directives must be
 // opcode-less. Inside an equal-preference group, spaces are not allowed.
 //
-// TLS 1.3 ciphers do not participate in this mechanism and instead have a
-// built-in preference order. Functions to set cipher lists do not affect TLS
-// 1.3, and functions to query the cipher list do not include TLS 1.3
-// ciphers.
+// Note: TLS 1.3 ciphersuites are only configurable via
+// |SSL_CTX_set_ciphersuites| or |SSL_set_ciphersuites|. Other setter functions have
+// no impact on TLS 1.3 ciphersuites.
 
 // SSL_DEFAULT_CIPHER_LIST is the default cipher suite configuration. It is
 // substituted when a cipher string starts with 'DEFAULT'.

ssl/handshake_client.cc

@@ -224,6 +224,7 @@ static bool collect_cipher_protocol_ids(STACK_OF(SSL_CIPHER) *ciphers,
   CBB *cbb, uint32_t mask_k, uint32_t mask_a, uint16_t max_version,
   uint16_t min_version, bool *any_enabled) {
   *any_enabled = false;
+
   for (const SSL_CIPHER *cipher : ciphers) {
     // Skip disabled ciphers
     if ((cipher->algorithm_mkey & mask_k) ||
@@ -259,49 +260,31 @@ static bool ssl_write_client_cipher_list(const SSL_HANDSHAKE *hs, CBB *out,
     return false;
   }
 
-  // Add TLS 1.3 ciphers.
-  if (hs->max_version >= TLS1_3_VERSION && ssl->ctx->tls13_cipher_list) {
-    // Use the configured TLSv1.3 ciphers list.
-    STACK_OF(SSL_CIPHER) *ciphers = ssl->ctx->tls13_cipher_list->ciphers.get();
+  // Add all ciphers unless TLS 1.3 only connection
+  if (hs->min_version < TLS1_3_VERSION && type != ssl_client_hello_inner) {
     bool any_enabled = false;
-    if (!collect_cipher_protocol_ids(ciphers, &child, mask_k,
+    if (!collect_cipher_protocol_ids(SSL_get_ciphers(ssl), &child, mask_k,
       mask_a, hs->max_version, hs->min_version, &any_enabled)) {
       return false;
     }
+
     // If all ciphers were disabled, return the error to the caller.
     if (!any_enabled) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHERS_AVAILABLE);
       return false;
     }
-  } else if (hs->max_version >= TLS1_3_VERSION) {
-    // Use the built in TLSv1.3 ciphers. Order ChaCha20-Poly1305 relative to
-    // AES-GCM based on hardware support.
-    const bool has_aes_hw = ssl->config->aes_hw_override
-                                ? ssl->config->aes_hw_override_value
-                                : EVP_has_aes_hardware();
-
-    if (!has_aes_hw &&
-        !CBB_add_u16(&child, TLS1_CK_CHACHA20_POLY1305_SHA256 & 0xffff)) {
-      return false;
-    }
-    if (!CBB_add_u16(&child, TLS1_3_CK_AES_128_GCM_SHA256 & 0xffff) ||
-        !CBB_add_u16(&child, TLS1_3_CK_AES_256_GCM_SHA384 & 0xffff)) {
-      return false;
-    }
-    if (has_aes_hw &&
-        !CBB_add_u16(&child, TLS1_3_CK_CHACHA20_POLY1305_SHA256 & 0xffff)) {
-      return false;
-    }
-  }
-
-  if (hs->min_version < TLS1_3_VERSION && type != ssl_client_hello_inner) {
+  } else if (hs->max_version >= TLS1_3_VERSION && ssl->ctx->tls13_cipher_list) {
+    // Only TLS 1.3 ciphers
+    STACK_OF(SSL_CIPHER) *ciphers = ssl->ctx->tls13_cipher_list->ciphers.get();
     bool any_enabled = false;
-    if (!collect_cipher_protocol_ids(SSL_get_ciphers(ssl), &child, mask_k,
+
+    if (!collect_cipher_protocol_ids(ciphers, &child, mask_k,
       mask_a, hs->max_version, hs->min_version, &any_enabled)) {
       return false;
     }
+
     // If all ciphers were disabled, return the error to the caller.
-    if (!any_enabled && hs->max_version < TLS1_3_VERSION) {
+    if (!any_enabled) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHERS_AVAILABLE);
       return false;
     }

ssl/internal.h

@@ -715,6 +715,9 @@ const EVP_MD *ssl_get_handshake_digest(uint16_t version,
 // rejected. If false, nonsense will be silently ignored. If |config_tls13| is
 // true, only TLS 1.3 ciphers are considered in |ssl_cipher_collect_ciphers|. If
 // false, TLS 1.2 and below ciphers participate in |ssl_cipher_collect_ciphers|.
+// In every invocation, |ctx->cipher_list| is updated with any user-configured
+// or default TLS 1.3 cipher suites in |ctx->tls13_cipher_list|.
+//
 // An empty result is considered an error regardless of |strict| or
 // |config_tls13|. |has_aes_hw| indicates if the list should be ordered based on
 // having support for AES in hardware or not.
@@ -2371,6 +2374,8 @@ bool ssl_write_client_hello_without_extensions(const SSL_HANDSHAKE *hs,
                                                ssl_client_hello_type_t type,
                                                bool empty_session_id);
 
+int update_cipher_list(UniquePtr<SSLCipherPreferenceList> &dst, UniquePtr<SSLCipherPreferenceList> &ciphers, UniquePtr<SSLCipherPreferenceList> &tls13_ciphers);
+
 // ssl_add_client_hello constructs a ClientHello and adds it to the outgoing
 // flight. It returns true on success and false on error.
 bool ssl_add_client_hello(SSL_HANDSHAKE *hs);
@@ -3244,9 +3249,12 @@ struct SSL_CONFIG {
 
   X509_VERIFY_PARAM *param = nullptr;
 
-  // crypto
+  // All ciphersuites
   UniquePtr<SSLCipherPreferenceList> cipher_list;
 
+  // TLS 1.3 specific ciphersuites
+  UniquePtr<SSLCipherPreferenceList> tls13_cipher_list;
+
   // This is used to hold the local certificate used (i.e. the server
   // certificate for a server or the client certificate for a client).
   UniquePtr<CERT> cert;
@@ -3721,6 +3729,18 @@ struct ssl_method_st {
   const bssl::SSL_X509_METHOD *x509_method;
 };
 
+// TLS13_DEFAULT_CIPHER_LIST_AES_HW is the default TLS 1.3 cipher suite
+// configuration when AES hardware acceleration is enabled.
+#define TLS13_DEFAULT_CIPHER_LIST_AES_HW "TLS_AES_128_GCM_SHA256:" \
+                                         "TLS_AES_256_GCM_SHA384:" \
+                                         "TLS_CHACHA20_POLY1305_SHA256"
+
+// TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW is the default TLS 1.3 cipher suite
+// configuration when no AES hardware acceleration is enabled.
+#define TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW "TLS_CHACHA20_POLY1305_SHA256:" \
+                                            "TLS_AES_128_GCM_SHA256:" \
+                                            "TLS_AES_256_GCM_SHA384"
+
 #define MIN_SAFE_FRAGMENT_SIZE 512
 struct ssl_ctx_st : public bssl::RefCounted<ssl_ctx_st> {
   explicit ssl_ctx_st(const SSL_METHOD *ssl_method);
@@ -3754,12 +3774,12 @@ struct ssl_ctx_st : public bssl::RefCounted<ssl_ctx_st> {
   // quic_method is the method table corresponding to the QUIC hooks.
   const SSL_QUIC_METHOD *quic_method = nullptr;
 
-  // Currently, cipher_list holds the tls1.2 and below ciphersuites.
-  // TODO: move |tls13_cipher_list| to |cipher_list| during cipher
-  // configuration.
+  // cipher_list holds all available cipher suites for tls 1.3,
+  // and 1.2 and below
   bssl::UniquePtr<bssl::SSLCipherPreferenceList> cipher_list;
 
-  // tls13_cipher_list holds the tls1.3 and above ciphersuites.
+  // tls13_cipher_list holds the default or configured tls1.3 and above
+  // cipher suites.
   bssl::UniquePtr<bssl::SSLCipherPreferenceList> tls13_cipher_list;
 
   X509_STORE *cert_store = nullptr;

ssl/ssl_cipher.cc

@@ -1234,6 +1234,86 @@ static bool is_known_default_alias_keyword_filter_rule(const char *rule,
   return false;
 }
 
+// update_cipher_list updates |ctx->cipher_list| by:
+// 1. Removing any existing TLS 1.3 ciphersuites
+// 2. Adding configured ciphersuites from |ctx->tls13_cipher_list|
+// 3. Configuring a new |ctx->cipher_list->in_group_flags|
+// This function maintains the ordering of ciphersuites and places TLS 1.3
+// ciphersuites at the front of the list.
+// Returns one on success and zero on failure.
+int update_cipher_list(UniquePtr<SSLCipherPreferenceList> &dst, UniquePtr<SSLCipherPreferenceList> &ciphers, UniquePtr<SSLCipherPreferenceList> &tls13_ciphers) {
+  bssl::UniquePtr<STACK_OF(SSL_CIPHER)> tmp_cipher_list;
+  int num_removed_tls13_ciphers = 0, num_added_tls13_ciphers = 0;
+  Array<bool> updated_in_group_flags;
+
+  if (ciphers && ciphers->ciphers) {
+    tmp_cipher_list.reset(sk_SSL_CIPHER_dup(ciphers->ciphers.get()));
+  } else {
+    tmp_cipher_list.reset(sk_SSL_CIPHER_new_null());
+  }
+
+  if (tmp_cipher_list == NULL) {
+    return 0;
+  }
+
+  // Delete any existing TLSv1.3 ciphersuites. These will be first in the list
+  while (sk_SSL_CIPHER_num(tmp_cipher_list.get()) > 0 &&
+         SSL_CIPHER_get_min_version(sk_SSL_CIPHER_value(tmp_cipher_list.get(), 0))
+         == TLS1_3_VERSION) {
+    sk_SSL_CIPHER_delete(tmp_cipher_list.get(), 0);
+    num_removed_tls13_ciphers++;
+  }
+
+  int num_updated_tls12_ciphers = sk_SSL_CIPHER_num(tmp_cipher_list.get());
+
+  if (tls13_ciphers && tls13_ciphers->ciphers) {
+    STACK_OF(SSL_CIPHER) *tls13_cipher_stack = tls13_ciphers->ciphers.get();
+    num_added_tls13_ciphers = sk_SSL_CIPHER_num(tls13_cipher_stack);
+    for (int i = sk_SSL_CIPHER_num(tls13_cipher_stack) - 1; i >= 0; i--) {
+      const SSL_CIPHER *tls13_cipher = sk_SSL_CIPHER_value(tls13_cipher_stack, i);
+      if (!sk_SSL_CIPHER_unshift(tmp_cipher_list.get(), tls13_cipher)) {
+        return 0;
+      }
+    }
+  }
+
+
+  if (!updated_in_group_flags.Init(num_added_tls13_ciphers + num_updated_tls12_ciphers)) {
+    return 0;
+  }
+  std::fill(updated_in_group_flags.begin(), updated_in_group_flags.end(), false);
+
+  // Copy in_group_flags from |ctx->tls13_cipher_list|
+  if (tls13_ciphers && tls13_ciphers->in_group_flags) {
+    const auto& tls13_flags = tls13_ciphers->in_group_flags;
+    // Ensure value of last element in |in_group_flags| is 0. The last cipher
+    // in a list must be the end of any group in that list.
+    if (tls13_flags[num_added_tls13_ciphers - 1] != 0) {
+      tls13_flags[num_added_tls13_ciphers - 1] = false;
+    }
+    for (int i = 0; i < num_added_tls13_ciphers; i++) {
+      updated_in_group_flags[i] = tls13_flags[i];
+    }
+  }
+
+  // Copy remaining in_group_flags from |ctx->cipher_list|
+  if (ciphers && ciphers->in_group_flags) {
+    for (int i = 0; i < num_updated_tls12_ciphers; i++) {
+      updated_in_group_flags[i + num_added_tls13_ciphers] =
+        ciphers->in_group_flags[i + num_removed_tls13_ciphers];
+    }
+  }
+
+  Span<const bool> flags_span(updated_in_group_flags.data(), updated_in_group_flags.size());
+  UniquePtr<SSLCipherPreferenceList> new_list = MakeUnique<SSLCipherPreferenceList>();
+  if (!new_list || !new_list->Init(std::move(tmp_cipher_list), flags_span)) {
+    return 0;
+  }
+
+  dst = std::move(new_list);
+  return 1;
+}
+
 bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
                             const bool has_aes_hw, const char *rule_str,
                             bool strict, bool config_tls13) {

ssl/ssl_lib.cc

@@ -586,7 +586,16 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
     return nullptr;
   }
 
-  if (!SSL_CTX_set_strict_cipher_list(ret.get(), SSL_DEFAULT_CIPHER_LIST) ||
+  const bool has_aes_hw = EVP_has_aes_hardware();
+  const char *cipher_rule;
+  if (has_aes_hw) {
+    cipher_rule = TLS13_DEFAULT_CIPHER_LIST_AES_HW;
+  } else {
+    cipher_rule = TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW;
+  }
+
+  if (!SSL_CTX_set_ciphersuites(ret.get(), cipher_rule) ||
+      !SSL_CTX_set_strict_cipher_list(ret.get(), SSL_DEFAULT_CIPHER_LIST) ||
       // Lock the SSL_CTX to the specified version, for compatibility with
       // legacy uses of SSL_METHOD.
       !SSL_CTX_set_max_proto_version(ret.get(), method->version) ||
@@ -2178,17 +2187,25 @@ const char *SSL_get_cipher_list(const SSL *ssl, int n) {
 int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str) {
   const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
                                                : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
+  if (!ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
                                 false /* not strict */,
-                                false /* don't configure TLSv1.3 ciphers */);
+                                false /* don't configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  return update_cipher_list(ctx->cipher_list, ctx->cipher_list, ctx->tls13_cipher_list);
 }
 
 int SSL_CTX_set_strict_cipher_list(SSL_CTX *ctx, const char *str) {
   const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
                                                : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
+  if (!ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
                                 true /* strict */,
-                                false /* don't configure TLSv1.3 ciphers */);
+                                false /* don't configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  return update_cipher_list(ctx->cipher_list, ctx->cipher_list, ctx->tls13_cipher_list);
 }
 
 int SSL_set_cipher_list(SSL *ssl, const char *str) {
@@ -2198,17 +2215,29 @@ int SSL_set_cipher_list(SSL *ssl, const char *str) {
   const bool has_aes_hw = ssl->config->aes_hw_override
                               ? ssl->config->aes_hw_override_value
                               : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
+  if (!ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
                                 false /* not strict */,
-                                false /* don't configure TLSv1.3 ciphers */);
+                                false /* don't configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  UniquePtr<SSLCipherPreferenceList> &tls13_ciphers = ssl->config->tls13_cipher_list ? ssl->config->tls13_cipher_list :
+                                        ssl->ctx->tls13_cipher_list;
+
+  return update_cipher_list(ssl->config->cipher_list, ssl->config->cipher_list, tls13_ciphers);
 }
 
 int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str) {
   const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
                                                : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ctx->tls13_cipher_list, has_aes_hw, str,
+
+  if (!ssl_create_cipher_list(&ctx->tls13_cipher_list, has_aes_hw, str,
                                 false /* not strict */,
-                                true /* only configure TLSv1.3 ciphers */);
+                                true /* only configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  return update_cipher_list(ctx->cipher_list, ctx->cipher_list, ctx->tls13_cipher_list);
 }
 
 int SSL_set_ciphersuites(SSL *ssl, const char *str) {
@@ -2218,9 +2247,16 @@ int SSL_set_ciphersuites(SSL *ssl, const char *str) {
   const bool has_aes_hw = ssl->config->aes_hw_override
                               ? ssl->config->aes_hw_override_value
                               : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
-                                false /* not strict */,
-                                true /* configure TLSv1.3 ciphers */);
+  if (!ssl_create_cipher_list(&ssl->config->tls13_cipher_list,
+                                has_aes_hw, str, false /* not strict */,
+                                true /* configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  UniquePtr<SSLCipherPreferenceList> &ciphers = ssl->config->cipher_list ? ssl->config->cipher_list :
+                                          ssl->ctx->cipher_list;
+
+  return update_cipher_list(ssl->config->cipher_list, ciphers, ssl->config->tls13_cipher_list);
 }
 
 int SSL_set_strict_cipher_list(SSL *ssl, const char *str) {
@@ -2230,9 +2266,16 @@ int SSL_set_strict_cipher_list(SSL *ssl, const char *str) {
   const bool has_aes_hw = ssl->config->aes_hw_override
                               ? ssl->config->aes_hw_override_value
                               : EVP_has_aes_hardware();
-  return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
-                                true /* strict */,
-                                false /* don't configure TLSv1.3 ciphers */);
+  if (!ssl_create_cipher_list(&ssl->config->cipher_list,
+                                has_aes_hw, str, true /* strict */,
+                                false /* don't configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  UniquePtr<SSLCipherPreferenceList> &tls13_ciphers = ssl->config->tls13_cipher_list ? ssl->config->tls13_cipher_list :
+                                        ssl->ctx->tls13_cipher_list;
+
+  return update_cipher_list(ssl->config->cipher_list, ssl->config->cipher_list, tls13_ciphers);
 }
 
 const char *SSL_get_servername(const SSL *ssl, const int type) {