commit d8dbde79f9803c4cfce11eadfd49ca1284b49734
author David Benjamin <davidben@google.com> Thu Nov 30 16:27:25 2017 -0500
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Dec 05 19:46:27 2017 +0000
tree bb5d848f2066cc8e6aafaaf9578587dbf094bac0
parent 6979c7e8eb0c96cbf57bd8f657d59a50f0487fcf

Don't allow negative EC_KEY private keys.

We check that the private key is less than the order, but we forgot the
other end.

Update-Note: It's possible some caller was relying on this, but since
    that function already checked the other half of the range, I'm
    expecting this to be a no-op change.

Change-Id: I4a53357d7737735b3cfbe97d379c8ca4eca5d5ac
Reviewed-on: https://boringssl-review.googlesource.com/23665
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

crypto/fipsmodule/ec/ec_key.c

diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c
index bba4402..f64cb21 100644
--- a/crypto/fipsmodule/ec/ec_key.c
+++ b/crypto/fipsmodule/ec/ec_key.c
@@ -242,7 +242,8 @@
   }
   // XXX: |BN_cmp| is not constant time.
   if (key->priv_key != NULL &&
-      BN_cmp(key->priv_key, EC_GROUP_get0_order(group)) >= 0) {
+      (BN_is_negative(key->priv_key) ||
+       BN_cmp(key->priv_key, EC_GROUP_get0_order(group)) >= 0)) {
     return 0;
   }
   return 1;
@@ -255,7 +256,8 @@
 int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key) {
   // XXX: |BN_cmp| is not constant time.
   if (key->group != NULL &&
-      BN_cmp(priv_key, EC_GROUP_get0_order(key->group)) >= 0) {
+      (BN_is_negative(priv_key) ||
+       BN_cmp(priv_key, EC_GROUP_get0_order(key->group)) >= 0)) {
     OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER);
     return 0;
   }
@@ -318,7 +320,8 @@
   // check if generator * priv_key == pub_key
   if (eckey->priv_key) {
     // XXX: |BN_cmp| is not constant time.
-    if (BN_cmp(eckey->priv_key, EC_GROUP_get0_order(eckey->group)) >= 0) {
+    if (BN_is_negative(eckey->priv_key) ||
+        BN_cmp(eckey->priv_key, EC_GROUP_get0_order(eckey->group)) >= 0) {
       OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER);
       goto err;
     }

crypto/fipsmodule/ec/ec_test.cc

diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index 139840e..8e7a81d 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -510,6 +510,25 @@
   EXPECT_EQ(0, EC_POINT_cmp(group.get(), result.get(), generator, nullptr));
 }
 
+// Test that EC_KEY_set_private_key rejects invalid values.
+TEST_P(ECCurveTest, SetInvalidPrivateKey) {
+  bssl::UniquePtr<EC_KEY> key(EC_KEY_new_by_curve_name(GetParam().nid));
+  ASSERT_TRUE(key);
+
+  bssl::UniquePtr<BIGNUM> bn(BN_new());
+  ASSERT_TRUE(BN_one(bn.get()));
+  BN_set_negative(bn.get(), 1);
+  EXPECT_FALSE(EC_KEY_set_private_key(key.get(), bn.get()))
+      << "Unexpectedly set a key of -1";
+  ERR_clear_error();
+
+  ASSERT_TRUE(
+      BN_copy(bn.get(), EC_GROUP_get0_order(EC_KEY_get0_group(key.get()))));
+  EXPECT_FALSE(EC_KEY_set_private_key(key.get(), bn.get()))
+      << "Unexpectedly set a key of the group order.";
+  ERR_clear_error();
+}
+
 static std::vector<EC_builtin_curve> AllCurves() {
   const size_t num_curves = EC_get_builtin_curves(nullptr, 0);
   std::vector<EC_builtin_curve> curves(num_curves);