From dedea28c2ef59eb86f5d9704e5609ae13fa8b3c2 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 25 Nov 2015 10:30:58 -0500
Subject: [PATCH 1/5] Make crypto_seed_rng() and crypto_rand() less scary.

These functions must really never fail; so have crypto_rand() assert
that it's working okay, and have crypto_seed_rng() demand that
callers check its return value.  Also have crypto_seed_rng() check
RAND_status() before returning.
---
 src/common/compat.h       |  2 ++
 src/common/crypto.c       | 10 ++++++----
 src/common/crypto.h       |  3 ++-
 src/or/main.c             |  5 ++++-
 src/test/bench.c          |  5 ++++-
 src/test/test_workqueue.c |  5 ++++-
 src/test/testing_common.c |  5 ++++-
 7 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/common/compat.h b/src/common/compat.h
index c7c468c754..c3d6abd07c 100644
--- a/src/common/compat.h
+++ b/src/common/compat.h
@@ -118,6 +118,7 @@
 #define ATTR_CONST __attribute__((const))
 #define ATTR_MALLOC __attribute__((malloc))
 #define ATTR_NORETURN __attribute__((noreturn))
+#define ATTR_WUR __attribute__((warn_unused_result))
 /* Alas, nonnull is not at present a good idea for us.  We'd like to get
  * warnings when we pass NULL where we shouldn't (which nonnull does, albeit
  * spottily), but we don't want to tell the compiler to make optimizations
@@ -153,6 +154,7 @@
 #define ATTR_NORETURN
 #define ATTR_NONNULL(x)
 #define ATTR_UNUSED
+#define ATTR_WUR
 #define PREDICT_LIKELY(exp) (exp)
 #define PREDICT_UNLIKELY(exp) (exp)
 #endif
diff --git a/src/common/crypto.c b/src/common/crypto.c
index 815c2ec0c5..b7dc4b86af 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -2358,7 +2358,7 @@ crypto_seed_rng(void)
 
   memwipe(buf, 0, sizeof(buf));
 
-  if (rand_poll_ok || load_entropy_ok)
+  if ((rand_poll_ok || load_entropy_ok) && RAND_status() == 1)
     return 0;
   else
     return -1;
@@ -2380,12 +2380,14 @@ int
 crypto_rand_unmocked(char *to, size_t n)
 {
   int r;
+  if (n == 0)
+    return 0;
+
   tor_assert(n < INT_MAX);
   tor_assert(to);
   r = RAND_bytes((unsigned char*)to, (int)n);
-  if (r == 0)
-    crypto_log_errors(LOG_WARN, "generating random data");
-  return (r == 1) ? 0 : -1;
+  tor_assert(r >= 0);
+  return 0;
 }
 
 /** Return a pseudorandom integer, chosen uniformly from the values
diff --git a/src/common/crypto.h b/src/common/crypto.h
index 6256f7346b..d2ced63bd5 100644
--- a/src/common/crypto.h
+++ b/src/common/crypto.h
@@ -16,6 +16,7 @@
 #include <stdio.h>
 #include "torint.h"
 #include "testsupport.h"
+#include "compat.h"
 
 /*
   Macro to create an arbitrary OpenSSL version number as used by
@@ -258,7 +259,7 @@ int crypto_expand_key_material_rfc5869_sha256(
                                     uint8_t *key_out, size_t key_out_len);
 
 /* random numbers */
-int crypto_seed_rng(void);
+int crypto_seed_rng(void) ATTR_WUR;
 MOCK_DECL(int,crypto_rand,(char *to, size_t n));
 int crypto_rand_unmocked(char *to, size_t n);
 int crypto_strongest_rand(uint8_t *out, size_t out_len);
