[tor-commits] [tor/master] Rename crypto_pk_check_key(), use it more reasonably, add tests
nickm at torproject.org
nickm at torproject.org
Wed Sep 5 00:47:14 UTC 2018
commit 600e046ed32f9826bcbdf223d99ed8af23a67504
Author: Nick Mathewson <nickm at torproject.org>
Date: Fri Aug 31 16:15:20 2018 -0400
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.
---
src/feature/relay/router.c | 2 +-
src/feature/rend/rendservice.c | 2 +-
src/lib/crypt_ops/crypto_rsa.h | 2 +-
src/lib/crypt_ops/crypto_rsa_nss.c | 15 +++++++---
src/lib/crypt_ops/crypto_rsa_openssl.c | 18 +++++++----
src/test/test_crypto.c | 55 ++++++++++++++++++++++++++++++++++
6 files changed, 82 insertions(+), 12 deletions(-)
diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c
index 7685760ac..55e8f1403 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 1a99bd56e..77775602b 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 aaf32ec1b..dd25bc31b 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 b6d8bb647..90e9c34d3 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 7ff71f6dd..f203a0683 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 04077b42f..b08c5cbc2 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},
More information about the tor-commits
mailing list