Merge branch 'maint-0.4.3'

changes/ticket33119

@@ -0,0 +1,8 @@
+  o Major bugfixes (security, denial-of-service):
+    - Fix a denial-of-service bug that could be used by anyone to consume a
+      bunch of CPU on any Tor relay or authority, or by directories to
+      consume a bunch of CPU on clients or hidden services. Because
+      of the potential for CPU consumption to introduce observable
+      timing patterns, we are treating this as a high-severity security
+      issue.  Fixes bug 33119; bugfix on 0.2.1.5-alpha. We are also tracking
+      this issue as TROVE-2020-002.

src/feature/dirparse/parsecommon.c

@@ -403,12 +403,19 @@ get_next_token(memarea_t *area,
   }
 
   if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a public key */
+    if (o_syn != NEED_KEY && o_syn != NEED_KEY_1024 && o_syn != OBJ_OK) {
+      RET_ERR("Unexpected public key.");
+    }
     tok->key = crypto_pk_asn1_decode(tok->object_body, tok->object_size);
     if (! tok->key)
       RET_ERR("Couldn't parse public key.");
   } else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private key */
+    if (o_syn != NEED_SKEY_1024 && o_syn != OBJ_OK) {
+      RET_ERR("Unexpected private key.");
+    }
     tok->key = crypto_pk_asn1_decode_private(tok->object_body,
-                                             tok->object_size);
+                                             tok->object_size,
+                                             1024);
     if (! tok->key)
       RET_ERR("Couldn't parse private key.");
   }

src/lib/crypt_ops/crypto_rsa.c

@@ -490,7 +490,7 @@ crypto_pk_write_private_key_to_string(crypto_pk_t *env,
 static int
 crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src,
                                    size_t len, int severity,
-                                   bool private_key)
+                                   bool private_key, int max_bits)
 {
   if (len == (size_t)-1) // "-1" indicates "use the length of the string."
     len = strlen(src);
@@ -510,7 +510,7 @@ crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src,
   }
 
   crypto_pk_t *pk = private_key
-    ? crypto_pk_asn1_decode_private((const char*)buf, n)
+    ? crypto_pk_asn1_decode_private((const char*)buf, n, max_bits)
     : crypto_pk_asn1_decode((const char*)buf, n);
   if (! pk) {
     log_fn(severity, LD_CRYPTO,
@@ -539,7 +539,8 @@ int
 crypto_pk_read_public_key_from_string(crypto_pk_t *env,
                                       const char *src, size_t len)
 {
-  return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, false);
+  return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, false,
+                                            -1);
 }
 
 /** Read a PEM-encoded private key from the <b>len</b>-byte string <b>src</b>
@@ -550,7 +551,21 @@ int
 crypto_pk_read_private_key_from_string(crypto_pk_t *env,
                                        const char *src, ssize_t len)
 {
-  return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true);
+  return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true,
+                                            -1);
+}
+
+/**
+ * As crypto_pk_read_private_key_from_string(), but reject any key
+ * with a modulus longer than 1024 bits before doing any expensive
+ * validation on it.
+ */
+int
+crypto_pk_read_private_key1024_from_string(crypto_pk_t *env,
+                                           const char *src, ssize_t len)
+{
+  return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true,
+                                            1024);
 }
 
 /** If a file is longer than this, we won't try to decode its private key */
@@ -578,7 +593,7 @@ crypto_pk_read_private_key_from_filename(crypto_pk_t *env,
   }
 
   int rv = crypto_pk_read_from_string_generic(env, buf, (ssize_t)st.st_size,
-                                              LOG_WARN, true);
+                                              LOG_WARN, true, -1);
   if (rv < 0) {
     log_warn(LD_CRYPTO, "Unable to decode private key from file %s",
              escaped(keyfile));
@@ -662,7 +677,7 @@ crypto_pk_base64_decode_private(const char *str, size_t len)
     goto out;
   }
 
