Add new cipherlist-setting APIs that reject nonsense.

The new APIs are SSL_CTX_set_strict_cipher_list() and
SSL_set_strict_cipher_list().  They have two motivations:

First, typos in cipher lists can go undetected for a long time, and
can have surprising consequences when silently ignored.

Second, there is a tendency to use superstition in the construction of
cipher lists, for example by "turning off" things that do not actually
exist.  This leads to the corrosive belief that DEFAULT and ALL ought
not to be trusted.  This belief is false.

Change-Id: I42909b69186e0b4cf45457e5c0bc968f6bbf231a
Reviewed-on: https://boringssl-review.googlesource.com/13925
Commit-Queue: Matt Braithwaite <mab@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>

fuzz/client.cc

@@ -279,7 +279,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) {
   SSL_set_alpn_protos(client, kALPNProtocols, sizeof(kALPNProtocols));
 
   // Enable ciphers that are off by default.
-  SSL_set_cipher_list(client, "ALL:NULL-SHA");
+  SSL_set_strict_cipher_list(client, "ALL:NULL-SHA");
 
   BIO_write(in, buf, len);
   if (SSL_do_handshake(client) == 1) {

fuzz/server.cc

@@ -274,7 +274,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) {
   SSL_set_tls_channel_id_enabled(server, 1);
 
   // Enable ciphers that are off by default.
-  SSL_set_cipher_list(server, "ALL:NULL-SHA");
+  SSL_set_strict_cipher_list(server, "ALL:NULL-SHA");
 
   DH *dh = DH_get_1024_160(nullptr);
   SSL_set_tmp_dh(server, dh);

fuzz/ssl_ctx_api.cc

@@ -344,11 +344,14 @@ static const std::function<void(SSL_CTX *, CBS *)> kAPIs[] = {
       if (!GetString(&ciphers, cbs)) {
         return;
       }
-      SSL_CTX_set_cipher_list(ctx, ciphers.c_str());
+      SSL_CTX_set_strict_cipher_list(ctx, ciphers.c_str());
     },
     [](SSL_CTX *ctx, CBS *cbs) {
-      // This function was left blank rather than removed to avoid invalidating
-      // the existing corpus. New entries may reuse it.
+      std::string ciphers;
+      if (!GetString(&ciphers, cbs)) {
+        return;
+      }
+      SSL_CTX_set_cipher_list(ctx, ciphers.c_str());
     },
     [](SSL_CTX *ctx, CBS *cbs) {
       // This function was left blank rather than removed to avoid invalidating

include/openssl/ssl.h

@@ -1313,7 +1313,9 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher,
  *   |TLSv1_2| matches ciphers new in TLS 1.2. This is confusing and should not
  *   be used.
  *
- * Unknown rules silently match nothing.
+ * Unknown rules are silently ignored by legacy APIs, and rejected by APIs with
+ * "strict" in the name, which should be preferred. Cipher lists can be long and
+ * it's easy to commit typos.
  *
  * The special |@STRENGTH| directive will sort all enabled ciphers by strength.
  *
@@ -1342,12 +1344,29 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher,
  * substituted when a cipher string starts with 'DEFAULT'. */
 #define SSL_DEFAULT_CIPHER_LIST "ALL"
 
+/* SSL_CTX_set_strict_cipher_list configures the cipher list for |ctx|,
+ * evaluating |str| as a cipher string and returning error if |str| contains
+ * anything meaningless. It returns one on success and zero on failure. */
+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 returns one on success and zero on failure. */
+ * |str| as a cipher string. 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. */
 OPENSSL_EXPORT int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str);
 
+/* SSL_set_strict_cipher_list configures the cipher list for |ssl|, evaluating
+ * |str| as a cipher string and returning error if |str| contains anything
+ * meaningless. It returns one on success and zero on failure. */
+OPENSSL_EXPORT int SSL_set_strict_cipher_list(SSL *ssl, const char *str);
+
 /* SSL_set_cipher_list configures the cipher list for |ssl|, evaluating |str| as
- * a cipher string. It returns one on success and zero on failure. */
+ * a cipher string. 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. */
 OPENSSL_EXPORT int SSL_set_cipher_list(SSL *ssl, const char *str);
 
 /* SSL_get_ciphers returns the cipher list for |ssl|, in order of preference. */

ssl/internal.h

@@ -227,12 +227,13 @@ const EVP_MD *ssl_get_handshake_digest(uint32_t algorithm_prf,
 /* ssl_create_cipher_list evaluates |rule_str| according to the ciphers in
  * |ssl_method|. It sets |*out_cipher_list| to a newly-allocated
  * |ssl_cipher_preference_list_st| containing the result. It returns
- * |(*out_cipher_list)->ciphers| on success and NULL on
- * failure. */
+ * |(*out_cipher_list)->ciphers| on success and NULL on failure. If |strict| is
+ * true, nonsense will be rejected. If false, nonsense will be silently
+ * ignored. */
 STACK_OF(SSL_CIPHER) *
 ssl_create_cipher_list(const SSL_PROTOCOL_METHOD *ssl_method,
                        struct ssl_cipher_preference_list_st **out_cipher_list,
-                       const char *rule_str);
+                       const char *rule_str, int strict);
 
 /* ssl_cipher_get_value returns the cipher suite id of |cipher|. */
 uint16_t ssl_cipher_get_value(const SSL_CIPHER *cipher);

ssl/ssl_cipher.c

@@ -1070,7 +1070,7 @@ static int ssl_cipher_strength_sort(CIPHER_ORDER **head_p,
 static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
                                       const char *rule_str,
                                       CIPHER_ORDER **head_p,
-                                      CIPHER_ORDER **tail_p) {
+                                      CIPHER_ORDER **tail_p, int strict) {
   uint32_t alg_mkey, alg_auth, alg_enc, alg_mac;
   uint16_t min_version;
   const char *l, *buf;
@@ -1206,6 +1206,10 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
         }
         if (j == kCipherAliasesLen) {
           skip_rule = 1;
+          if (strict) {
+            OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_COMMAND);
+            return 0;
+          }
         }
       }
 
