Rename crypto_pk_check_key(), use it more reasonably, add tests

This function was a wrapper around RSA_check_key() in openssl, which
checks for invalid RSA private keys (like those where p or q are
composite, or where d is not the inverse of e, or where n != p*q).
We don't need a function like this in NSS, since unlike OpenSSL, NSS
won't let you import a bogus private key.

I've renamed the function and changed its return type to make it
more reasonable, and added a unit test for trying to read a key
where n != p*q.

6 files changed, 82 insertions, 12 deletions

diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c
index 7685760ac6..55e8f1403e 100644
--- a/src/feature/relay/router.c
+++ b/src/feature/relay/router.c
@@ -590,7 +590,7 @@ init_key_from_file(const char *fname, int generate, int severity,
           tor_log(severity, LD_GENERAL,"Error generating onion key");
           goto error;
         }
-        if (crypto_pk_check_key(prkey) <= 0) {
+        if (! crypto_pk_is_valid_private_key(prkey)) {
           tor_log(severity, LD_GENERAL,"Generated key seems invalid");
           goto error;
         }
diff --git a/src/feature/rend/rendservice.c b/src/feature/rend/rendservice.c
index 1a99bd56ed..77775602b5 100644
--- a/src/feature/rend/rendservice.c
+++ b/src/feature/rend/rendservice.c
@@ -1629,7 +1629,7 @@ rend_service_load_auth_keys(rend_service_t *s, const char *hfname)
         crypto_pk_free(prkey);
         goto err;
       }
