Move curve check out of tls12_check_peer_sigalg.

The current check has two problems:

- It only runs on the server, where there isn't a curve list at all. This was a
  mistake in https://boringssl-review.googlesource.com/1843 which flipped it
  from client-only to server-only.

- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
  Upstream added it as part of their Suite B mode, which requires 1.2.

Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.

Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_clnt.c

@@ -920,11 +920,10 @@ err:
   return -1;
 }
 
-/* ssl3_check_certificate_for_cipher returns one if |leaf| is a suitable server
- * certificate type for |cipher|. Otherwise, it returns zero and pushes an error
- * on the error queue. */
-static int ssl3_check_certificate_for_cipher(X509 *leaf,
-                                             const SSL_CIPHER *cipher) {
+/* ssl3_check_leaf_certificate returns one if |leaf| is a suitable leaf server
+ * certificate for |ssl|. Otherwise, it returns zero and pushes an error on the
+ * error queue. */
+static int ssl3_check_leaf_certificate(SSL *ssl, X509 *leaf) {
   int ret = 0;
   EVP_PKEY *pkey = X509_get_pubkey(leaf);
   if (pkey == NULL) {
@@ -932,6 +931,7 @@ static int ssl3_check_certificate_for_cipher(X509 *leaf,
   }
 
   /* Check the certificate's type matches the cipher. */
+  const SSL_CIPHER *cipher = ssl->s3->tmp.new_cipher;
   int expected_type = ssl_cipher_get_key_type(cipher);
   assert(expected_type != EVP_PKEY_NONE);
   if (pkey->type != expected_type) {
@@ -939,9 +939,9 @@ static int ssl3_check_certificate_for_cipher(X509 *leaf,
     goto err;
   }
 
-  /* TODO(davidben): This behavior is preserved from upstream. Should key usages
-   * be checked in other cases as well? */
   if (cipher->algorithm_auth & SSL_aECDSA) {
+    /* TODO(davidben): This behavior is preserved from upstream. Should key
+     * usages be checked in other cases as well? */
     /* This call populates the ex_flags field correctly */
     X509_check_purpose(leaf, -1, 0);
     if ((leaf->ex_flags & EXFLAG_KUSAGE) &&
@@ -949,6 +949,11 @@ static int ssl3_check_certificate_for_cipher(X509 *leaf,
       OPENSSL_PUT_ERROR(SSL, SSL_R_ECC_CERT_NOT_FOR_SIGNING);
       goto err;
     }
+
+    if (!tls1_check_ec_cert(ssl, leaf)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECC_CERT);
+      goto err;
+    }
   }
 
   ret = 1;
@@ -1018,7 +1023,7 @@ int ssl3_get_server_certificate(SSL *s) {
   }
 
   X509 *leaf = sk_X509_value(sk, 0);
-  if (!ssl3_check_certificate_for_cipher(leaf, s->s3->tmp.new_cipher)) {
+  if (!ssl3_check_leaf_certificate(s, leaf)) {
     al = SSL_AD_ILLEGAL_PARAMETER;
     goto f_err;
   }

ssl/t1_lib.c

@@ -660,23 +660,6 @@ int tls12_check_peer_sigalg(SSL *ssl, const EVP_MD **out_md, int *out_alert,
     return 0;
   }
 
-  if (pkey->type == EVP_PKEY_EC) {
-    uint16_t curve_id;
-    uint8_t comp_id;
-    /* Check compression and curve matches extensions */
-    if (!tls1_curve_params_from_ec_key(&curve_id, &comp_id, pkey->pkey.ec)) {
-      *out_alert = SSL_AD_INTERNAL_ERROR;
-      return 0;
-    }
-
-    if (ssl->server && (!tls1_check_curve_id(ssl, curve_id) ||
-                        comp_id != TLSEXT_ECPOINTFORMAT_uncompressed)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
-      *out_alert = SSL_AD_ILLEGAL_PARAMETER;
-      return 0;
-    }
-  }
-
   /* Check signature matches a type we sent */
   sent_sigslen = tls12_get_psigalgs(ssl, &sent_sigs);
   for (i = 0; i < sent_sigslen; i += 2, sent_sigs += 2) {

ssl/test/bssl_shim.cc

@@ -42,6 +42,7 @@
 #include <openssl/crypto.h>
 #include <openssl/err.h>
 #include <openssl/hmac.h>
+#include <openssl/obj.h>
 #include <openssl/rand.h>
 #include <openssl/ssl.h>
 
@@ -1223,6 +1224,12 @@ static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
   if (config->disable_npn) {
     SSL_set_options(ssl.get(), SSL_OP_DISABLE_NPN);
   }
+  if (config->p384_only) {
+    int nid = NID_secp384r1;
+    if (!SSL_set1_curves(ssl.get(), &nid, 1)) {
+      return false;
+    }
+  }
 
   int sock = Connect(config->port);
   if (sock == -1) {

ssl/test/runner/runner.go

@@ -1997,6 +1997,16 @@ func addBasicTests() {
 			resumeSession:        true,
 			expectResumeRejected: true,
 		},
+		{
+			name: "CheckLeafCurve",
+			config: Config{
+				CipherSuites: []uint16{TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
+				Certificates: []Certificate{getECDSACertificate()},
+			},
+			flags:         []string{"-p384-only"},
+			shouldFail:    true,
+			expectedError: ":BAD_ECC_CERT:",
+		},
 	}
 	testCases = append(testCases, basicTests...)
 }

ssl/test/test_config.cc

@@ -98,6 +98,7 @@ const Flag<bool> kBoolFlags[] = {
   { "-renegotiate-freely", &TestConfig::renegotiate_freely },
   { "-renegotiate-ignore", &TestConfig::renegotiate_ignore },
   { "-disable-npn", &TestConfig::disable_npn },
+  { "-p384-only", &TestConfig::p384_only },
 };
 
 const Flag<std::string> kStringFlags[] = {

ssl/test/test_config.h

@@ -101,6 +101,7 @@ struct TestConfig {
   bool renegotiate_ignore = false;
   bool disable_npn = false;
   int expect_server_key_exchange_hash = 0;
+  bool p384_only = false;
 };
 
 bool ParseConfig(int argc, char **argv, TestConfig *out_config);