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.

3 files changed, 25 insertions, 10 deletions

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;
 }