-      if (crypto_pk_check_key(prkey) <= 0) {
+      if (! crypto_pk_is_valid_private_key(prkey)) {
         log_warn(LD_BUG,"Generated client key seems invalid");
         crypto_pk_free(prkey);
         goto err;
diff --git a/src/lib/crypt_ops/crypto_rsa.h b/src/lib/crypt_ops/crypto_rsa.h
index aaf32ec1b0..dd25bc31bd 100644
--- a/src/lib/crypt_ops/crypto_rsa.h
+++ b/src/lib/crypt_ops/crypto_rsa.h
@@ -64,7 +64,7 @@ int crypto_pk_read_private_key_from_string(crypto_pk_t *env,
 int crypto_pk_write_private_key_to_filename(crypto_pk_t *env,
                                             const char *fname);
 
-int crypto_pk_check_key(crypto_pk_t *env);
+int crypto_pk_is_valid_private_key(crypto_pk_t *env);
 int crypto_pk_cmp_keys(const crypto_pk_t *a, const crypto_pk_t *b);
 int crypto_pk_eq_keys(const crypto_pk_t *a, const crypto_pk_t *b);
 size_t crypto_pk_keysize(const crypto_pk_t *env);
diff --git a/src/lib/crypt_ops/crypto_rsa_nss.c b/src/lib/crypt_ops/crypto_rsa_nss.c
index b6d8bb647d..90e9c34d30 100644
--- a/src/lib/crypt_ops/crypto_rsa_nss.c
+++ b/src/lib/crypt_ops/crypto_rsa_nss.c
@@ -244,12 +244,14 @@ crypto_pk_generate_key_with_bits,(crypto_pk_t *key, int bits))
   return result;
 }
 
-/** Return true iff <b>env</b> has a valid key.
+/** Return true iff <b>env</b> is a valid private key.
  */
 int
-crypto_pk_check_key(crypto_pk_t *key)
+crypto_pk_is_valid_private_key(crypto_pk_t *key)
 {
-  return key && key->pubkey;
+  /* We don't need to do validation here, since unlike OpenSSL, NSS won't let
+   * us load private keys without validating them. */
+  return key && key->seckey;
 }
 
 /** Return true iff <b>env</b> contains a public key whose public exponent
@@ -435,7 +437,7 @@ crypto_pk_public_encrypt(crypto_pk_t *env, char *to, size_t tolen,
   tor_assert(tolen < INT_MAX);
   tor_assert(fromlen < INT_MAX);
 
-  if (BUG(!crypto_pk_check_key(env)))
+  if (BUG(! env->pubkey))
     return -1;
 
   unsigned int result_len = 0;
@@ -724,6 +726,11 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
     crypto_nss_log_errors(LOG_WARN, "decoding an RSA private key");
   }
 
+  if (! crypto_pk_is_valid_private_key(output)) {
+    crypto_pk_free(output);
+    output = NULL;
+  }
+
   if (slot)
     PK11_FreeSlot(slot);
 
diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c
index 7ff71f6dd4..f203a06835 100644
--- a/src/lib/crypt_ops/crypto_rsa_openssl.c
+++ b/src/lib/crypt_ops/crypto_rsa_openssl.c
@@ -183,18 +183,21 @@ crypto_pk_generate_key_with_bits,(crypto_pk_t *env, int bits))
   return 0;
 }
 
-/** Return true iff <b>env</b> has a valid key.
+/** Return true if <b>env</b> has a valid key; false otherwise.
  */
 int
-crypto_pk_check_key(crypto_pk_t *env)
+crypto_pk_is_valid_private_key(crypto_pk_t *env)
 {
   int r;
   tor_assert(env);
 
   r = RSA_check_key(env->key);
-  if (r <= 0)
+  if (r <= 0) {
     crypto_openssl_log_errors(LOG_WARN,"checking RSA key");
-  return r;
+    return 0;
+  } else {
+    return 1;
+  }
 }
 
 /** Return true iff <b>env</b> contains a public key whose public exponent
@@ -578,5 +581,10 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
     crypto_openssl_log_errors(LOG_WARN,"decoding public key");
     return NULL;
   }
-  return crypto_new_pk_from_openssl_rsa_(rsa);
+  crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa);
+  if (! crypto_pk_is_valid_private_key(result)) {
+    crypto_pk_free(result);
+    return NULL;
+  }
+  return result;
 }
diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c
index 04077b42fb..b08c5cbc22 100644
--- a/src/test/test_crypto.c
+++ b/src/test/test_crypto.c
@@ -18,6 +18,7 @@
 #include "lib/crypt_ops/crypto_rand.h"
 #include "lib/crypt_ops/crypto_init.h"
 #include "ed25519_vectors.inc"
+#include "test/log_test_helpers.h"
 
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
@@ -1488,6 +1489,58 @@ test_crypto_pk_pem_encrypted(void *arg)
  done:
   crypto_pk_free(pk);
 }
+
+static void
+test_crypto_pk_invalid_private_key(void *arg)
+{
+  (void)arg;
+  /* Here is a simple invalid private key: it was produced by making
+   * a regular private key, and then adding 2 to the modulus. */
+  const char pem[] =
+    "-----BEGIN RSA PRIVATE KEY-----\n"
+    "MIIEpQIBAAKCAQEAskRyZrs+YAukvBmZlgo6/rCxyKF2xyUk073ap+2CgRUnSfGG\n"
+    "mflHlzqVq7tpH50DafpS+fFAbaEaNV/ac20QG0rUZi38HTB4qURWOu6n0Bws6E4l\n"
+    "UX/AkvDlWnuYH0pHHi2c3DGNFjwoJpjKuUTk+cRffVR8X3Kjr62SUDUaBNW0Kecz\n"
+    "3SYLbmgmZI16dFZ+g9sNM3znXZbhvb33WwPqpZSSPs37cPgF7eS6mAw/gUMx6zfE\n"
+    "HRmUnOQSzUdS05rvc/hsiCLhiIZ8hgfkD07XnTT1Ds8DwE55k7BUWY2wvwWCNLsH\n"
+    "qtqAxTr615XdkMxVkYgImpqPybarpfNYhFqkOwIDAQABAoIBACPC3VxEdbfYvhxJ\n"
+    "2mih9sG++nswAN7kUaX0cRe86rAwaShJPmJHApiQ1ROVTfpciiHJaLnhLraPWe2Z\n"
+    "I/6Bw3hmI4O399p3Lc1u+wlpdNqnvE6B1rSptx0DHE9xecvVH70rE0uM2Su7t6Y+\n"
+    "gnR2IKUGQs2mlCilm7aTUEWs0WJkkl4CG1dyxItuOSdNBjOEzXimJyiB10jEBFsp\n"
+    "SZeCF2FZ7AJbck5CVC42+oTsiDbZrHTHOn7v26rFGdONeHD1wOI1v7JwHFpCB923\n"
+    "aEHBzsPbMeq7DWG1rjzCYpcXHhTDBDBWSia4SEhyr2Nl7m7qxWWWwR+x4dqAj3rD\n"
+    "HeTmos0CgYEA6uf1CLpjPpOs5IaW1DQI8dJA/xFEAC/6GVgq4nFOGHZrm8G3L5o+\n"
+    "qvtQNMpDs2naWuZpqROFqv24o01DykHygR72GlPIY6uvmmf5tvJLoGnbFUay33L4\n"
+    "7b9dkNhuEIBNPzVDie0pgS77WgaPbYkVv5fnDwgPuVnkqfakEt7Pz2MCgYEAwkZ5\n"
+    "R1wLuTQEA2Poo6Gf4L8Bg6yNYI46LHDqDIs818iYLjtcnEEvbPfaoKNpOn7s7s4O\n"
+    "Pc+4HnT1aIQs0IKVLRTp+5a/9wfOkPZnobWOUHZk9UzBL3Hc1uy/qhp93iE3tSzx\n"
+    "v0O1pvR+hr3guTCZx8wZnDvaMgG3hlyPnVlHdrMCgYEAzQQxGbMC1ySv6quEjCP2\n"
+    "AogMbhE1lixJTUFj/EoDbNo9xKznIkauly/Lqqc1OysRhfA/G2+MY9YZBX1zwtyX\n"
+    "uBW7mPKynDrFgi9pBECnvJNmwET57Ic9ttIj6Tzbos83nAjyrzgr1zGX8dRz7ZeN\n"
+    "QbBj2vygLJbGOYinXkjUeh0CgYEAhN5aF9n2EqZmkEMGWtMxWy6HRJ0A3Cap1rcq\n"
+    "+4VHCXWhzwy+XAeg/e/N0MuyLlWcif7XcqLcE8h+BwtO8xQ8HmcNWApUJAls12wO\n"
+    "mGRpftJaXgIupdpD5aJpu1b++qrRRNIGTH9sf1D8L/8w8LcylZkbcuTkaAsQj45C\n"
+    "kqT64U0CgYEAq47IKS6xc3CDc17BqExR6t+1yRe+4ml+z1zcVbfUKony4pGvl1yo\n"
+    "rk0IYDN5Vd8h5xtXrkPdX9h+ywmohnelDKsayEuE+opyqEpSU4/96Bb22RZUoucb\n"
+    "LWkV5gZx5hFnDFtEd4vadMIiY4jVv/3JqiZDKwMVBJKlHRXJEEmIEBk=\n"
+    "-----END RSA PRIVATE KEY-----\n";
+
+  crypto_pk_t *pk = NULL;
+
+  pk = crypto_pk_new();
+  setup_capture_of_logs(LOG_WARN);
+  tt_int_op(-1, OP_EQ,
+            crypto_pk_read_private_key_from_string(pk, pem, strlen(pem)));
+#ifdef ENABLE_NSS
+  expect_single_log_msg_containing("received bad data");
+#else
+  expect_single_log_msg_containing("while checking RSA key");
+#endif
+ done:
+  teardown_capture_of_logs();
+  crypto_pk_free(pk);
+}
+
 #ifdef HAVE_TRUNCATE
 #define do_truncate truncate
 #else
@@ -3109,6 +3162,8 @@ struct testcase_t crypto_tests[] = {
   { "pk_fingerprints", test_crypto_pk_fingerprints, TT_FORK, NULL, NULL },
   { "pk_base64", test_crypto_pk_base64, TT_FORK, NULL, NULL },
   { "pk_pem_encrypted", test_crypto_pk_pem_encrypted, TT_FORK, NULL, NULL },
+  { "pk_invalid_private_key", test_crypto_pk_invalid_private_key, 0,
+    NULL, NULL },
   CRYPTO_LEGACY(digests),
   { "digest_names", test_crypto_digest_names, 0, NULL, NULL },
   { "sha3", test_crypto_sha3, TT_FORK, NULL, NULL},