Make 256-bit ciphers a preference for CECPQ2, not a requirement.

If 256-bit ciphers are a requirement for CECPQ2 then that introduces a
link between supported ciphers and supported groups: offering CECPQ2
without a 256-bit cipher is invalid. But that's a little weird since
these things were otherwise independent.

So, rather than require a 256-bit cipher for CECPQ2, just prefer them.

Change-Id: I491749e41708cd9c5eeed5b4ae23c11e5c0b9725
Reviewed-on: https://boringssl-review.googlesource.com/c/34504
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

ssl/test/runner/runner.go

@@ -13599,7 +13599,7 @@ func addTLS13CipherPreferenceTests() {
 		},
 	})
 
-	// Test that CECPQ2 cannot be used with TLS_AES_128_GCM_SHA256.
+	// CECPQ2 prefers 256-bit ciphers but will use AES-128 if there's nothing else.
 	testCases = append(testCases, testCase{
 		testType: serverTest,
 		name:     "TLS13-CipherPreference-CECPQ2-AES128Only",
@@ -13612,9 +13612,39 @@ func addTLS13CipherPreferenceTests() {
 		flags: []string{
 			"-curves", strconv.Itoa(int(CurveCECPQ2)),
 		},
-		shouldFail:         true,
-		expectedError:      ":NO_SHARED_CIPHER:",
-		expectedLocalError: "remote error: handshake failure",
+	})
+	// When a 256-bit cipher is offered, even if not in first place, it should be
+	// picked.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "TLS13-CipherPreference-CECPQ2-AES256Preferred",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			CipherSuites: []uint16{
+				TLS_AES_128_GCM_SHA256,
+				TLS_AES_256_GCM_SHA384,
+			},
+		},
+		flags: []string{
+			"-curves", strconv.Itoa(int(CurveCECPQ2)),
+		},
+		expectedCipher: TLS_AES_256_GCM_SHA384,
+	})
+	// ... but when CECPQ2 isn't being used, the client's preference controls.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "TLS13-CipherPreference-CECPQ2-AES128PreferredOtherwise",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			CipherSuites: []uint16{
+				TLS_AES_128_GCM_SHA256,
+				TLS_AES_256_GCM_SHA384,
+			},
+		},
+		flags: []string{
+			"-curves", strconv.Itoa(int(CurveX25519)),
+		},
+		expectedCipher: TLS_AES_128_GCM_SHA256,
 	})
 
 	// Test that CECPQ2 continues to honor AES vs ChaCha20 logic.

ssl/tls13_server.cc

@@ -17,6 +17,8 @@
 #include <assert.h>
 #include <string.h>
 
+#include <tuple>
+
 #include <openssl/aead.h>
 #include <openssl/bytestring.h>
 #include <openssl/digest.h>
@@ -95,6 +97,37 @@ static int ssl_ext_supported_versions_add_serverhello(SSL_HANDSHAKE *hs,
   return 1;
 }
 
+// CipherScorer produces a "score" for each possible cipher suite offered by
+// the client.
+class CipherScorer {
+ public:
+  CipherScorer(uint16_t group_id)
+      : aes_is_fine_(EVP_has_aes_hardware()),
+        security_128_is_fine_(group_id != SSL_CURVE_CECPQ2) {}
+
+  typedef std::tuple<bool, bool, bool> Score;
+
+  // MinScore returns a |Score| that will compare less than the score of all
+  // cipher suites.
+  Score MinScore() const {
+    return Score(false, false, false);
+  }
+
+  Score Evaluate(const SSL_CIPHER *a) const {
+    return Score(
+        // Something is always preferable to nothing.
+        true,
+        // Either 128-bit is fine, or 256-bit is preferred.
+        security_128_is_fine_ || a->algorithm_enc != SSL_AES128GCM,
+        // Either AES is fine, or else ChaCha20 is preferred.
+        aes_is_fine_ || a->algorithm_enc == SSL_CHACHA20POLY1305);
+  }
+
+ private:
+  const bool aes_is_fine_;
+  const bool security_128_is_fine_;
+};
+
 static const SSL_CIPHER *choose_tls13_cipher(
     const SSL *ssl, const SSL_CLIENT_HELLO *client_hello, uint16_t group_id) {
   if (client_hello->cipher_suites_len % 2 != 0) {
@@ -105,11 +138,12 @@ static const SSL_CIPHER *choose_tls13_cipher(
   CBS_init(&cipher_suites, client_hello->cipher_suites,
            client_hello->cipher_suites_len);
 
-  const bool aes_is_fine = EVP_has_aes_hardware();
-  const bool require_256_bit = group_id == SSL_CURVE_CECPQ2;
   const uint16_t version = ssl_protocol_version(ssl);
 
   const SSL_CIPHER *best = nullptr;
+  CipherScorer scorer(group_id);
+  CipherScorer::Score best_score = scorer.MinScore();
+
   while (CBS_len(&cipher_suites) > 0) {
     uint16_t cipher_suite;
     if (!CBS_get_u16(&cipher_suites, &cipher_suite)) {
@@ -124,23 +158,12 @@ static const SSL_CIPHER *choose_tls13_cipher(
       continue;
     }
 
-    // Post-quantum key exchanges should be paired with 256-bit ciphers.
-    if (require_256_bit && candidate->algorithm_enc == SSL_AES128GCM) {
-      continue;
-    }
-
-    // TLS 1.3 removes legacy ciphers, so honor the client order, but prefer
-    // ChaCha20 if we do not have AES hardware.
-    if (aes_is_fine) {
-      return candidate;
-    }
-
-    if (candidate->algorithm_enc == SSL_CHACHA20POLY1305) {
-      return candidate;
-    }
-
-    if (best == nullptr) {
+    const CipherScorer::Score candidate_score = scorer.Evaluate(candidate);
+    // |candidate_score| must be larger to displace the current choice. That way
+    // the client's order controls between ciphers with an equal score.
+    if (candidate_score > best_score) {
       best = candidate;
+      best_score = candidate_score;
     }
   }