commit 600e046ed32f9826bcbdf223d99ed8af23a67504 Author: Nick Mathewson nickm@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},