From bf5f19231008f98e20899efe5b7e6e556de8a849 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Sat, 1 Jul 2017 11:13:53 -0400
Subject: [PATCH] Add some addition tests for the cipher parsing code and tidy.

The in_group check is redundant and test an extremely absurd corner of
the syntax.

Change-Id: Ia54bcd7cda7ba05415d3a250ee93e1acedcc43d6
Reviewed-on: https://boringssl-review.googlesource.com/17542
Reviewed-by: David Benjamin <davidben@google.com>
---
 ssl/ssl_cipher.c | 17 +++++------------
 ssl/ssl_test.cc  | 28 +++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/ssl/ssl_cipher.c b/ssl/ssl_cipher.c
index 562c1f30..5d888783 100644
--- a/ssl/ssl_cipher.c
+++ b/ssl/ssl_cipher.c
@@ -1035,7 +1035,7 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
   uint32_t alg_mkey, alg_auth, alg_enc, alg_mac;
   uint16_t min_version;
   const char *l, *buf;
-  int multi, skip_rule, rule, ok, in_group = 0, has_group = 0;
+  int multi, skip_rule, rule, in_group = 0, has_group = 0;
   size_t j, buf_len;
   uint32_t cipher_id;
   char ch;
@@ -1082,10 +1082,7 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
       rule = CIPHER_SPECIAL;
       l++;
     } else if (ch == '[') {
-      if (in_group) {
-        OPENSSL_PUT_ERROR(SSL, SSL_R_NESTED_GROUP);
-        return 0;
-      }
+      assert(!in_group);
       in_group = 1;
       has_group = 1;
       l++;
@@ -1185,15 +1182,11 @@ static int ssl_cipher_process_rulestr(const SSL_PROTOCOL_METHOD *ssl_method,
 
     /* Ok, we have the rule, now apply it. */
     if (rule == CIPHER_SPECIAL) {
-      /* special command */
-      ok = 0;
-      if (buf_len == 8 && !strncmp(buf, "STRENGTH", 8)) {
-        ok = ssl_cipher_strength_sort(head_p, tail_p);
-      } else {
+      if (buf_len != 8 || strncmp(buf, "STRENGTH", 8) != 0) {
         OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_COMMAND);
+        return 0;
       }
-
-      if (ok == 0) {
+      if (!ssl_cipher_strength_sort(head_p, tail_p)) {
         return 0;
       }
 
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 7737e756..2c648acb 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -180,7 +180,7 @@ static const CipherTest kCipherTests[] = {
     // Standard names may be used instead of OpenSSL names.
     {
         "[TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256|"
-         "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256]:"
+        "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256]:"
         "[TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256]:"
         "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
         {
@@ -214,6 +214,32 @@ static const CipherTest kCipherTests[] = {
         },
         false,
     },
+    // Additional masks after @STRENGTH get silently discarded.
+    //
+    // TODO(davidben): Make this an error. If not silently discarded, they get
+    // interpreted as + opcodes which are very different.
+    {
+        "ECDHE-RSA-AES128-GCM-SHA256:"
+        "ECDHE-RSA-AES256-GCM-SHA384:"
+        "@STRENGTH+AES256",
+        {
+            {TLS1_CK_ECDHE_RSA_WITH_AES_256_GCM_SHA384, 0},
+            {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
+        },
+        false,
+    },
+    {
+        "ECDHE-RSA-AES128-GCM-SHA256:"
+        "ECDHE-RSA-AES256-GCM-SHA384:"
+        "@STRENGTH+AES256:"
+        "ECDHE-RSA-CHACHA20-POLY1305",
+        {
+            {TLS1_CK_ECDHE_RSA_WITH_AES_256_GCM_SHA384, 0},
+            {TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 0},
+            {TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 0},
+        },
+        false,
+    },
     // Exact ciphers may not be used in multi-part rules; they are treated
     // as unknown aliases.
     {