diff --git a/src/or/main.c b/src/or/main.c
index 9b3dbb5586..0f8d7ff3fa 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -1389,7 +1389,10 @@ run_scheduled_events(time_t now)
   if (time_to.add_entropy < now) {
     if (time_to.add_entropy) {
       /* We already seeded once, so don't die on failure. */
-      crypto_seed_rng();
+      if (crypto_seed_rng() < 0) {
+        log_warn(LD_GENERAL, "Tried to re-seed RNG, but failed. We already "
+                 "seeded once, though, so we won't exit here.");
+      }
     }
 /** How often do we add more entropy to OpenSSL's RNG pool? */
 #define ENTROPY_INTERVAL (60*60)
diff --git a/src/test/bench.c b/src/test/bench.c
index 2a27377c80..70ec025b7b 100644
--- a/src/test/bench.c
+++ b/src/test/bench.c
@@ -643,7 +643,10 @@ main(int argc, const char **argv)
 
   reset_perftime();
 
-  crypto_seed_rng();
+  if (crypto_seed_rng() < 0) {
+    printf("Couldn't seed RNG; exiting.\n");
+    return 1;
+  }
   crypto_init_siphash_key();
   options = options_new();
   init_logging(1);
diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c
index 0d79733cf0..6edfd313cb 100644
--- a/src/test/test_workqueue.c
+++ b/src/test/test_workqueue.c
@@ -391,7 +391,10 @@ main(int argc, char **argv)
   init_logging(1);
   network_init();
   crypto_global_init(1, NULL, NULL);
-  crypto_seed_rng();
+  if (crypto_seed_rng() < 0) {
+    printf("Couldn't seed RNG; exiting.\n");
+    return 1;
+  }
 
   rq = replyqueue_new(as_flags);
   tor_assert(rq);
diff --git a/src/test/testing_common.c b/src/test/testing_common.c
index 441024bd7d..2ea158fddd 100644
--- a/src/test/testing_common.c
+++ b/src/test/testing_common.c
@@ -272,7 +272,10 @@ main(int c, const char **v)
     return 1;
   }
   crypto_set_tls_dh_prime();
-  crypto_seed_rng();
+  if (crypto_seed_rng() < 0) {
+    printf("Couldn't seed RNG; exiting.\n");
+    return 1;
+  }
   rep_hist_init();
   network_init();
   setup_directory();

From 10fdee628552bca65ddb86b17e0b057628ad3703 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 25 Nov 2015 10:36:34 -0500
Subject: [PATCH 2/5] Add crypto-initializer functions to those whose return
 values must be checked

---
 src/common/crypto.c       | 3 ++-
 src/common/crypto.h       | 4 ++--
 src/test/test_workqueue.c | 5 ++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/common/crypto.c b/src/common/crypto.c
