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>
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index e0f75c3..974a371 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -13599,7 +13599,7 @@
},
})
- // 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 @@
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.
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 562fecb..caaf0c7 100644
--- a/ssl/tls13_server.cc
+++ b/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 @@
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 @@
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 @@
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;
}
}