-  pk = crypto_pk_asn1_decode_private(der, der_len);
+  pk = crypto_pk_asn1_decode_private(der, der_len, -1);
 
  out:
   memwipe(der, 0, len+1);

src/lib/crypt_ops/crypto_rsa.h

@@ -61,6 +61,8 @@ int crypto_pk_read_public_key_from_string(crypto_pk_t *env,
                                           const char *src, size_t len);
 int crypto_pk_read_private_key_from_string(crypto_pk_t *env,
                                            const char *s, ssize_t len);
+int crypto_pk_read_private_key1024_from_string(crypto_pk_t *env,
+                                               const char *src, ssize_t len);
 int crypto_pk_write_private_key_to_filename(crypto_pk_t *env,
                                             const char *fname);
 
@@ -95,7 +97,8 @@ int crypto_pk_asn1_encode(const crypto_pk_t *pk, char *dest, size_t dest_len);
 crypto_pk_t *crypto_pk_asn1_decode(const char *str, size_t len);
 int crypto_pk_asn1_encode_private(const crypto_pk_t *pk,
                                   char *dest, size_t dest_len);
-crypto_pk_t *crypto_pk_asn1_decode_private(const char *str, size_t len);
+crypto_pk_t *crypto_pk_asn1_decode_private(const char *str, size_t len,
+                                           int max_bits);
 int crypto_pk_get_fingerprint(crypto_pk_t *pk, char *fp_out,int add_space);
 int crypto_pk_get_hashed_fingerprint(crypto_pk_t *pk, char *fp_out);
 void crypto_add_spaces_to_fp(char *out, size_t outlen, const char *in);

src/lib/crypt_ops/crypto_rsa_nss.c

@@ -679,9 +679,12 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk,
 /** Given a buffer containing the DER representation of the
  * private key <b>str</b>, decode and return the result on success, or NULL
  * on failure.
+ *
+ * If <b>max_bits</b> is nonnegative, reject any key longer than max_bits
+ * without performing any expensive validation on it.
  */
 crypto_pk_t *
-crypto_pk_asn1_decode_private(const char *str, size_t len)
+crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits)
 {
   tor_assert(str);
   tor_assert(len < INT_MAX);
@@ -731,6 +734,15 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
     output = NULL;
   }
 
+  if (output) {
+    const int bits = SECKEY_PublicKeyStrengthInBits(output->pubkey);
+    if (max_bits >= 0 && bits > max_bits) {
+      log_info(LD_CRYPTO, "Private key longer than expected.");
+      crypto_pk_free(output);
+      output = NULL;
+    }
+  }
+
   if (slot)
     PK11_FreeSlot(slot);
 

src/lib/crypt_ops/crypto_rsa_openssl.c

@@ -33,6 +33,7 @@ ENABLE_GCC_WARNING("-Wredundant-decls")
 #include "lib/encoding/binascii.h"
 
 #include <string.h>
+#include <stdbool.h>
 
 /** Declaration for crypto_pk_t structure. */
 struct crypto_pk_t
@@ -564,11 +565,64 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest,
   return len;
 }
 