index b7dc4b86af..1ca86ea8f3 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -322,7 +322,8 @@ int
 crypto_global_init(int useAccel, const char *accelName, const char *accelDir)
 {
   if (!crypto_global_initialized_) {
-    crypto_early_init();
+    if (crypto_early_init() < 0)
+      return -1;
 
     crypto_global_initialized_ = 1;
 
diff --git a/src/common/crypto.h b/src/common/crypto.h
index d2ced63bd5..60f9e28902 100644
--- a/src/common/crypto.h
+++ b/src/common/crypto.h
@@ -112,10 +112,10 @@ typedef struct crypto_dh_t crypto_dh_t;
 /* global state */
 const char * crypto_openssl_get_version_str(void);
 const char * crypto_openssl_get_header_version_str(void);
-int crypto_early_init(void);
+int crypto_early_init(void) ATTR_WUR;
 int crypto_global_init(int hardwareAccel,
                        const char *accelName,
-                       const char *accelPath);
+                       const char *accelPath) ATTR_WUR;
 void crypto_thread_cleanup(void);
 int crypto_global_cleanup(void);
 
diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c
index 6edfd313cb..1202f80fa3 100644
--- a/src/test/test_workqueue.c
+++ b/src/test/test_workqueue.c
@@ -390,7 +390,10 @@ main(int argc, char **argv)
 
   init_logging(1);
   network_init();
-  crypto_global_init(1, NULL, NULL);
+  if (crypto_global_init(1, NULL, NULL) < 0) {
+    printf("Couldn't initialize crypto subsystem; exiting.\n");
+    return 1;
+  }
   if (crypto_seed_rng() < 0) {
     printf("Couldn't seed RNG; exiting.\n");
     return 1;

From ddcbe264745a0c10d80d8ad74125d23eb251662d Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 25 Nov 2015 10:42:00 -0500
Subject: [PATCH 3/5] Now that crypto_rand() cannot fail, it should return
 void.

---
 src/common/crypto.c            | 12 +++++-------
 src/common/crypto.h            |  4 ++--
 src/common/crypto_curve25519.c |  3 +--
 src/common/tortls.c            |  3 +--
 src/or/config.c                |  3 +--
 src/or/connection_or.c         |  3 +--
 src/or/control.c               |  3 +--
 src/or/ext_orport.c            |  3 +--
 src/or/onion_fast.c            |  8 ++------
 src/or/rendclient.c            |  6 +-----
 src/or/rendcommon.c            | 12 ++----------
 src/test/test_extorport.c      |  5 ++---
 12 files changed, 20 insertions(+), 45 deletions(-)

diff --git a/src/common/crypto.c b/src/common/crypto.c
index 1ca86ea8f3..9e27ad30c4 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -270,8 +270,7 @@ crypto_init_siphash_key(void)
   if (have_seeded_siphash)
     return 0;
 
-  if (crypto_rand((char*) &key, sizeof(key)) < 0)
-    return -1;
+  crypto_rand((char*) &key, sizeof(key));
   siphash_set_global_key(&key);
   have_seeded_siphash = 1;
   return 0;
@@ -2368,27 +2367,26 @@ crypto_seed_rng(void)
 /** Write <b>n</b> bytes of strong random data to <b>to</b>. Return 0 on
  * success, -1 on failure, with support for mocking for unit tests.
  */
-MOCK_IMPL(int,
+MOCK_IMPL(void,
 crypto_rand, (char *to, size_t n))
 {
-  return crypto_rand_unmocked(to, n);
+  crypto_rand_unmocked(to, n);
 }
 
 /** Write <b>n</b> bytes of strong random data to <b>to</b>. Return 0 on
  * success, -1 on failure.  Most callers will want crypto_rand instead.
  */
-int
+void
 crypto_rand_unmocked(char *to, size_t n)
 {
   int r;
   if (n == 0)
-    return 0;
+    return;
 
   tor_assert(n < INT_MAX);
   tor_assert(to);
   r = RAND_bytes((unsigned char*)to, (int)n);
   tor_assert(r >= 0);
-  return 0;
 }
 
 /** Return a pseudorandom integer, chosen uniformly from the values
diff --git a/src/common/crypto.h b/src/common/crypto.h
index 60f9e28902..3b471c2956 100644
--- a/src/common/crypto.h
+++ b/src/common/crypto.h
@@ -260,8 +260,8 @@ int crypto_expand_key_material_rfc5869_sha256(
 
 /* random numbers */
 int crypto_seed_rng(void) ATTR_WUR;
-MOCK_DECL(int,crypto_rand,(char *to, size_t n));
-int crypto_rand_unmocked(char *to, size_t n);
+MOCK_DECL(void,crypto_rand,(char *to, size_t n));
+void crypto_rand_unmocked(char *to, size_t n);
 int crypto_strongest_rand(uint8_t *out, size_t out_len);
 int crypto_rand_int(unsigned int max);
 int crypto_rand_int_range(unsigned int min, unsigned int max);
diff --git a/src/common/crypto_curve25519.c b/src/common/crypto_curve25519.c
index ac0b08a552..00302a2ff0 100644
--- a/src/common/crypto_curve25519.c
+++ b/src/common/crypto_curve25519.c
@@ -113,8 +113,7 @@ curve25519_rand_seckey_bytes(uint8_t *out, int extra_strong)
 {
   uint8_t k_tmp[CURVE25519_SECKEY_LEN];
 
-  if (crypto_rand((char*)out, CURVE25519_SECKEY_LEN) < 0)
-    return -1;
+  crypto_rand((char*)out, CURVE25519_SECKEY_LEN);
   if (extra_strong && !crypto_strongest_rand(k_tmp, CURVE25519_SECKEY_LEN)) {
     /* If they asked for extra-strong entropy and we have some, use it as an
      * HMAC key to improve not-so-good entropy rather than using it directly,
diff --git a/src/common/tortls.c b/src/common/tortls.c
index 536043e558..e3c6859828 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -601,8 +601,7 @@ tor_tls_create_certificate(crypto_pk_t *rsa,
     goto error;
 
   { /* our serial number is 8 random bytes. */
-    if (crypto_rand((char *)serial_tmp, sizeof(serial_tmp)) < 0)
-      goto error;
+    crypto_rand((char *)serial_tmp, sizeof(serial_tmp));
     if (!(serial_number = BN_bin2bn(serial_tmp, sizeof(serial_tmp), NULL)))
       goto error;
     if (!(BN_to_ASN1_INTEGER(serial_number, X509_get_serialNumber(x509))))
diff --git a/src/or/config.c b/src/or/config.c
index fa860af337..431d3664c5 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -7329,8 +7329,7 @@ init_cookie_authentication(const char *fname, const char *header,
 
   /* Generate the cookie */
   *cookie_out = tor_malloc(cookie_len);
-  if (crypto_rand((char *)*cookie_out, cookie_len) < 0)
-    goto done;
+  crypto_rand((char *)*cookie_out, cookie_len);
 
   /* Create the string that should be written on the file. */
   memcpy(cookie_file_str, header, strlen(header));
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index a967c93aca..c454d3f076 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -2290,8 +2290,7 @@ connection_or_send_auth_challenge_cell(or_connection_t *conn)
 
   auth_challenge_cell_t *ac = auth_challenge_cell_new();
 
-  if (crypto_rand((char*)ac->challenge, sizeof(ac->challenge)) < 0)
-    goto done;
+  crypto_rand((char*)ac->challenge, sizeof(ac->challenge));
 
   auth_challenge_cell_add_methods(ac, AUTHTYPE_RSA_SHA256_TLSSECRET);
   auth_challenge_cell_set_n_methods(ac,
diff --git a/src/or/control.c b/src/or/control.c
index 220e7e514f..c89fdde138 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -3418,8 +3418,7 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len,
     tor_free(client_nonce);
     return -1;
   }
-  const int fail = crypto_rand(server_nonce, SAFECOOKIE_SERVER_NONCE_LEN);
-  tor_assert(!fail);
+  crypto_rand(server_nonce, SAFECOOKIE_SERVER_NONCE_LEN);
 
   /* Now compute and send the server-to-controller response, and the
    * server's nonce. */
diff --git a/src/or/ext_orport.c b/src/or/ext_orport.c
index e8c8aa60a4..f159f7d0a6 100644
--- a/src/or/ext_orport.c
+++ b/src/or/ext_orport.c
@@ -193,8 +193,7 @@ handle_client_auth_nonce(const char *client_nonce, size_t client_nonce_len,
     return -1;
 
   /* Get our nonce */
-  if (crypto_rand(server_nonce, EXT_OR_PORT_AUTH_NONCE_LEN) < 0)
-    return -1;
+  crypto_rand(server_nonce, EXT_OR_PORT_AUTH_NONCE_LEN);
 
   { /* set up macs */
     size_t hmac_s_msg_len = strlen(EXT_OR_PORT_AUTH_SERVER_TO_CLIENT_CONST) +
diff --git a/src/or/onion_fast.c b/src/or/onion_fast.c
index 7584112570..22bef4eee0 100644
--- a/src/or/onion_fast.c
+++ b/src/or/onion_fast.c
@@ -30,10 +30,7 @@ fast_onionskin_create(fast_handshake_state_t **handshake_state_out,
 {
   fast_handshake_state_t *s;
   *handshake_state_out = s = tor_malloc(sizeof(fast_handshake_state_t));
-  if (crypto_rand((char*)s->state, sizeof(s->state)) < 0) {
-    tor_free(s);
-    return -1;
-  }
+  crypto_rand((char*)s->state, sizeof(s->state));
   memcpy(handshake_out, s->state, DIGEST_LEN);
   return 0;
 }
@@ -56,8 +53,7 @@ fast_server_handshake(const uint8_t *key_in, /* DIGEST_LEN bytes */
   size_t out_len;
   int r = -1;
 
-  if (crypto_rand((char*)handshake_reply_out, DIGEST_LEN)<0)
-    return -1;
+  crypto_rand((char*)handshake_reply_out, DIGEST_LEN);
 
   memcpy(tmp, key_in, DIGEST_LEN);
   memcpy(tmp+DIGEST_LEN, handshake_reply_out, DIGEST_LEN);
diff --git a/src/or/rendclient.c b/src/or/rendclient.c
index a39e518e99..11e940ce85 100644
--- a/src/or/rendclient.c
+++ b/src/or/rendclient.c
@@ -65,11 +65,7 @@ rend_client_send_establish_rendezvous(origin_circuit_t *circ)
   tor_assert(circ->rend_data);
   log_info(LD_REND, "Sending an ESTABLISH_RENDEZVOUS cell");
 
-  if (crypto_rand(circ->rend_data->rend_cookie, REND_COOKIE_LEN) < 0) {
-    log_warn(LD_BUG, "Internal error: Couldn't produce random cookie.");
-    circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL);
-    return -1;
-  }
+  crypto_rand(circ->rend_data->rend_cookie, REND_COOKIE_LEN);
 
   /* Set timestamp_dirty, because circuit_expire_building expects it,
    * and the rend cookie also means we've used the circ. */
diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c
index 22599e9830..1e6c6dabda 100644
--- a/src/or/rendcommon.c
+++ b/src/or/rendcommon.c
@@ -268,11 +268,7 @@ rend_encrypt_v2_intro_points_basic(char **encrypted_out,
   tor_assert(client_cookies && smartlist_len(client_cookies) > 0);
 
   /* Generate session key. */
-  if (crypto_rand(session_key, CIPHER_KEY_LEN) < 0) {
-    log_warn(LD_REND, "Unable to generate random session key to encrypt "
-                      "introduction point string.");
-    goto done;
-  }
+  crypto_rand(session_key, CIPHER_KEY_LEN);
 
   /* Determine length of encrypted introduction points including session
    * keys. */
@@ -334,11 +330,7 @@ rend_encrypt_v2_intro_points_basic(char **encrypted_out,
            REND_BASIC_AUTH_CLIENT_MULTIPLE;
        i < REND_BASIC_AUTH_CLIENT_MULTIPLE - 1; i++) {
     client_part = tor_malloc_zero(REND_BASIC_AUTH_CLIENT_ENTRY_LEN);
-    if (crypto_rand(client_part, REND_BASIC_AUTH_CLIENT_ENTRY_LEN) < 0) {
-      log_warn(LD_REND, "Unable to generate fake client entry.");
-      tor_free(client_part);
-      goto done;
-    }
+    crypto_rand(client_part, REND_BASIC_AUTH_CLIENT_ENTRY_LEN);
     smartlist_add(encrypted_session_keys, client_part);
   }
   /* Sort smartlist and put elements in result in order. */
diff --git a/src/test/test_extorport.c b/src/test/test_extorport.c
index 2e5a32eef3..5d38ed8fa2 100644
--- a/src/test/test_extorport.c
+++ b/src/test/test_extorport.c
@@ -309,15 +309,14 @@ test_ext_or_cookie_auth(void *arg)
   tor_free(client_hash2);
 }
 
-static int
+static void
 crypto_rand_return_tse_str(char *to, size_t n)
 {
   if (n != 32) {
     TT_FAIL(("Asked for %d bytes, not 32", (int)n));
-    return -1;
+    return;
   }
   memcpy(to, "te road There is always another ", 32);
-  return 0;
 }
 
 static void

From 1cfa2bc859c0a7f27b49b80dc1be4be2acc91ee8 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 25 Nov 2015 12:28:20 -0500
Subject: [PATCH 4/5] Fix documentation for crypto_rand*

---
 src/common/crypto.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/common/crypto.c b/src/common/crypto.c
index 9e27ad30c4..9669493a83 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -2364,8 +2364,11 @@ crypto_seed_rng(void)
     return -1;
 }
 
-/** Write <b>n</b> bytes of strong random data to <b>to</b>. Return 0 on
- * success, -1 on failure, with support for mocking for unit tests.
+/** Write <b>n</b> bytes of strong random data to <b>to</b>. Supports mocking
+ * for unit tests.
+ *
+ * This function is not allowed to fail; if it would fail to generate strong
+ * entropy, it must terminate the process instead.
  */
 MOCK_IMPL(void,
 crypto_rand, (char *to, size_t n))
@@ -2373,8 +2376,11 @@ crypto_rand, (char *to, size_t n))
   crypto_rand_unmocked(to, n);
 }
 
-/** Write <b>n</b> bytes of strong random data to <b>to</b>. Return 0 on
- * success, -1 on failure.  Most callers will want crypto_rand instead.
+/** Write <b>n</b> bytes of strong random data to <b>to</b>.  Most callers
+ * will want crypto_rand instead.
+ *
+ * This function is not allowed to fail; if it would fail to generate strong
+ * entropy, it must terminate the process instead.
  */
 void
 crypto_rand_unmocked(char *to, size_t n)

From 943369f927967268cacd2067ccae0bc5f1c5835e Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 25 Nov 2015 13:25:21 -0500
Subject: [PATCH 5/5] Add a changes file for bug 17686

---
 changes/bug17686 | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changes/bug17686

diff --git a/changes/bug17686 b/changes/bug17686
new file mode 100644
index 0000000000..8fa16c794b
--- /dev/null
+++ b/changes/bug17686
@@ -0,0 +1,4 @@
+  o Minor features:
+    - Adjust Tor's use of OpenSSL's RNG APIs so that they absolutely,
+      positively are not allowed to fail. Previously we depended on
+      internals about OpenSSL behavior. Closes ticket 17686.