@@ -1253,7 +1257,7 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
 STACK_OF(SSL_CIPHER) *
 ssl_create_cipher_list(const SSL_PROTOCOL_METHOD *ssl_method,
                        struct ssl_cipher_preference_list_st **out_cipher_list,
-                       const char *rule_str) {
+                       const char *rule_str, int strict) {
   STACK_OF(SSL_CIPHER) *cipherstack = NULL;
   CIPHER_ORDER *co_list = NULL, *head = NULL, *tail = NULL, *curr;
   uint8_t *in_group_flags = NULL;
@@ -1334,7 +1338,7 @@ ssl_create_cipher_list(const SSL_PROTOCOL_METHOD *ssl_method,
   const char *rule_p = rule_str;
   if (strncmp(rule_str, "DEFAULT", 7) == 0) {
     if (!ssl_cipher_process_rulestr(ssl_method, SSL_DEFAULT_CIPHER_LIST, &head,
-                                    &tail)) {
+                                    &tail, strict)) {
       goto err;
     }
     rule_p += 7;
@@ -1344,7 +1348,7 @@ ssl_create_cipher_list(const SSL_PROTOCOL_METHOD *ssl_method,
   }
 
   if (*rule_p != '\0' &&
-      !ssl_cipher_process_rulestr(ssl_method, rule_p, &head, &tail)) {
+      !ssl_cipher_process_rulestr(ssl_method, rule_p, &head, &tail, strict)) {
     goto err;
   }
 

ssl/ssl_lib.c

@@ -277,7 +277,7 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
   }
 
   ssl_create_cipher_list(ret->method, &ret->cipher_list,
-                         SSL_DEFAULT_CIPHER_LIST);
+                         SSL_DEFAULT_CIPHER_LIST, 1 /* strict */);
   if (ret->cipher_list == NULL ||
       sk_SSL_CIPHER_num(ret->cipher_list->ciphers) <= 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_LIBRARY_HAS_NO_CIPHERS);
