From 22df69103f807d92595c67ea421aa51b0b34ed13 Mon Sep 17 00:00:00 2001
From: Adam Langley <agl@google.com>
Date: Tue, 25 Jul 2017 12:27:37 -0700
Subject: [PATCH] Document the behaviour of non-standard separators in cipher
 strings.

OpenSSL allows spaces, commas and semi-colons to be used as separators
in cipher strings, in addition to the usual colons.

This change documents that spaces cannot be used in equal-preference
groups and forbids these alternative separators in strict mode.

Change-Id: I3879e25aed54539c281511627e6a282e9463bdc3
Reviewed-on: https://boringssl-review.googlesource.com/18424
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
---
 include/openssl/ssl.h |  7 ++++---
 ssl/ssl_cipher.cc     | 12 ++++++++----
 ssl/ssl_test.cc       | 15 +++++++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 993046c47..0196dd961 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1350,8 +1350,9 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher,
  *   be used.
  *
  * 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.
+ * "strict" in the name, which should be preferred. Cipher lists can be long
+ * and it's easy to commit typos. Strict functions will also reject the use of
+ * spaces, semi-colons and commas as alternative separators.
  *
  * The special |@STRENGTH| directive will sort all enabled ciphers by strength.
  *
@@ -1369,7 +1370,7 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher,
  *   [ECDHE-ECDSA-CHACHA20-POLY1305|ECDHE-ECDSA-AES128-GCM-SHA256]
  *
  * Once an equal-preference group is used, future directives must be
- * opcode-less.
+ * 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
diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc
index dbb4c7521..f1a215fc6 100644
--- a/ssl/ssl_cipher.cc
+++ b/ssl/ssl_cipher.cc
@@ -756,8 +756,12 @@ const EVP_MD *ssl_get_handshake_digest(uint32_t algorithm_prf,
   }
 }
 
-#define ITEM_SEP(a) \
-  (((a) == ':') || ((a) == ' ') || ((a) == ';') || ((a) == ','))
+static bool is_cipher_list_separator(char c, int is_strict) {
+  if (c == ':') {
+    return true;
+  }
+  return !is_strict && (c == ' ' || c == ';' || c == ',');
+}
 
 /* rule_equals returns one iff the NUL-terminated string |rule| is equal to the
  * |buf_len| bytes at |buf|. */
@@ -1092,7 +1096,7 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
       return 0;
     }
 
-    if (ITEM_SEP(ch)) {
+    if (is_cipher_list_separator(ch, strict)) {
       l++;
       continue;
     }
@@ -1186,7 +1190,7 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
 
       /* We do not support any "multi" options together with "@", so throw away
        * the rest of the command, if any left, until end or ':' is found. */
-      while (*l != '\0' && !ITEM_SEP(*l)) {
+      while (*l != '\0' && !is_cipher_list_separator(*l, strict)) {
         l++;
       }
     } else if (!skip_rule) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 48e21810f..22979647c 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -279,6 +279,19 @@ static const CipherTest kCipherTests[] = {
         },
         false,
     },
+    // Spaces, semi-colons and commas are separators.
+    {
+        "AES128-SHA: AES128-SHA256 AES256-SHA ,AES256-SHA256 ; AES128-GCM-SHA256",
+        {
+            {TLS1_CK_RSA_WITH_AES_128_SHA, 0},
+            {TLS1_CK_RSA_WITH_AES_128_SHA256, 0},
+            {TLS1_CK_RSA_WITH_AES_256_SHA, 0},
+            {TLS1_CK_RSA_WITH_AES_256_SHA256, 0},
+            {TLS1_CK_RSA_WITH_AES_128_GCM_SHA256, 0},
+        },
+        // …but not in strict mode.
+        true,
+    },
 };
 
 static const char *kBadRules[] = {
@@ -304,6 +317,8 @@ static const char *kBadRules[] = {
   "[ECDHE-RSA-CHACHA20-POLY1305|ECDHE-RSA-AES128-GCM-SHA256]:@STRENGTH",
   // Opcode supplied, but missing selector.
   "+",
+  // Spaces are forbidden in equal-preference groups.
+  "[AES128-SHA | AES128-SHA256]",
 };
 
 static const char *kMustNotIncludeNull[] = {