bug5170.patch

diff --git a/changes/bug5170 b/changes/bug5170
new file mode 100644
index 0000000..4e52c5e
--- /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 55b025d..0000000
--- 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 <openssl/rsa.h>
-#include <openssl/bio.h>
-#include <openssl/sha.h>
-#include <openssl/pem.h>
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#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 bda1ed0..adbf639 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 b7e5bc1..c0e3603 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 <b>a</b> and <b>b</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 49c488b..c71ed57 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 112d1a7..af95d44 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 a9cf899..da5b4e5 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 839de7d..e45fbb8 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 5f0f7cb..3c68b12 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 0000000..28ffbb1
--- /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 <openssl/evp.h>
+
+#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
+};