Check ECDSA curves in TLS 1.2 servers

In TLS 1.2 and below, the supported_curves list simultaneously contrains
ECDH and ECDSA. Since BoringSSL, previously, did not handle ECDSA
certificate selection in the library, we ignored the latter and left it
to the callers. If configured with an ECDSA certificate that didn't
match the peer's curve list, we proceeded anyway, and left it to the
client to reject the connection.

This contradicts RFC 8422, which says:

   The server constructs an appropriate certificate chain and conveys it
   to the client in the Certificate message.  If the client has used a
   Supported Elliptic Curves Extension, the public key in the server's
   certificate MUST respect the client's choice of elliptic curves.  A
   server that cannot satisfy this requirement MUST NOT choose an ECC
   cipher suite in its ServerHello message.)

As with the previous client certificate change, once we move certificate
selection into the library, we'll need to evaluate this ourselves. A
natural implementation of it will, as a side effect, cause us to enforce
this match, even when only a single certificate is configured. This CL
lands that behavior change ahead of time and, in case there are
compatibility impats, leaves a flag, SSL_set_check_ecdsa_curve, to
restore the old behavior. If the change goes through fine, we can retire
the flag after a few months.

If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, but these all result in some slightly suboptimal behavior,
so I think we should treat them as contingency plans.

To help debugging, I gave this a dedicated error, though doing so is a
little tricky because of the PSK fallback. (See the
CheckECDSACurve-PSK-TLS12 test.)

Update-Note: A TLS 1.2 (or below) server, using an ECDSA certificate,
connecting to a client which doesn't advertise its ECDSA curve will now
fail the connection slightly earlier, rather than sending the
certificate and waiting for the client to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_WRONG_CURVE. If the client was buggy and did not correctly
advertise its own capabilities, this may cause a connection to fail
despite previously succeeding. We have included a temporary API,
SSL_set_check_ecdsa_curve, to disable this behavior in the event this
has any impact, but please contact the BoringSSL team if you need it,
as it will interfere with improvements down the line.

TLS 1.3 is not impacted by this change, neither are clients, or RSA
certificiates. Additionally, if your server was already looking at the
curve list before configuring an ECDSA certificate in TLS 1.2, this
will also have no impact.

Bug: 249
Change-Id: I2f1d4e2627641319556847cbbbcdddf347bbc8a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66688
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

6 files changed, 132 insertions, 20 deletions

diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 661aeda..510062a 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -295,24 +295,14 @@ static UniquePtr<STACK_OF(SSL_CIPHER)> ssl_parse_client_cipher_list(
 // ssl_get_compatible_server_ciphers determines the key exchange and
 // authentication cipher suite masks compatible with the server configuration
 // and current ClientHello parameters of |hs|. It sets |*out_mask_k| to the key
-// exchange mask and |*out_mask_a| to the authentication mask.
-static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs,
+// exchange mask and |*out_mask_a| to the authentication mask. It returns true
+// on success and false on error.
+static bool ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs,
                                               uint32_t *out_mask_k,
                                               uint32_t *out_mask_a) {
   uint32_t mask_k = 0;
   uint32_t mask_a = 0;
 
-  if (ssl_has_certificate(hs)) {
-    uint16_t unused;
-    bool sign_ok = tls1_choose_signature_algorithm(hs, &unused);
-    ERR_clear_error();
-
-    mask_a |= ssl_cipher_auth_mask_for_key(hs->local_pubkey.get(), sign_ok);
-    if (EVP_PKEY_id(hs->local_pubkey.get()) == EVP_PKEY_RSA) {
-      mask_k |= SSL_kRSA;
-    }
-  }
-
   // Check for a shared group to consider ECDHE ciphers.
   uint16_t unused;
   if (tls1_get_shared_group(hs, &unused)) {
@@ -320,13 +310,47 @@ static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs,
   }
 
   // PSK requires a server callback.
-  if (hs->config->psk_server_callback != NULL) {
+  if (hs->config->psk_server_callback != nullptr) {
     mask_k |= SSL_kPSK;
     mask_a |= SSL_aPSK;
   }
 
+  if (ssl_has_certificate(hs)) {
+    bool sign_ok = tls1_choose_signature_algorithm(hs, &unused);
+    ERR_clear_error();
+
+    // ECDSA keys must additionally be checked against the peer's supported
+    // curve list.
+    int key_type = EVP_PKEY_id(hs->local_pubkey.get());
+    if (hs->config->check_ecdsa_curve && key_type == EVP_PKEY_EC) {
+      EC_KEY *ec_key = EVP_PKEY_get0_EC_KEY(hs->local_pubkey.get());
+      uint16_t group_id;
+      if (!ssl_nid_to_group_id(
+              &group_id, EC_GROUP_get_curve_name(EC_KEY_get0_group(ec_key))) ||
+          std::find(hs->peer_supported_group_list.begin(),
+                    hs->peer_supported_group_list.end(),
+                    group_id) == hs->peer_supported_group_list.end()) {
+        sign_ok = false;
+
+        // If this would make us unable to pick any cipher, return an error.
+        // This is not strictly necessary, but it gives us a more specific
+        // error to help the caller diagnose issues.
+        if (mask_a == 0) {
+          OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
+          return false;
+        }
+      }
+    }
+
+    mask_a |= ssl_cipher_auth_mask_for_key(hs->local_pubkey.get(), sign_ok);
+    if (key_type == EVP_PKEY_RSA) {
+      mask_k |= SSL_kRSA;
+    }
+  }
+
   *out_mask_k = mask_k;
   *out_mask_a = mask_a;
+  return true;
 }
 
 static const SSL_CIPHER *choose_cipher(
@@ -360,7 +384,9 @@ static const SSL_CIPHER *choose_cipher(
   }
 
   uint32_t mask_k, mask_a;
-  ssl_get_compatible_server_ciphers(hs, &mask_k, &mask_a);
+  if (!ssl_get_compatible_server_ciphers(hs, &mask_k, &mask_a)) {
+    return nullptr;
+  }
 
   for (size_t i = 0; i < sk_SSL_CIPHER_num(prio); i++) {
     const SSL_CIPHER *c = sk_SSL_CIPHER_value(prio, i);
@@ -395,6 +421,7 @@ static const SSL_CIPHER *choose_cipher(
     }
   }
 
+  OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
   return nullptr;
 }
 
@@ -818,7 +845,6 @@ static enum ssl_hs_wait_t do_select_certificate(SSL_HANDSHAKE *hs) {
                                        : ssl->ctx->cipher_list.get();
   hs->new_cipher = choose_cipher(hs, &client_hello, prefs);
   if (hs->new_cipher == NULL) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
     return ssl_hs_error;
   }
diff --git a/ssl/internal.h b/ssl/internal.h
index 844c579..8be743b 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -3195,6 +3195,10 @@ struct SSL_CONFIG {
   // below, will check its certificate against the server's requested
   // certificate types.
   bool check_client_certificate_type : 1;
+
+  // check_ecdsa_curve indicates whether the server, in TLS 1.2 and below, will
+  // check its certificate against the client's supported ECDSA curves.
+  bool check_ecdsa_curve : 1;
 };
 
 // From RFC 8446, used in determining PSK modes.
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index b0d2968..3f767a4 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -706,7 +706,8 @@ SSL_CONFIG::SSL_CONFIG(SSL *ssl_arg)
       quic_use_legacy_codepoint(false),
       permute_extensions(false),
       alps_use_new_codepoint(false),
-      check_client_certificate_type(true) {
+      check_client_certificate_type(true),
+      check_ecdsa_curve(true) {
   assert(ssl);
 }
 
@@ -3048,6 +3049,13 @@ void SSL_set_check_client_certificate_type(SSL *ssl, int enable) {
   ssl->config->check_client_certificate_type = !!enable;
 }
 
+void SSL_set_check_ecdsa_curve(SSL *ssl, int enable) {
+  if (!ssl->config) {
+    return;
+  }
+  ssl->config->check_ecdsa_curve = !!enable;
+}
+
 void SSL_set_quic_use_legacy_codepoint(SSL *ssl, int use_legacy) {
   if (!ssl->config) {
     return;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index bbb4d36..24e4ac7 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -9961,11 +9961,13 @@ func addSignatureAlgorithmTests() {
 			}
 
 			var curveFlags []string
+			var runnerCurves []CurveID
 			if alg.curve != 0 && ver.version <= VersionTLS12 {
 				// In TLS 1.2, the ECDH curve list also constrains ECDSA keys. Ensure the
-				// corresponding curve is enabled on the shim. Also include X25519 to
-				// ensure the shim and runner have something in common for ECDH.
+				// corresponding curve is enabled. Also include X25519 to ensure the shim
+				// and runner have something in common for ECDH.
 				curveFlags = flagInts("-curves", []int{int(CurveX25519), int(alg.curve)})
+				runnerCurves = []CurveID{CurveX25519, alg.curve}
 			}
 
 			var signError, signLocalError, verifyError, verifyLocalError, defaultError, defaultLocalError string
@@ -9994,7 +9996,8 @@ func addSignatureAlgorithmTests() {
 					testType: testType,
 					name:     prefix + "Sign" + suffix,
 					config: Config{
-						MaxVersion: ver.version,
+						MaxVersion:       ver.version,
+						CurvePreferences: runnerCurves,
 						VerifySignatureAlgorithms: []signatureAlgorithm{
 							fakeSigAlg1,
 							alg.id,
@@ -10018,6 +10021,7 @@ func addSignatureAlgorithmTests() {
 					name:     prefix + "Sign-Negotiate" + suffix,
 					config: Config{
 						MaxVersion:                ver.version,
+						CurvePreferences:          runnerCurves,
 						VerifySignatureAlgorithms: allAlgorithms,
 					},
 					shimCertificate: cert,
@@ -12062,6 +12066,71 @@ func addCurveTests() {
 			"-expect-curve-id", strconv.Itoa(int(CurveX25519)),
 		},
 	})
+
+	// In TLS 1.2, the curve list is also used to signal ECDSA curves.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "CheckECDSACurve-TLS12",
+		config: Config{
+			MinVersion:       VersionTLS12,
+			MaxVersion:       VersionTLS12,
+			CurvePreferences: []CurveID{CurveP384},
+		},
+		shimCertificate: &ecdsaP256Certificate,
+		shouldFail:      true,
+		expectedError:   ":WRONG_CURVE:",
+	})
+
+	// This behavior may, temporarily, be disabled with a flag.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "NoCheckECDSACurve-TLS12",
+		config: Config{
+			MinVersion:       VersionTLS12,
+			MaxVersion:       VersionTLS12,
+			CurvePreferences: []CurveID{CurveP384},
+		},
+		shimCertificate: &ecdsaP256Certificate,
+		flags:           []string{"-no-check-ecdsa-curve"},
+	})
+
+	// If the ECDSA certificate is ineligible due to a curve mismatch, the
+	// server may still consider a PSK cipher suite.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "CheckECDSACurve-PSK-TLS12",
+		config: Config{
+			MinVersion: VersionTLS12,
+			MaxVersion: VersionTLS12,
+			CipherSuites: []uint16{
+				TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+				TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA,
+			},
+			CurvePreferences:     []CurveID{CurveP384},
+			PreSharedKey:         []byte("12345"),
+			PreSharedKeyIdentity: "luggage combo",
+		},
+		shimCertificate: &ecdsaP256Certificate,
+		expectations: connectionExpectations{
+			cipher: TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA,
+		},
+		flags: []string{
+			"-psk", "12345",
+			"-psk-identity", "luggage combo",
+		},
+	})
+
+	// In TLS 1.3, the curve list only controls ECDH.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "CheckECDSACurve-NotApplicable-TLS13",
+		config: Config{
+			MinVersion:       VersionTLS13,
+			MaxVersion:       VersionTLS13,
+			CurvePreferences: []CurveID{CurveP384},
+		},
+		shimCertificate: &ecdsaP256Certificate,
+	})
 }
 
 func addTLS13RecordTests() {
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 1883558..c0b435e 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -421,6 +421,7 @@ std::vector<Flag> SortedFlags() {
       BoolFlag("-wpa-202304", &TestConfig::wpa_202304),
       BoolFlag("-no-check-client-certificate-type",
                &TestConfig::no_check_client_certificate_type),
+      BoolFlag("-no-check-ecdsa-curve", &TestConfig::no_check_ecdsa_curve),
   };
   std::sort(flags.begin(), flags.end(), [](const Flag &a, const Flag &b) {
     return strcmp(a.name, b.name) < 0;
@@ -1787,6 +1788,9 @@ bssl::UniquePtr<SSL> TestConfig::NewSSL(
   if (no_check_client_certificate_type) {
     SSL_set_check_client_certificate_type(ssl.get(), 0);
   }
+  if (no_check_ecdsa_curve) {
+    SSL_set_check_ecdsa_curve(ssl.get(), 0);
+  }
   if (no_tls13) {
     SSL_set_options(ssl.get(), SSL_OP_NO_TLSv1_3);
   }
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 8911a66..26a11bc 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -201,6 +201,7 @@ struct TestConfig {
   bool fips_202205 = false;
   bool wpa_202304 = false;
   bool no_check_client_certificate_type = false;
+  bool no_check_ecdsa_curve = false;
 
   std::vector<const char*> handshaker_args;