Commit 600e046e authored by Nick Mathewson's avatar Nick Mathewson 🏃
Browse files

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.
parent 3b5d6ef1
......@@ -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;
}
......
......@@ -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;
......
......@@ -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);
......
......@@ -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);
......
......@@ -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;
}
......@@ -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},
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment