diff --git a/changes/bug5170 b/changes/bug5170 new file mode 100644 index 0000000000..4e52c5ea6b --- /dev/null +++ b/changes/bug5170 @@ -0,0 +1,5 @@ + o Code simplification and refactoring: + - Remove contrib/id_to_fp.c since it wasn't used anywhere. + - Since OpenSSL 0.9.7 i2d_* functions support allocating output + buffer. Avoid calling twice: i2d_RSAPublicKey, i2d_DHparams, + i2d_X509, i2d_PublicKey. Fixes #5170. diff --git a/contrib/id_to_fp.c b/contrib/id_to_fp.c deleted file mode 100644 index 55b025dfaf..0000000000 --- a/contrib/id_to_fp.c +++ /dev/null @@ -1,77 +0,0 @@ -/* Copyright 2006 Nick Mathewson; see LICENSE for licensing information */ - -/* id_to_fp.c : Helper for directory authority ops. When somebody sends us - * a private key, this utility converts the private key into a fingerprint - * so you can de-list that fingerprint. - */ - -#include -#include -#include -#include - -#include -#include -#include - -#define die(s) do { fprintf(stderr, "%s\n", s); goto err; } while (0) - -int -main(int argc, char **argv) -{ - BIO *b = NULL; - RSA *key = NULL; - unsigned char *buf = NULL, *bufp; - int len, i; - unsigned char digest[20]; - int status = 1; - - if (argc < 2) { - fprintf(stderr, "Reading key from stdin...\n"); - if (!(b = BIO_new_fp(stdin, BIO_NOCLOSE))) - die("couldn't read from stdin"); - } else if (argc == 2) { - if (strcmp(argv[1], "-h") == 0 || - strcmp(argv[1], "--help") == 0) { - fprintf(stdout, "Usage: %s [keyfile]\n", argv[0]); - status = 0; - goto err; - } else { - if (!(b = BIO_new_file(argv[1], "r"))) - die("couldn't open file"); - } - } else { - fprintf(stderr, "Usage: %s [keyfile]\n", argv[0]); - goto err; - } - if (!(key = PEM_read_bio_RSAPrivateKey(b, NULL, NULL, NULL))) - die("couldn't parse key"); - - len = i2d_RSAPublicKey(key, NULL); - if (len < 0) - die("Bizarre key"); - bufp = buf = malloc(len+1); - if (!buf) - die("Out of memory"); - len = i2d_RSAPublicKey(key, &bufp); - if (len < 0) - die("Bizarre key"); - - SHA1(buf, len, digest); - for (i=0; i < 20; i += 2) { - printf("%02X%02X ", (int)digest[i], (int)digest[i+1]); - } - printf("\n"); - - status = 0; - -err: - if (buf) - free(buf); - if (key) - RSA_free(key); - if (b) - BIO_free(b); - return status; -} - diff --git a/src/common/crypto.c b/src/common/crypto.c index bda1ed0c33..adbf6396d0 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -1152,22 +1152,21 @@ int crypto_pk_asn1_encode(crypto_pk_t *pk, char *dest, size_t dest_len) { int len; - unsigned char *buf, *cp; - len = i2d_RSAPublicKey(pk->key, NULL); - if (len < 0 || (size_t)len > dest_len || dest_len > SIZE_T_CEILING) + unsigned char *buf = NULL; + + len = i2d_RSAPublicKey(pk->key, &buf); + if (len < 0 || buf == NULL) return -1; - cp = buf = tor_malloc(len+1); - len = i2d_RSAPublicKey(pk->key, &cp); - if (len < 0) { - crypto_log_errors(LOG_WARN,"encoding public key"); - tor_free(buf); + + if ((size_t)len > dest_len || dest_len > SIZE_T_CEILING) { + OPENSSL_free(buf); return -1; } /* We don't encode directly into 'dest', because that would be illegal * type-punning. (C99 is smarter than me, C99 is smarter than me...) */ memcpy(dest,buf,len); - tor_free(buf); + OPENSSL_free(buf); return len; } @@ -1198,24 +1197,17 @@ crypto_pk_asn1_decode(const char *str, size_t len) int crypto_pk_get_digest(crypto_pk_t *pk, char *digest_out) { - unsigned char *buf, *bufp; + unsigned char *buf = NULL; int len; - len = i2d_RSAPublicKey(pk->key, NULL); - if (len < 0) + len = i2d_RSAPublicKey(pk->key, &buf); + if (len < 0 || buf == NULL) return -1; - buf = bufp = tor_malloc(len+1); - len = i2d_RSAPublicKey(pk->key, &bufp); - if (len < 0) { - crypto_log_errors(LOG_WARN,"encoding public key"); - tor_free(buf); - return -1; - } if (crypto_digest(digest_out, (char*)buf, len) < 0) { - tor_free(buf); + OPENSSL_free(buf); return -1; } - tor_free(buf); + OPENSSL_free(buf); return 0; } @@ -1224,24 +1216,17 @@ crypto_pk_get_digest(crypto_pk_t *pk, char *digest_out) int crypto_pk_get_all_digests(crypto_pk_t *pk, digests_t *digests_out) { - unsigned char *buf, *bufp; + unsigned char *buf = NULL; int len; - len = i2d_RSAPublicKey(pk->key, NULL); - if (len < 0) + len = i2d_RSAPublicKey(pk->key, &buf); + if (len < 0 || buf == NULL) return -1; - buf = bufp = tor_malloc(len+1); - len = i2d_RSAPublicKey(pk->key, &bufp); - if (len < 0) { - crypto_log_errors(LOG_WARN,"encoding public key"); - tor_free(buf); - return -1; - } if (crypto_digest_all(digests_out, (char*)buf, len) < 0) { - tor_free(buf); + OPENSSL_free(buf); return -1; } - tor_free(buf); + OPENSSL_free(buf); return 0; } @@ -1703,7 +1688,7 @@ crypto_store_dynamic_dh_modulus(const char *fname) { int len, new_len; DH *dh = NULL; - unsigned char *dh_string_repr = NULL, *cp = NULL; + unsigned char *dh_string_repr = NULL; char *base64_encoded_dh = NULL; char *file_string = NULL; int retval = -1; @@ -1727,15 +1712,8 @@ crypto_store_dynamic_dh_modulus(const char *fname) if (!BN_set_word(dh->g, DH_GENERATOR)) goto done; - len = i2d_DHparams(dh, NULL); - if (len < 0) { - log_warn(LD_CRYPTO, "Error occured while DER encoding DH modulus (1)."); - goto done; - } - - cp = dh_string_repr = tor_malloc_zero(len+1); - len = i2d_DHparams(dh, &cp); - if ((len < 0) || ((cp - dh_string_repr) != len)) { + len = i2d_DHparams(dh, &dh_string_repr); + if ((len < 0) || (dh_string_repr == NULL)) { log_warn(LD_CRYPTO, "Error occured while DER encoding DH modulus (2)."); goto done; } @@ -1762,7 +1740,7 @@ crypto_store_dynamic_dh_modulus(const char *fname) done: if (dh) DH_free(dh); - tor_free(dh_string_repr); + OPENSSL_free(dh_string_repr); tor_free(base64_encoded_dh); tor_free(file_string); diff --git a/src/common/tortls.c b/src/common/tortls.c index b7e5bc1a5f..c0e36034d2 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -806,24 +806,24 @@ tor_cert_new(X509 *x509_cert) tor_cert_t *cert; EVP_PKEY *pkey; RSA *rsa; - int length, length2; - unsigned char *cp; + int length; + unsigned char *buf = NULL; if (!x509_cert) return NULL; - length = i2d_X509(x509_cert, NULL); + length = i2d_X509(x509_cert, &buf); cert = tor_malloc_zero(sizeof(tor_cert_t)); - if (length <= 0) { + if (length <= 0 || buf == NULL) { tor_free(cert); log_err(LD_CRYPTO, "Couldn't get length of encoded x509 certificate"); X509_free(x509_cert); return NULL; } cert->encoded_len = (size_t) length; - cp = cert->encoded = tor_malloc(length); - length2 = i2d_X509(x509_cert, &cp); - tor_assert(length2 == length); + cert->encoded = tor_malloc(length); + memcpy(cert->encoded, buf, length); + OPENSSL_free(buf); cert->cert = x509_cert; @@ -980,27 +980,25 @@ tor_tls_cert_get_key(tor_cert_t *cert) } /** Return true iff a and b represent the same public key. */ -static int -pkey_eq(EVP_PKEY *a, EVP_PKEY *b) +int +tor_tls_evp_pkey_eq(EVP_PKEY *a, EVP_PKEY *b) { /* We'd like to do this, but openssl 0.9.7 doesn't have it: return EVP_PKEY_cmp(a,b) == 1; */ - unsigned char *a_enc=NULL, *b_enc=NULL, *a_ptr, *b_ptr; - int a_len1, b_len1, a_len2, b_len2, result; - a_len1 = i2d_PublicKey(a, NULL); - b_len1 = i2d_PublicKey(b, NULL); - if (a_len1 != b_len1) - return 0; - a_ptr = a_enc = tor_malloc(a_len1); - b_ptr = b_enc = tor_malloc(b_len1); - a_len2 = i2d_PublicKey(a, &a_ptr); - b_len2 = i2d_PublicKey(b, &b_ptr); - tor_assert(a_len2 == a_len1); - tor_assert(b_len2 == b_len1); - result = tor_memeq(a_enc, b_enc, a_len1); - tor_free(a_enc); - tor_free(b_enc); + unsigned char *a_enc = NULL, *b_enc = NULL; + int a_len, b_len, result; + a_len = i2d_PublicKey(a, &a_enc); + b_len = i2d_PublicKey(b, &b_enc); + if (a_len != b_len || a_len < 0) { + result = 0; + } else { + result = tor_memeq(a_enc, b_enc, a_len); + } + if (a_enc) + OPENSSL_free(a_enc); + if (b_enc) + OPENSSL_free(b_enc); return result; } @@ -1019,7 +1017,7 @@ tor_tls_cert_matches_key(const tor_tls_t *tls, const tor_cert_t *cert) link_key = X509_get_pubkey(peercert); cert_key = X509_get_pubkey(cert->cert); - result = link_key && cert_key && pkey_eq(cert_key, link_key); + result = link_key && cert_key && tor_tls_evp_pkey_eq(cert_key, link_key); X509_free(peercert); if (link_key) diff --git a/src/common/tortls.h b/src/common/tortls.h index 49c488b365..c71ed573f2 100644 --- a/src/common/tortls.h +++ b/src/common/tortls.h @@ -138,5 +138,10 @@ int tor_tls_cert_is_valid(int severity, int check_rsa_1024); const char *tor_tls_get_ciphersuite_name(tor_tls_t *tls); +#ifdef TORTLS_PRIVATE +/* Prototypes for private functions only used by the unit tests. */ +int tor_tls_evp_pkey_eq(EVP_PKEY *a, EVP_PKEY *b); +#endif + #endif diff --git a/src/test/include.am b/src/test/include.am index 112d1a79d8..af95d44470 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -23,6 +23,7 @@ src_test_test_SOURCES = \ src/test/test_microdesc.c \ src/test/test_pt.c \ src/test/test_replay.c \ + src/test/test_tortls.c \ src/test/test_util.c \ src/test/test_config.c \ src/ext/tinytest.c diff --git a/src/test/test.c b/src/test/test.c index a9cf899a0e..da5b4e5256 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -2133,6 +2133,7 @@ extern struct testcase_t config_tests[]; extern struct testcase_t introduce_tests[]; extern struct testcase_t replaycache_tests[]; extern struct testcase_t cell_format_tests[]; +extern struct testcase_t tortls_tests[]; static struct testgroup_t testgroups[] = { { "", test_array }, @@ -2147,6 +2148,7 @@ static struct testgroup_t testgroups[] = { { "pt/", pt_tests }, { "config/", config_tests }, { "replaycache/", replaycache_tests }, + { "tortls/", tortls_tests }, { "introduce/", introduce_tests }, END_OF_GROUPS }; diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 839de7dd52..e45fbb8898 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -14,6 +14,10 @@ #include "crypto_curve25519.h" #endif +extern const char AUTHORITY_SIGNKEY_1[]; +extern const char AUTHORITY_SIGNKEY_1_DIGEST[]; +extern const char AUTHORITY_SIGNKEY_1_DIGEST256[]; + /** Run unit tests for Diffie-Hellman functionality. */ static void test_crypto_dh(void) @@ -505,6 +509,35 @@ test_crypto_pk(void) tor_free(encoded); } +/** Sanity check for crypto pk digests */ +static void +test_crypto_digests(void) +{ + crypto_pk_t *k = NULL; + ssize_t r; + digests_t pkey_digests; + char digest[DIGEST_LEN]; + + k = crypto_pk_new(); + test_assert(k); + r = crypto_pk_read_private_key_from_string(k, AUTHORITY_SIGNKEY_1, -1); + test_assert(!r); + + r = crypto_pk_get_digest(k, digest); + test_assert(r == 0); + test_memeq(hex_str(digest, DIGEST_LEN), + AUTHORITY_SIGNKEY_1_DIGEST, HEX_DIGEST_LEN); + + r = crypto_pk_get_all_digests(k, &pkey_digests); + + test_memeq(hex_str(pkey_digests.d[DIGEST_SHA1], DIGEST_LEN), + AUTHORITY_SIGNKEY_1_DIGEST, HEX_DIGEST_LEN); + test_memeq(hex_str(pkey_digests.d[DIGEST_SHA256], DIGEST256_LEN), + AUTHORITY_SIGNKEY_1_DIGEST256, HEX_DIGEST256_LEN); +done: + crypto_pk_free(k); +} + /** Run unit tests for misc crypto formatting functionality (base64, base32, * fingerprints, etc) */ static void @@ -1103,6 +1136,7 @@ struct testcase_t crypto_tests[] = { { "aes_EVP", test_crypto_aes, TT_FORK, &pass_data, (void*)"evp" }, CRYPTO_LEGACY(sha), CRYPTO_LEGACY(pk), + CRYPTO_LEGACY(digests), CRYPTO_LEGACY(dh), CRYPTO_LEGACY(s2k), { "aes_iv_AES", test_crypto_aes_iv, TT_FORK, &pass_data, (void*)"aes" }, diff --git a/src/test/test_data.c b/src/test/test_data.c index 5f0f7cba01..3c68b1294b 100644 --- a/src/test/test_data.c +++ b/src/test/test_data.c @@ -63,6 +63,11 @@ const char AUTHORITY_SIGNKEY_1[] = "Yx4lqK0ca5IkTp3HevwnlWaJgbaOTUspCVshzJBhDA==\n" "-----END RSA PRIVATE KEY-----\n"; +const char AUTHORITY_SIGNKEY_1_DIGEST[] = + "CBF56A83368A5150F1A9AAADAFB4D77F8C4170E2"; +const char AUTHORITY_SIGNKEY_1_DIGEST256[] = + "AF7C5468DBE3BA54A052726038D7F15F3C4CA511B1952645B3D96D83A8DFB51C"; + /** Second of 3 example authority certificates for unit testing. */ const char AUTHORITY_CERT_2[] = "dir-key-certificate-version 3\n" diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c new file mode 100644 index 0000000000..28ffbb1759 --- /dev/null +++ b/src/test/test_tortls.c @@ -0,0 +1,45 @@ +/* Copyright (c) 2013-2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include + +#include "orconfig.h" +#define CRYPTO_PRIVATE +#define TORTLS_PRIVATE +#include "or.h" +#include "test.h" + + +static void +test_tortls_evp_pkey_eq(void) +{ + crypto_pk_t *pk1 = NULL, *pk2 = NULL; + EVP_PKEY *evp1 = NULL, *evp2 = NULL; + + pk1 = pk_generate(0); + pk2 = pk_generate(1); + test_assert(pk1 && pk2); + + evp1 = crypto_pk_get_evp_pkey_(pk1, 0); + evp2 = crypto_pk_get_evp_pkey_(pk2, 0); + test_assert(evp1 && evp2); + + test_assert(tor_tls_evp_pkey_eq(evp1, evp2) == 0); + test_assert(tor_tls_evp_pkey_eq(evp1, evp1) == 1); + +done: + crypto_pk_free(pk1); + crypto_pk_free(pk2); + if (evp1) + EVP_PKEY_free(evp1); + if (evp2) + EVP_PKEY_free(evp2); +} + +#define TORTLS_LEGACY(name) \ + { #name, legacy_test_helper, 0, &legacy_setup, test_tortls_ ## name } + +struct testcase_t tortls_tests[] = { + TORTLS_LEGACY(evp_pkey_eq), + END_OF_TESTCASES +};