Preserve the TLS 1.3 cipher policy when a cipher setter fails

CMakeLists.txt

@@ -1458,6 +1458,7 @@ if(BUILD_TESTING)
   set(CRYPTO_TEST_EXEC crypto_test${TEST_EXECUTABLE_EXT})
   set(RANDOM_TEST_EXEC urandom_test${TEST_EXECUTABLE_EXT})
   set(SSL_TEST_EXEC ssl_test${TEST_EXECUTABLE_EXT})
+  set(SSL_CIPHERS_MEM_TEST_EXEC ssl_ciphers_mem_test${TEST_EXECUTABLE_EXT})
   set(MEM_TEST_EXEC mem_test${TEST_EXECUTABLE_EXT})
   set(MEM_SET_TEST_EXEC mem_set_test${TEST_EXECUTABLE_EXT})
   set(INTEGRATION_TEST_EXEC integration_test${TEST_EXECUTABLE_EXT})
@@ -1732,6 +1733,7 @@ if(BUILD_TESTING)
           COMMAND crypto/crypto_test
           COMMAND crypto/urandom_test
           COMMAND ssl/ssl_test
+          COMMAND ssl/ssl_ciphers_mem_test
           WORKING_DIRECTORY ${AWSLC_BINARY_DIR}
           DEPENDS all_tests
           ${MAYBE_USES_TERMINAL})

include/openssl/ssl.h

@@ -1726,19 +1726,28 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out,
 // anything meaningless. It updates |ctx->cipher_list| with any values in
 // |ctx->tls13_cipher_list|.
 //
-// It returns one on success and zero on failure.
+// It returns one on success and zero on failure. If |str| parses successfully
+// but selects no TLS 1.2 and below suites (including an empty |str|), it returns
+// zero and clears those suites, preserving the TLS 1.3 suites. Other failures,
+// including parsing errors, leave the configuration unchanged.
 OPENSSL_EXPORT int SSL_CTX_set_strict_cipher_list(SSL_CTX *ctx,
                                                   const char *str);
 
 // SSL_CTX_set_cipher_list configures the cipher list for |ctx|, evaluating
 // |str| as a cipher string. It updates |ctx->cipher_list| with any values in
 // |ctx->tls13_cipher_list|. It returns one on success and zero on failure.
 //
-// Prefer to use |SSL_CTX_set_strict_cipher_list|. This function tolerates
-// garbage inputs, unless an empty cipher list results. However, an empty
-// string which also results in an empty cipher list, is allowed. This
-// behavior is strongly advised against and only meant for OpenSSL
-// compatibility.
+// Prefer to use |SSL_CTX_set_strict_cipher_list|. This function ignores
+// unrecognized cipher names for OpenSSL compatibility. This leniency is
+// strongly discouraged.
+//
+// If a non-empty |str| parses successfully but selects no TLS 1.2 and below
+// suites, this function returns zero and clears those suites, preserving the
+// TLS 1.3 suites. Other failures leave the configuration unchanged.
+//
+// An empty |str| clears the TLS 1.2 and below suites and returns one,
+// preserving the TLS 1.3 suites. OpenSSL instead returns zero with
+// |SSL_R_NO_CIPHER_MATCH| while still installing the TLS 1.3-only list.
 //
 // Note: this API only sets the TLSv1.2 and below ciphers.
 // Use |SSL_CTX_set_ciphersuites| to configure TLS 1.3 specific ciphers.
@@ -1751,20 +1760,39 @@ OPENSSL_EXPORT int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str);
 // TLS 1.3 cipher suites by first checking |ssl->config->tls13_cipher_list| and
 // otherwise falling back to |ssl->ctx->tls13_cipher_list|.
 //
-// It returns one on success and zero on failure.
+// It returns one on success and zero on failure. If |str| parses successfully
+// but selects no TLS 1.2 and below suites (including an empty |str|), it returns
+// zero and clears those suites, preserving the TLS 1.3 suites. Other failures,
+// including parsing errors, leave the configuration unchanged.
 OPENSSL_EXPORT int SSL_set_strict_cipher_list(SSL *ssl, const char *str);
 
 // SSL_CTX_set_ciphersuites configures the available TLSv1.3 ciphersuites on
 // |ctx|, evaluating |str| as a cipher string. It updates |ctx->cipher_list|
 // with any values in |ctx->tls13_cipher_list|. It returns one on success and
 // zero on failure.
+//
+// An empty |str| succeeds and configures no TLS 1.3 suites. Any failure,
+// including a non-empty |str| that matches none, leaves the configuration
+// unchanged.
+//
+// An empty list does not disable the TLS 1.3 protocol or enable automatic
+// fallback to TLS 1.2. To disable TLS 1.3, use |SSL_CTX_set_max_proto_version|
+// with |TLS1_2_VERSION| instead.
 OPENSSL_EXPORT int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str);
 
 // SSL_set_ciphersuites configures the available TLSv1.3 ciphersuites on
 // |ssl|, evaluating |str| as a cipher string. It updates
 // |ssl->config->cipher_list| with any values in
 // |ssl->config->tls13_cipher_list|. It returns one on success and zero on
 // failure.
+//
+// An empty |str| succeeds and configures no TLS 1.3 suites. Any failure,
+// including a non-empty |str| that matches none, leaves the configuration
+// unchanged.
+//
+// An empty list does not disable the TLS 1.3 protocol or enable automatic
+// fallback to TLS 1.2. To disable TLS 1.3, use |SSL_set_max_proto_version| with
+// |TLS1_2_VERSION| instead.
 OPENSSL_EXPORT int SSL_set_ciphersuites(SSL *ssl, const char *str);
 
 // SSL_set_cipher_list configures the cipher list for |ssl|, evaluating |str| as
@@ -1777,10 +1805,17 @@ OPENSSL_EXPORT int SSL_set_ciphersuites(SSL *ssl, const char *str);
 //
 // It returns one on success and zero on failure.
 //
-// Prefer to use |SSL_set_strict_cipher_list|. This function tolerates garbage
-// inputs, unless an empty cipher list results. However, an empty string which
-// also results in an empty cipher list, is allowed. This behavior is strongly
-// advised against and only meant for OpenSSL compatibility.
+// Prefer to use |SSL_set_strict_cipher_list|. This function ignores
+// unrecognized cipher names for OpenSSL compatibility. This leniency is
+// strongly discouraged.
+//
+// If a non-empty |str| parses successfully but selects no TLS 1.2 and below
+// suites, this function returns zero and clears those suites, preserving the
+// TLS 1.3 suites. Other failures leave the configuration unchanged.
+//
+// An empty |str| clears the TLS 1.2 and below suites and returns one,
+// preserving the TLS 1.3 suites. OpenSSL instead returns zero with
+// |SSL_R_NO_CIPHER_MATCH| while still installing the TLS 1.3-only list.
 OPENSSL_EXPORT int SSL_set_cipher_list(SSL *ssl, const char *str);
 
 // SSL_CTX_get_ciphers returns the cipher list for |ctx|, in order of

ssl/CMakeLists.txt

@@ -129,10 +129,18 @@ if(BUILD_TESTING)
   target_add_awslc_include_paths(TARGET ${SSL_TEST_EXEC} SCOPE PRIVATE)
   set_test_location(${SSL_TEST_EXEC})
 
+  # CRYPTO_set_mem_functions installs process-global hooks only once. Keep
+  # allocation failure tests isolated from the other SSL tests.
+  add_executable(${SSL_CIPHERS_MEM_TEST_EXEC} ssl_ciphers_mem_test.cc)
+  target_link_libraries(${SSL_CIPHERS_MEM_TEST_EXEC} boringssl_gtest_main ssl)
+  target_add_awslc_include_paths(TARGET ${SSL_CIPHERS_MEM_TEST_EXEC} SCOPE PRIVATE)
+  set_test_location(${SSL_CIPHERS_MEM_TEST_EXEC})
+
   if(WIN32)
     target_link_libraries(${SSL_TEST_EXEC} ws2_32)
+    target_link_libraries(${SSL_CIPHERS_MEM_TEST_EXEC} ws2_32)
   endif()
