Bug 1648822 Add stricter validation of DH keys when in FIPS mode.

Update:
FIPS now also requires us to do y^q mod p testing on key generation (always).
We now do that in FIPS mode only, but in all modes we do full DH verification
for DH and ECDH. Because of this, the path has now separated out the prime
checks, which are now only done for the DH operation if we aren't using a known
prime and the subprime value has been provided. I've also learned we can accept
keys that we do full validation on in FIPS mode, so I've added that to
this patch, though we still can't generate those kinds of keys without
adding the subprime at keygen time.

The new FIPS standard is dh operations must use approved primes. Approved
primes are those selected in the tls and ike RFCs. Currently tls and ike
have modes with checks whether the primes are approved, but the check may
not always happen. The safest thing to do in FIPS mode is only allow those
primes. In addition, FIPS requires 1< y < p-1 (or technically 2<=y<=p-2, since
y is an integer those two tests are identical).

While making changes I realized we would want a mode where we can do more strict
checks on the prime while not requiring that the prime be an approved prime. We
already allow for strict checking if q is supplied with the private key, but there
were a couple of issues with that check:

    1. there was no way of actually setting q in the current NSS pk11wrap interfaces.
    2. If the prime was a safe prime, but g was an actual generator, then we would fail the y^q mod p = 1 tests for 50% of the keys, even though those keys are safe.
    3. We weren't checking primality of p and q.

So the old code:

  if (q) {
    check y^q mod p = 1
    if not fail
  }

  check 1 <y < p-1 (done in DH_Derive).

New code:

 if (! p is approved prime) {
   if (FIPS) fail;
   if (q) {
      y_test = y
      if (p,q-> p is a safe prime) {
         y_test = 1
      }
      check prime is prime Fail if not
      check subprime is subprime fail if not
      y_test^q mod p = 1
   }
 }
 check 1 < y < p-1 (done in DH_Derive)

This means:

Existing code non-fips without setting the subprime continues to run as before.
Non-fips code which sets the subprime now runs slower, but p and q are checked
  if p or q where not prime, the derive fails (which it should).
In FIPS mode only approved primes will succeed now.
Non-fips code can now set the subprime to q=(p-1)/2 if it doesn't have an
explicit q value (like in tls). If the derive succeeds, we know that p is a
safe prime. If p is approved, the checks are skipped because we already know
that p is a safe prime. Code can optionally do a test derive on a new p and
remember it's safe so that we know longer need to  check ever call (though if
q is not (p-1)/2, you will need to  continue to do the checks each call
because y could still be a small subgroup).

This patch:

gtests/softoken_gtest

  1. Added New dh tests to softoken_gtests. The tests were added to softoken_gtests
because we need to test both non-FIPS and FIPS mode. Test vectors include a
category, so the same test vectors can be used in FIPS and non-FIPS even though
each class may have different results. Most of the test vectors where created
either by dhparams command in openssl, dsaparams in openssl, and the nss makepqg
command. Each vector includes a label, prime, base, optional subprime, optional
public key, test type, and key class (basically size).
  2. If public key is not supplied, we use a generated public key.
  3. If subPrime is supplied to wet it on the private key after generation.

lib/freebl/dh.c

    add primality tests to KEA_VerifyKey().

lib/softokn/

  1. Allow CKA_SUBPRIME to be set after key generation or import. This affects