@@ -1506,7 +1506,25 @@ const char *SSL_get_cipher_list(const SSL *ssl, int n) {
 
 int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str) {
   STACK_OF(SSL_CIPHER) *cipher_list =
-      ssl_create_cipher_list(ctx->method, &ctx->cipher_list, str);
+      ssl_create_cipher_list(ctx->method, &ctx->cipher_list, str,
+                             0 /* not strict */);
+  if (cipher_list == NULL) {
+    return 0;
+  }
+
+  /* |ssl_create_cipher_list| may succeed but return an empty cipher list. */
+  if (sk_SSL_CIPHER_num(cipher_list) == 0) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);
+    return 0;
+  }
+
+  return 1;
+}
+
+int SSL_CTX_set_strict_cipher_list(SSL_CTX *ctx, const char *str) {
+  STACK_OF(SSL_CIPHER) *cipher_list =
+      ssl_create_cipher_list(ctx->method, &ctx->cipher_list, str,
+                             1 /* strict */);
   if (cipher_list == NULL) {
     return 0;
   }
@@ -1522,7 +1540,25 @@ int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str) {
 
 int SSL_set_cipher_list(SSL *ssl, const char *str) {
   STACK_OF(SSL_CIPHER) *cipher_list =
-      ssl_create_cipher_list(ssl->ctx->method, &ssl->cipher_list, str);
+      ssl_create_cipher_list(ssl->ctx->method, &ssl->cipher_list, str,
+                             0 /* not strict */);
+  if (cipher_list == NULL) {
+    return 0;
+  }
+
+  /* |ssl_create_cipher_list| may succeed but return an empty cipher list. */
+  if (sk_SSL_CIPHER_num(cipher_list) == 0) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);
+    return 0;
+  }
+
+  return 1;
+}
+
+int SSL_set_strict_cipher_list(SSL *ssl, const char *str) {
+  STACK_OF(SSL_CIPHER) *cipher_list =
+      ssl_create_cipher_list(ssl->ctx->method, &ssl->cipher_list, str,
+                             1 /* strict */);
   if (cipher_list == NULL) {
     return 0;
   }

ssl/ssl_test.cc

@@ -59,6 +59,8 @@ struct CipherTest {
   const char *rule;
   // The list of expected ciphers, in order.
   std::vector<ExpectedCipher> expected;
+  // True if this cipher list should fail in strict mode.
+  bool strict_fail;
 };
 
 struct CurveTest {
@@ -81,6 +83,7 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
     // + reorders selected ciphers to the end, keeping their relative order.
     {
@@ -95,6 +98,7 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
     // ! banishes ciphers from future selections.
     {
@@ -107,6 +111,7 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, 0},
             {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
     // Multiple masks can be ANDed in a single rule.
     {
@@ -114,6 +119,7 @@ static const CipherTest kCipherTests[] = {
         {
             {TLS1_CK_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
     // - removes selected ciphers, but preserves their order for future
     // selections. Select AES_128_GCM, but order the key exchanges RSA, DHE_RSA,
@@ -126,20 +132,37 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_DHE_RSA_WITH_AES_128_GCM_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
-    // Unknown selectors are no-ops.
+    // Unknown selectors are no-ops, except in strict mode.
     {
         "ECDHE-ECDSA-CHACHA20-POLY1305:"
         "ECDHE-RSA-CHACHA20-POLY1305:"
         "ECDHE-ECDSA-AES128-GCM-SHA256:"
         "ECDHE-RSA-AES128-GCM-SHA256:"
-        "BOGUS1:-BOGUS2:+BOGUS3:!BOGUS4",
+        "BOGUS1",
         {
             {TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 0},
             {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        true,
+    },
+    // Unknown selectors are no-ops, except in strict mode.
+    {
+        "ECDHE-ECDSA-CHACHA20-POLY1305:"
+        "ECDHE-RSA-CHACHA20-POLY1305:"
+        "ECDHE-ECDSA-AES128-GCM-SHA256:"
+        "ECDHE-RSA-AES128-GCM-SHA256:"
+        "-BOGUS2:+BOGUS3:!BOGUS4",
+        {
+            {TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, 0},
+            {TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 0},
+            {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0},
+            {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
+        },
+        true,
     },
     // Square brackets specify equi-preference groups.
     {
@@ -152,13 +175,14 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        false,
     },
     // @STRENGTH performs a stable strength-sort of the selected ciphers and
     // only the selected ciphers.
     {
         // To simplify things, banish all but {ECDHE_RSA,RSA} x
         // {CHACHA20,AES_256_CBC,AES_128_CBC} x SHA1.
-        "!kEDH:!AESGCM:!3DES:!SHA256:!MD5:!SHA384:"
+        "!kEDH:!AESGCM:!3DES:!SHA256:!SHA384:"
         // Order some ciphers backwards by strength.
         "ALL:-CHACHA20:-AES256:-AES128:-ALL:"
         // Select ECDHE ones and sort them by strength. Ties should resolve
@@ -174,6 +198,7 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_RSA_WITH_AES_128_SHA, 0},
             {TLS1_CK_RSA_WITH_AES_256_SHA, 0},
         },
+        false,
     },
     // Exact ciphers may not be used in multi-part rules; they are treated
     // as unknown aliases.
@@ -186,6 +211,7 @@ static const CipherTest kCipherTests[] = {
             {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0},
             {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
         },
+        true,
     },
     // SSLv3 matches everything that existed before TLS 1.2.
     {
@@ -193,6 +219,7 @@ static const CipherTest kCipherTests[] = {
         {
             {TLS1_CK_RSA_WITH_AES_128_SHA256, 0},
         },
+        false,
     },
     // TLSv1.2 matches everything added in TLS 1.2.
     {
@@ -200,14 +227,17 @@ static const CipherTest kCipherTests[] = {
         {
             {TLS1_CK_RSA_WITH_AES_128_SHA, 0},
         },
+        false,
     },
-    // The two directives have no intersection.
+    // The two directives have no intersection.  But each component is valid, so
+    // even in strict mode it is accepted.
     {
         "AES128-SHA:AES128-SHA256:!TLSv1.2+SSLv3",
         {
             {TLS1_CK_RSA_WITH_AES_128_SHA, 0},
             {TLS1_CK_RSA_WITH_AES_128_SHA256, 0},
         },
+        false,
     },
 };
 
@@ -239,8 +269,6 @@ static const char *kBadRules[] = {
 static const char *kMustNotIncludeNull[] = {
   "ALL",
   "DEFAULT",
-  "ALL:!eNULL",
-  "ALL:!NULL",
   "HIGH",
   "FIPS",
   "SHA",
@@ -309,6 +337,12 @@ static bool TestCipherRule(const CipherTest &t) {
     return false;
   }
 
+  if (!SSL_CTX_set_strict_cipher_list(ctx.get(), t.rule) != t.strict_fail) {
+    fprintf(stderr, "Unexpected strict failure result testing cipher rule '%s':"
+            " expected %d\n", t.rule, t.strict_fail);
+    return false;
+  }
+
   // Compare the two lists.
   if (sk_SSL_CIPHER_num(ctx->cipher_list->ciphers) != t.expected.size()) {
     fprintf(stderr, "Error: cipher rule '%s' evaluated to:\n", t.rule);
@@ -335,7 +369,7 @@ static bool TestRuleDoesNotIncludeNull(const char *rule) {
   if (!ctx) {
     return false;
   }
-  if (!SSL_CTX_set_cipher_list(ctx.get(), rule)) {
+  if (!SSL_CTX_set_strict_cipher_list(ctx.get(), rule)) {
     fprintf(stderr, "Error: cipher rule '%s' failed\n", rule);
     return false;
   }
@@ -875,7 +909,7 @@ static size_t GetClientHelloLen(uint16_t max_version, uint16_t session_version,
   // Set a one-element cipher list so the baseline ClientHello is unpadded.
   bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
   if (!ssl || !SSL_set_session(ssl.get(), session.get()) ||
-      !SSL_set_cipher_list(ssl.get(), "ECDHE-RSA-AES128-GCM-SHA256") ||
+      !SSL_set_strict_cipher_list(ssl.get(), "ECDHE-RSA-AES128-GCM-SHA256") ||
       !SSL_set_max_proto_version(ssl.get(), max_version)) {
     return 0;
   }
@@ -1760,7 +1794,7 @@ static bool ClientHelloMatches(uint16_t version, const uint8_t *expected,
       !SSL_CTX_set_max_proto_version(ctx.get(), version) ||
       // Our default cipher list varies by CPU capabilities, so manually place
       // the ChaCha20 ciphers in front.
-      !SSL_CTX_set_cipher_list(ctx.get(), "CHACHA20:ALL")) {
+      !SSL_CTX_set_strict_cipher_list(ctx.get(), "CHACHA20:ALL")) {
     return false;
   }
 

ssl/test/bssl_shim.cc

@@ -927,7 +927,7 @@ static bssl::UniquePtr<SSL_CTX> SetupCtx(const TestConfig *config) {
     cipher_list = config->cipher;
     SSL_CTX_set_options(ssl_ctx.get(), SSL_OP_CIPHER_SERVER_PREFERENCE);
   }
-  if (!SSL_CTX_set_cipher_list(ssl_ctx.get(), cipher_list.c_str())) {
+  if (!SSL_CTX_set_strict_cipher_list(ssl_ctx.get(), cipher_list.c_str())) {
     return nullptr;
   }
 

tool/ciphers.cc

@@ -32,7 +32,7 @@ bool Ciphers(const std::vector<std::string> &args) {
   const std::string &ciphers_string = args.back();
 
   bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(SSLv23_client_method()));
-  if (!SSL_CTX_set_cipher_list(ctx.get(), ciphers_string.c_str())) {
+  if (!SSL_CTX_set_strict_cipher_list(ctx.get(), ciphers_string.c_str())) {
     fprintf(stderr, "Failed to parse cipher suite config.\n");
     ERR_print_errors_fp(stderr);
     return false;

tool/client.cc

@@ -277,7 +277,7 @@ bool Client(const std::vector<std::string> &args) {
   }
 
   if (args_map.count("-cipher") != 0 &&
-      !SSL_CTX_set_cipher_list(ctx.get(), args_map["-cipher"].c_str())) {
+      !SSL_CTX_set_strict_cipher_list(ctx.get(), args_map["-cipher"].c_str())) {
     fprintf(stderr, "Failed setting cipher list\n");
     return false;
   }

tool/server.cc

@@ -188,7 +188,7 @@ bool Server(const std::vector<std::string> &args) {
   }
 
   if (args_map.count("-cipher") != 0 &&
-      !SSL_CTX_set_cipher_list(ctx.get(), args_map["-cipher"].c_str())) {
+      !SSL_CTX_set_strict_cipher_list(ctx.get(), args_map["-cipher"].c_str())) {
     fprintf(stderr, "Failed setting cipher list\n");
     return false;
   }