-  add_dependencies(all_tests ${SSL_TEST_EXEC})
+  add_dependencies(all_tests ${SSL_TEST_EXEC} ${SSL_CIPHERS_MEM_TEST_EXEC})
   add_subdirectory(test)
 endif()
 

ssl/crypto_policy.cc

@@ -531,37 +531,31 @@ size_t MergeDefaultPQSigalgs(uint16_t *ids, size_t n, size_t cap,
 
 // ApplyCipherRule applies the cipher rule |rule| to |ctx|, as
 // |SSL_CTX_set_cipher_list| does when |config_tls13| is false and
-// |SSL_CTX_set_ciphersuites| when it is true, and returns false having left |ctx|
-// as it was if any step fails.
+// |SSL_CTX_set_ciphersuites| when it is true, and returns false having left
+// |ctx| as it was if any step fails.
 //
-// Neither public setter is a no-op on failure. |ssl_create_cipher_list| installs
-// its result, empty or not, before reporting that the rule matched nothing, and
-// the |update_cipher_list| that merges the TLS 1.2 and TLS 1.3 lists back
-// together allocates, so it can fail after the first list is already in place.
-// Either way the context is left holding part of a policy it could not apply,
-// which for the first is no ciphers at all. Building both lists aside and moving
-// them in once every step has succeeded is what keeps a failure to the defaults.
+// |SSL_CTX_set_ciphersuites| already leaves |ctx| unchanged on failure. Unlike
+// |SSL_CTX_set_cipher_list|, which empties the TLS 1.2 and below suites when a
+// rule matches nothing, a policy that cannot be satisfied must keep the
+// defaults, so the legacy list is built aside and installed only on success.
 bool ApplyCipherRule(SSL_CTX *ctx, const char *rule, bool config_tls13) {
+  if (config_tls13) {
+    return SSL_CTX_set_ciphersuites(ctx, rule) != 0;
+  }
+
   const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
                                                : EVP_has_aes_hardware();
-  UniquePtr<SSLCipherPreferenceList> configured;
-  if (!ssl_create_cipher_list(&configured, has_aes_hw, rule,
-                              false /* not strict */, config_tls13)) {
+  UniquePtr<SSLCipherPreferenceList> tls12_list;
+  if (!ssl_create_cipher_list(&tls12_list, has_aes_hw, rule,
+                              false /* not strict */,
+                              false /* don't configure TLSv1.3 ciphers */)) {
     return false;
   }
 
-  UniquePtr<SSLCipherPreferenceList> &tls12_list =
-      config_tls13 ? ctx->cipher_list : configured;
-  UniquePtr<SSLCipherPreferenceList> &tls13_list =
-      config_tls13 ? configured : ctx->tls13_cipher_list;
   UniquePtr<SSLCipherPreferenceList> merged;
-  if (!update_cipher_list(merged, tls12_list, tls13_list)) {
+  if (!update_cipher_list(merged, tls12_list, ctx->tls13_cipher_list)) {
     return false;
   }
-
-  if (config_tls13) {
-    ctx->tls13_cipher_list = std::move(configured);
-  }
   ctx->cipher_list = std::move(merged);
   return true;
 }

ssl/internal.h

@@ -608,9 +608,10 @@ 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|.
-// 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.
+// An empty result is still stored in |*out_cipher_list|. It returns false for
+// an empty result unless |strict| is false and |rule_str| is empty. Other
+// failures leave |*out_cipher_list| unchanged. |has_aes_hw| indicates if the
+// list should be ordered based on having support for AES in hardware or not.
 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_cipher.cc

@@ -1308,9 +1308,9 @@ bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
 
   *out_cipher_list = std::move(pref_list);
 
-  // Configuring an empty cipher list is an error when |strict| is true, but
-  // still updates the output. When otherwise, OpenSSL explicitly allows an
-  // empty list.
+  // An empty result is an error, but still updates the output. The exception
+  // is an explicitly empty rule in non-strict mode: the TLS 1.3 setters accept
+  // it as in OpenSSL, and AWS-LC's legacy setters retain that behavior too.
   if ((strict || (*rule_str != '\0')) &&
       sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);