+/** Check whether any component of a private key is too large in a way that
+ * seems likely to make verification too expensive. Return true if it's too
+ * long, and false otherwise. */
+static bool
+rsa_private_key_too_long(RSA *rsa, int max_bits)
+{
+  const BIGNUM *n, *e, *p, *q, *d, *dmp1, *dmq1, *iqmp;
+#ifdef OPENSSL_1_1_API
+  n = RSA_get0_n(rsa);
+  e = RSA_get0_e(rsa);
+  p = RSA_get0_p(rsa);
+  q = RSA_get0_q(rsa);
+  d = RSA_get0_d(rsa);
+  dmp1 = RSA_get0_dmp1(rsa);
+  dmq1 = RSA_get0_dmq1(rsa);
+  iqmp = RSA_get0_iqmp(rsa);
+
+  if (RSA_bits(rsa) > max_bits)
+    return true;
+#else
+  n = rsa->n;
+  e = rsa->e;
+  p = rsa->p;
+  q = rsa->q;
+  d = rsa->d;
+  dmp1 = rsa->dmp1;
+  dmq1 = rsa->dmq1;
+  iqmp = rsa->iqmp;
+#endif
+
+  if (n && BN_num_bits(n) > max_bits)
+    return true;
+  if (e && BN_num_bits(e) > max_bits)
+    return true;
+  if (p && BN_num_bits(p) > max_bits)
+    return true;
+  if (q && BN_num_bits(q) > max_bits)
+    return true;
+  if (d && BN_num_bits(d) > max_bits)
+    return true;
+  if (dmp1 && BN_num_bits(dmp1) > max_bits)
+    return true;
+  if (dmq1 && BN_num_bits(dmq1) > max_bits)
+    return true;
+  if (iqmp && BN_num_bits(iqmp) > max_bits)
+    return true;
+
+  return false;
+}
+
 /** Decode an ASN.1-encoded private key from <b>str</b>; return the result on
  * success and NULL on failure.
+ *
+ * If <b>max_bits</b> is nonnegative, reject any key longer than max_bits
+ * without performing any expensive validation on it.
  */
 crypto_pk_t *
-crypto_pk_asn1_decode_private(const char *str, size_t len)
+crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits)
 {
   RSA *rsa;
   unsigned char *buf;
@@ -578,7 +632,12 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
   rsa = d2i_RSAPrivateKey(NULL, &cp, len);
   tor_free(buf);
   if (!rsa) {
-    crypto_openssl_log_errors(LOG_WARN,"decoding public key");
+    crypto_openssl_log_errors(LOG_WARN,"decoding private key");
+    return NULL;
+  }
+  if (max_bits >= 0 && rsa_private_key_too_long(rsa, max_bits)) {
+    log_info(LD_CRYPTO, "Private key longer than expected.");
+    RSA_free(rsa);
     return NULL;
   }
   crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa);

src/test/test_crypto.c

@@ -1335,6 +1335,44 @@ test_crypto_pk_pem_encrypted(void *arg)
   crypto_pk_free(pk);
 }
 
+static void
+test_crypto_pk_bad_size(void *arg)
+{
+  (void)arg;
+  crypto_pk_t *pk1 = pk_generate(0);
+  crypto_pk_t *pk2 = NULL;
+  char buf[2048];
+  int n = crypto_pk_asn1_encode_private(pk1, buf, sizeof(buf));
+  tt_int_op(n, OP_GT, 0);
+
+  /* Set the max bit count smaller: we should refuse to decode the key.*/
+  pk2 = crypto_pk_asn1_decode_private(buf, n, 1020);
+  tt_assert(! pk2);
+
+  /* Set the max bit count one bit smaller: we should refuse to decode the
+     key.*/
+  pk2 = crypto_pk_asn1_decode_private(buf, n, 1023);
+  tt_assert(! pk2);
+
+  /* Correct size: should work. */
+  pk2 = crypto_pk_asn1_decode_private(buf, n, 1024);
+  tt_assert(pk2);
+  crypto_pk_free(pk2);
+
+  /* One bit larger: should work. */
+  pk2 = crypto_pk_asn1_decode_private(buf, n, 1025);
+  tt_assert(pk2);
+  crypto_pk_free(pk2);
+
+  /* Set the max bit count larger: it should decode fine. */
+  pk2 = crypto_pk_asn1_decode_private(buf, n, 2048);
+  tt_assert(pk2);
+
+ done:
+  crypto_pk_free(pk1);
+  crypto_pk_free(pk2);
+}
+
 static void
 test_crypto_pk_invalid_private_key(void *arg)
 {
@@ -3000,6 +3038,7 @@ 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_bad_size", test_crypto_pk_bad_size, 0, NULL, NULL },
   { "pk_invalid_private_key", test_crypto_pk_invalid_private_key, 0,
     NULL, NULL },
   CRYPTO_LEGACY(digests),