how we test for it's existance, since it is now always there on the key, we
check it's length to make sure it's non-zero.
  2. We implement the psuedocode above as real code.
  3. We create two new functions: sftl_VerifyDH_Prime which return SECSuccess if Prime is an approved prime. sftk_IsSafePrime which returns SECSuess of both prime and subprime look reasonable, and sets a Bool to PR_TRUE is subprime -> prime is safe (subprime = (prime-1)/2. These functions are implemented in sftkdhverify.c
  4.Cleanup incorrect nominclature on primes (safe primes are not strong primes).

gtests/softoken_gtest/manifest.mn

@@ -25,11 +25,12 @@ INCLUDES += \
 	-I$(CORE_DEPTH)/cpputil \
 	$(NULL)
 
-REQUIRES = nspr gtest
+REQUIRES = nspr gtest cpputil
 
 PROGRAM = softoken_gtest
 
 EXTRA_LIBS = \
 	$(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) \
+	$(DIST)/lib/$(LIB_PREFIX)cpputil.$(LIB_SUFFIX) \
 	$(DIST)/lib/$(LIB_PREFIX)gtestutil.$(LIB_SUFFIX) \
 	$(NULL)

gtests/softoken_gtest/softoken_gtest.cc

@@ -11,10 +11,14 @@
 
 #define GTEST_HAS_RTTI 0
 #include "gtest/gtest.h"
+#include "databuffer.h"
 #include <fstream>
+#include <chrono>
+using namespace std::chrono;
 
-namespace nss_test {
+#include "softoken_dh_vectors.h"
 
+namespace nss_test {
 class SoftokenTest : public ::testing::Test {
  protected:
   SoftokenTest() : mNSSDBDir("SoftokenTest.d-") {}
@@ -527,11 +531,205 @@ TEST_F(SoftokenNoDBTest, NeedUserInitNoDB) {
   ASSERT_EQ(SECSuccess, NSS_Shutdown());
 }
 
+SECStatus test_dh_value(const PQGParams *params, const SECItem *pub_key_value,
+                        PRBool genFailOK, time_t *time) {
+  SECKEYDHParams dh_params;
+  dh_params.base = params->base;
+  dh_params.prime = params->prime;
+
+  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+  EXPECT_TRUE(slot);
+  if (!slot) return SECFailure;
+
+  /* create a private/public key pair in with the given params */
+  SECKEYPublicKey *pub_tmp = nullptr;
+  ScopedSECKEYPrivateKey priv_key(
+      PK11_GenerateKeyPair(slot.get(), CKM_DH_PKCS_KEY_PAIR_GEN, &dh_params,
+                           &pub_tmp, PR_FALSE, PR_TRUE, nullptr));
+  if ((genFailOK) && ((priv_key.get() == nullptr) || (pub_tmp == nullptr))) {
+    return SECFailure;
+  }
+  EXPECT_NE(nullptr, priv_key.get()) << "PK11_GenerateKeyPair failed: "
+                                     << PORT_ErrorToName(PORT_GetError());
+  EXPECT_NE(nullptr, pub_tmp);
+  if ((priv_key.get() == nullptr) || (pub_tmp == nullptr)) return SECFailure;
+  ScopedSECKEYPublicKey pub_key(pub_tmp);
+  ScopedSECKEYPublicKey peer_pub_key_manager(nullptr);
+  SECKEYPublicKey *peer_pub_key = pub_key.get();
+
+  /* if a subprime has been given set it on the PKCS #11 key */
+  if (params->subPrime.data != nullptr) {
+    SECStatus rv;
+    EXPECT_EQ(SECSuccess, rv = PK11_WriteRawAttribute(
+                              PK11_TypePrivKey, priv_key.get(), CKA_SUBPRIME,
+                              (SECItem *)&params->subPrime))
+        << "PK11_WriteRawAttribute failed: "
+        << PORT_ErrorToString(PORT_GetError());
+    if (rv != SECSuccess) {
+      return rv;
+    }
+  }
+
+  /* find if we weren't passed a public value in, use the
+   * one we just generated */
+  if (pub_key_value && pub_key_value->data) {
+    peer_pub_key = SECKEY_CopyPublicKey(pub_key.get());
+    EXPECT_NE(nullptr, peer_pub_key);
+    if (peer_pub_key == nullptr) {
+      return SECFailure;
+    }
+    peer_pub_key->u.dh.publicValue = *pub_key_value;
+    peer_pub_key_manager.reset(peer_pub_key);
+  }
+
+  /* now do the derive. time it and return the time if
+   * the caller requested it. */
+  auto start = high_resolution_clock::now();
+  ScopedPK11SymKey derivedKey(PK11_PubDerive(
+      priv_key.get(), peer_pub_key, PR_FALSE, nullptr, nullptr,
+      CKM_DH_PKCS_DERIVE, CKM_HKDF_DERIVE, CKA_DERIVE, 32, nullptr));
+  auto stop = high_resolution_clock::now();
+  if (!derivedKey) {
+    std::cerr << "PK11_PubDerive failed: "
+              << PORT_ErrorToString(PORT_GetError()) << std::endl;
+  }
+
+  if (time) {
+    auto duration = duration_cast<microseconds>(stop - start);
+    *time = duration.count();
+  }
+  return derivedKey ? SECSuccess : SECFailure;
+}
+
+class SoftokenDhTest : public SoftokenTest {
+ protected:
+  SoftokenDhTest() : SoftokenTest("SoftokenDhTest.d-") {}
+  time_t reference_time[CLASS_LAST] = {0};
+
+  virtual void SetUp() {
+    SoftokenTest::SetUp();
+
+    ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+    ASSERT_TRUE(slot);
+
+    time_t time;
+    for (int i = CLASS_FIRST; i < CLASS_LAST; i++) {
+      PQGParams params;
+      params.prime.data = (unsigned char *)reference_prime[i];
+      params.prime.len = reference_prime_len[i];
+      params.base.data = (unsigned char *)g2;
+      params.base.len = sizeof(g2);
+      params.subPrime.data = nullptr;
+      params.subPrime.len = 0;
+      ASSERT_EQ(SECSuccess, test_dh_value(&params, nullptr, PR_FALSE, &time));
+      reference_time[i] = time / 2 + 3 * time;
+    }
+  };
+};
+
+const char *param_value(DhParamType param_type) {
+  switch (param_type) {
+    case TLS_APPROVED:
+      return "TLS_APPROVED";
+    case IKE_APPROVED:
+      return "IKE_APPROVED";
+    case SAFE_PRIME:
+      return "SAFE_PRIME";
+    case SAFE_PRIME_WITH_SUBPRIME:
+      return "SAFE_PRIME_WITH_SUBPRIME";
+    case KNOWN_SUBPRIME:
+      return "KNOWN_SUBPRIME";
+    case UNKNOWN_SUBPRIME:
+      return "UNKNOWN_SUBPRIME";
+    case WRONG_SUBPRIME:
+      return "WRONG_SUBPRIME";
+    case BAD_PUB_KEY:
+      return "BAD_PUB_KEY";
+  }
+  return "**Invalid**";
+}
+
+const char *key_value(DhKeyClass key_class) {
+  switch (key_class) {
+    case CLASS_1536:
+      return "CLASS_1536";
+    case CLASS_2048:
+      return "CLASS_2048";
+    case CLASS_3072:
+      return "CLASS_3072";
+    case CLASS_4096:
+      return "CLASS_4096";
+    case CLASS_6144:
+      return "CLASS_6144";
+    case CLASS_8192:
+      return "CLASS_8192";
+    case CLASS_LAST:
+      break;
+  }
+  return "**Invalid**";
+}
+
+class SoftokenDhValidate : public SoftokenDhTest,
+                           public ::testing::WithParamInterface<DhTestVector> {
+};
+
+/* test the DH validation process. In non-fips mode, only BAD_PUB_KEY tests
+ * should fail */
+TEST_P(SoftokenDhValidate, DhVectors) {
+  const DhTestVector dhTestValues = GetParam();
+  std::string testId = (char *)(dhTestValues.id);
+  std::string err = "Test(" + testId + ") failed";
+  SECStatus rv;
+  time_t time;
+
+  PQGParams params;
+  params.prime = dhTestValues.p;
+  params.base = dhTestValues.g;
+  params.subPrime = dhTestValues.q;
+
+  std::cerr << "Test: " + testId << std::endl
+            << "param_type: " << param_value(dhTestValues.param_type)
+            << ", key_class: " << key_value(dhTestValues.key_class) << std::endl
+            << "p: " << DataBuffer(dhTestValues.p.data, dhTestValues.p.len)
+            << std::endl
+            << "g: " << DataBuffer(dhTestValues.g.data, dhTestValues.g.len)
+            << std::endl
+            << "q: " << DataBuffer(dhTestValues.q.data, dhTestValues.q.len)
+            << std::endl
+            << "pub_key: "
+            << DataBuffer(dhTestValues.pub_key.data, dhTestValues.pub_key.len)
+            << std::endl;
+  rv = test_dh_value(&params, &dhTestValues.pub_key, PR_FALSE, &time);
+
+  switch (dhTestValues.param_type) {
+    case TLS_APPROVED:
+    case IKE_APPROVED:
+    case SAFE_PRIME:
+    case UNKNOWN_SUBPRIME:
+      EXPECT_EQ(SECSuccess, rv) << err;
+      EXPECT_LE(time, reference_time[dhTestValues.key_class]) << err;
+      break;
+    case KNOWN_SUBPRIME:
+    case SAFE_PRIME_WITH_SUBPRIME:
+      EXPECT_EQ(SECSuccess, rv) << err;
+      EXPECT_GT(time, reference_time[dhTestValues.key_class]) << err;
+      break;
+    case WRONG_SUBPRIME:
+    case BAD_PUB_KEY:
+      EXPECT_EQ(SECFailure, rv) << err;
+      break;
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(DhValidateCases, SoftokenDhValidate,
+                        ::testing::ValuesIn(DH_TEST_VECTORS));
+
 #ifndef NSS_FIPS_DISABLED
 
 class SoftokenFipsTest : public SoftokenTest {
  protected:
   SoftokenFipsTest() : SoftokenTest("SoftokenFipsTest.d-") {}
+  SoftokenFipsTest(const std::string &prefix) : SoftokenTest(prefix) {}
 
   virtual void SetUp() {
     SoftokenTest::SetUp();
@@ -540,12 +738,42 @@ class SoftokenFipsTest : public SoftokenTest {
     char *internal_name;
     ASSERT_FALSE(PK11_IsFIPS());
     internal_name = PR_smprintf("%s", SECMOD_GetInternalModule()->commonName);
-    ASSERT_EQ(SECSuccess, SECMOD_DeleteInternalModule(internal_name));
+    ASSERT_EQ(SECSuccess, SECMOD_DeleteInternalModule(internal_name))
+        << PORT_ErrorToName(PORT_GetError());
     PR_smprintf_free(internal_name);
     ASSERT_TRUE(PK11_IsFIPS());
   }
 };
 
+class SoftokenFipsDhTest : public SoftokenFipsTest {
+ protected:
+  SoftokenFipsDhTest() : SoftokenFipsTest("SoftokenFipsDhTest.d-") {}
+  time_t reference_time[CLASS_LAST] = {0};
+
+  virtual void SetUp() {
+    SoftokenFipsTest::SetUp();
+
+    ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+    ASSERT_TRUE(slot);
+
+    ASSERT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, ""));
+    ASSERT_EQ(SECSuccess, PK11_Authenticate(slot.get(), PR_FALSE, nullptr));
+
+    time_t time;
+    for (int i = CLASS_FIRST; i < CLASS_LAST; i++) {
+      PQGParams params;
+      params.prime.data = (unsigned char *)reference_prime[i];
+      params.prime.len = reference_prime_len[i];
+      params.base.data = (unsigned char *)g2;
+      params.base.len = sizeof(g2);
+      params.subPrime.data = nullptr;
+      params.subPrime.len = 0;
+      ASSERT_EQ(SECSuccess, test_dh_value(&params, nullptr, PR_FALSE, &time));
+      reference_time[i] = time / 2 + 3 * time;
+    }
+  };
+};
+
 const std::vector<std::string> kFipsPasswordCases[] = {
     // FIPS level1 -> level1 -> level1
     {"", "", ""},
@@ -613,12 +841,68 @@ TEST_P(SoftokenFipsBadPasswordTest, SetBadPassword) {
   }
 }
 
+class SoftokenFipsDhValidate
+    : public SoftokenFipsDhTest,
+      public ::testing::WithParamInterface<DhTestVector> {};
+
+/* test the DH validation process. In fips mode, primes with unknown
+ * subprimes, and all sorts of bad public keys should fail */
+TEST_P(SoftokenFipsDhValidate, DhVectors) {
+  const DhTestVector dhTestValues = GetParam();
+  std::string testId = (char *)(dhTestValues.id);
+  std::string err = "Test(" + testId + ") failed";
+  time_t time;
+  PRBool genFailOK = PR_FALSE;
+  SECStatus rv;
+
+  PQGParams params;
+  params.prime = dhTestValues.p;
+  params.base = dhTestValues.g;
+  params.subPrime = dhTestValues.q;
+  std::cerr << "Test:" + testId << std::endl
+            << "param_type: " << param_value(dhTestValues.param_type)
+            << ", key_class: " << key_value(dhTestValues.key_class) << std::endl
+            << "p: " << DataBuffer(dhTestValues.p.data, dhTestValues.p.len)
+            << std::endl
+            << "g: " << DataBuffer(dhTestValues.g.data, dhTestValues.g.len)
+            << std::endl
+            << "q: " << DataBuffer(dhTestValues.q.data, dhTestValues.q.len)
+            << std::endl
+            << "pub_key: "
+            << DataBuffer(dhTestValues.pub_key.data, dhTestValues.pub_key.len)
+            << std::endl;
+
+  if ((dhTestValues.param_type != TLS_APPROVED) &&
+      (dhTestValues.param_type != IKE_APPROVED)) {
+    genFailOK = PR_TRUE;
+  }
+  rv = test_dh_value(&params, &dhTestValues.pub_key, genFailOK, &time);
+
+  switch (dhTestValues.param_type) {
+    case TLS_APPROVED:
+    case IKE_APPROVED:
+      EXPECT_EQ(SECSuccess, rv) << err;
+      EXPECT_LE(time, reference_time[dhTestValues.key_class]) << err;
+      break;
+    case SAFE_PRIME:
+    case SAFE_PRIME_WITH_SUBPRIME:
+    case KNOWN_SUBPRIME:
+    case UNKNOWN_SUBPRIME:
+    case WRONG_SUBPRIME:
+    case BAD_PUB_KEY:
+      EXPECT_EQ(SECFailure, rv) << err;
+      break;
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(FipsPasswordCases, SoftokenFipsPasswordTest,
                         ::testing::ValuesIn(kFipsPasswordCases));
 
 INSTANTIATE_TEST_CASE_P(BadFipsPasswordCases, SoftokenFipsBadPasswordTest,
                         ::testing::ValuesIn(kFipsPasswordBadCases));
 
+INSTANTIATE_TEST_CASE_P(FipsDhCases, SoftokenFipsDhValidate,
+                        ::testing::ValuesIn(DH_TEST_VECTORS));
 #endif
 
 }  // namespace nss_test

gtests/softoken_gtest/softoken_gtest.gyp

@@ -16,6 +16,7 @@
       ],
       'dependencies': [
         '<(DEPTH)/exports.gyp:nss_exports',
+        '<(DEPTH)/cpputil/cpputil.gyp:cpputil',
         '<(DEPTH)/lib/util/util.gyp:nssutil3',
         '<(DEPTH)/gtests/google_test/google_test.gyp:gtest',
       ],

lib/freebl/blapi.h

@@ -380,6 +380,9 @@ extern SECStatus KEA_Derive(SECItem *prime,
  */
 extern PRBool KEA_Verify(SECItem *Y, SECItem *prime, SECItem *subPrime);
 
+/* verify a value is prime */
+PRBool KEA_PrimeCheck(SECItem *prime);
+
 /****************************************
  * J-PAKE key transport
  */