Do not allow changing keys with parameters already set.

This aligns with upstream's f72f00d49549c6620d7101f5e9bf7963da6df9ee. In
doing so, I had to fill in a bunch of NULL checks in p_ec_asn1.c, to
account for EVP's needlessly many "empty" states. For now, those cases
return a goofy -2 to align with upstream. Our EVP_PKEY_cmp_parameters
still returns negative values, so this is fine, though ideally we'd
narrow to boolean. That probably depends on some other changes. See
https://crbug.com/boringssl/536#c3.

Bug: 536
Change-Id: I1124c8ad5223ac23953d94ff9ca734fbb714e89c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55185
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

crypto/evp/evp.c

@@ -167,10 +167,21 @@ int EVP_PKEY_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
     return 0;
   }
 
+  // Once set, parameters may not change.
+  if (!EVP_PKEY_missing_parameters(to)) {
+    if (EVP_PKEY_cmp_parameters(to, from) == 1) {
+      return 1;
+    }
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DIFFERENT_PARAMETERS);
+    return 0;
+  }
+
   if (from->ameth && from->ameth->param_copy) {
     return from->ameth->param_copy(to, from);
   }
 
+  // TODO(https://crbug.com/boringssl/536): If the algorithm takes no
+  // parameters, copying them should vacuously succeed.
   return 0;
 }
 
@@ -419,6 +430,8 @@ int EVP_PKEY_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b) {
   if (a->ameth && a->ameth->param_cmp) {
     return a->ameth->param_cmp(a, b);
   }
+  // TODO(https://crbug.com/boringssl/536): If the algorithm doesn't use
+  // parameters, they should compare as vacuously equal.
   return -2;
 }
 

crypto/evp/evp_extra_test.cc

@@ -1150,3 +1150,73 @@ TEST(EVPExtraTest, TLSEncodedPoint) {
     EXPECT_EQ(Bytes(CBB_data(cbb.get()), CBB_len(cbb.get())), Bytes(test.spki));
   }
 }
+
+TEST(EVPExtraTest, Parameters) {
+  auto new_pkey_with_type = [](int type) -> bssl::UniquePtr<EVP_PKEY> {
+    bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
+    if (!pkey ||  //
+        !EVP_PKEY_set_type(pkey.get(), type)) {
+      return nullptr;
+    }
+    return pkey;
+  };
+
+  auto new_pkey_with_curve = [](int curve_nid) -> bssl::UniquePtr<EVP_PKEY> {
+    bssl::UniquePtr<EVP_PKEY_CTX> ctx(
+        EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr));
+    EVP_PKEY *pkey = nullptr;
+    if (!ctx ||  //
+        !EVP_PKEY_paramgen_init(ctx.get()) ||
+        !EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx.get(), curve_nid) ||
+        !EVP_PKEY_paramgen(ctx.get(), &pkey)) {
+      return nullptr;
+    }
+    return bssl::UniquePtr<EVP_PKEY>(pkey);
+  };
+
+  // RSA keys have no parameters.
+  bssl::UniquePtr<EVP_PKEY> rsa = new_pkey_with_type(EVP_PKEY_RSA);
+  ASSERT_TRUE(rsa);
+  EXPECT_FALSE(EVP_PKEY_missing_parameters(rsa.get()));
+
+  // EC keys have parameters.
+  bssl::UniquePtr<EVP_PKEY> ec_no_params = new_pkey_with_type(EVP_PKEY_EC);
+  ASSERT_TRUE(ec_no_params);
+  EXPECT_TRUE(EVP_PKEY_missing_parameters(ec_no_params.get()));
+
+  bssl::UniquePtr<EVP_PKEY> p256 = new_pkey_with_curve(NID_X9_62_prime256v1);
+  ASSERT_TRUE(p256);
+  EXPECT_FALSE(EVP_PKEY_missing_parameters(p256.get()));
+
+  bssl::UniquePtr<EVP_PKEY> p256_2 = new_pkey_with_curve(NID_X9_62_prime256v1);
+  ASSERT_TRUE(p256_2);
+  EXPECT_FALSE(EVP_PKEY_missing_parameters(p256_2.get()));
+
+  bssl::UniquePtr<EVP_PKEY> p384 = new_pkey_with_curve(NID_secp384r1);
+  ASSERT_TRUE(p384);
+  EXPECT_FALSE(EVP_PKEY_missing_parameters(p384.get()));
+
+  EXPECT_EQ(1, EVP_PKEY_cmp_parameters(p256.get(), p256_2.get()));
+  EXPECT_EQ(0, EVP_PKEY_cmp_parameters(p256.get(), p384.get()));
+
+  // Copying parameters onto a curve-less EC key works.
+  ASSERT_TRUE(EVP_PKEY_copy_parameters(ec_no_params.get(), p256.get()));
+  EXPECT_EQ(1, EVP_PKEY_cmp_parameters(p256.get(), ec_no_params.get()));
+
+  // No-op copies silently succeed.
+  ASSERT_TRUE(EVP_PKEY_copy_parameters(ec_no_params.get(), p256.get()));
+  EXPECT_EQ(1, EVP_PKEY_cmp_parameters(p256.get(), ec_no_params.get()));
+
+  // Copying parameters onto a type-less key works.
+  bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
+  ASSERT_TRUE(pkey);
+  ASSERT_TRUE(EVP_PKEY_copy_parameters(pkey.get(), p256.get()));
+  EXPECT_EQ(EVP_PKEY_EC, EVP_PKEY_id(pkey.get()));
+  EXPECT_EQ(1, EVP_PKEY_cmp_parameters(p256.get(), pkey.get()));
+
+  // |EVP_PKEY_copy_parameters| cannot change a key's type or curve.
+  EXPECT_FALSE(EVP_PKEY_copy_parameters(rsa.get(), p256.get()));
+  EXPECT_EQ(EVP_PKEY_RSA, EVP_PKEY_id(rsa.get()));
+  EXPECT_FALSE(EVP_PKEY_copy_parameters(rsa.get(), p256.get()));
+  EXPECT_EQ(EVP_PKEY_RSA, EVP_PKEY_id(rsa.get()));
+}

crypto/evp/p_ec_asn1.c

@@ -216,7 +216,7 @@ static int ec_bits(const EVP_PKEY *pkey) {
 }
 
 static int ec_missing_parameters(const EVP_PKEY *pkey) {
-  return EC_KEY_get0_group(pkey->pkey.ec) == NULL;
+  return pkey->pkey.ec == NULL || EC_KEY_get0_group(pkey->pkey.ec) == NULL;
 }
 
 static int ec_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
@@ -239,8 +239,14 @@ static int ec_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
 }
 
 static int ec_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b) {
+  if (a->pkey.ec == NULL || b->pkey.ec == NULL) {
+    return -2;
+  }
   const EC_GROUP *group_a = EC_KEY_get0_group(a->pkey.ec),
                  *group_b = EC_KEY_get0_group(b->pkey.ec);
+  if (group_a == NULL || group_b == NULL) {
+    return -2;
+  }
   if (EC_GROUP_cmp(group_a, group_b, NULL) != 0) {
     // mismatch
     return 0;