commit 22df69103f807d92595c67ea421aa51b0b34ed13
author Adam Langley <agl@google.com> Tue Jul 25 12:27:37 2017 -0700
committer David Benjamin <davidben@google.com> Tue Jul 25 20:48:44 2017 +0000
tree 9097cf2cef19e37a1c5e2aa2ac7e070dde05047c
parent 6d81cf3d9ccc8f13a503991b4c162e414baaa6d0

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

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 993046c..0196dd9 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1350,8 +1350,9 @@
  *   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 @@
  *   [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

ssl/ssl_cipher.cc

diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc
index dbb4c75..f1a215f 100644
--- a/ssl/ssl_cipher.cc
+++ b/ssl/ssl_cipher.cc
@@ -756,8 +756,12 @@
   }
 }
 
-#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 @@
       return 0;
     }
 
-    if (ITEM_SEP(ch)) {
+    if (is_cipher_list_separator(ch, strict)) {
       l++;
       continue;
     }
@@ -1186,7 +1190,7 @@
 
       /* 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) {

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 48e2181..2297964 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -279,6 +279,19 @@
         },
         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 @@
   "[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[] = {