Merge branch 'tor-github/pr/761'

5 files changed, 140 insertions, 45 deletions

diff --git a/src/lib/crypt_ops/crypto_init.c b/src/lib/crypt_ops/crypto_init.c
index cf491f32d1..5c2780b2ca 100644
--- a/src/lib/crypt_ops/crypto_init.c
+++ b/src/lib/crypt_ops/crypto_init.c
@@ -152,6 +152,12 @@ crypto_prefork(void)
 #ifdef ENABLE_NSS
   crypto_nss_prefork();
 #endif
+  /* It is not safe to share a fast_rng object across a fork boundary unless
+   * we actually have zero-on-fork support in map_anon.c.  If we have
+   * drop-on-fork support, we will crash; if we have neither, we will yield
+   * a copy of the parent process's rng, which is scary and insecure.
+   */
+  destroy_thread_fast_rng();
 }
 
 /** Run operations that the crypto library requires to be happy again
diff --git a/src/lib/crypt_ops/crypto_rand_fast.c b/src/lib/crypt_ops/crypto_rand_fast.c
index 760e1025ed..01817c618f 100644
--- a/src/lib/crypt_ops/crypto_rand_fast.c
+++ b/src/lib/crypt_ops/crypto_rand_fast.c
@@ -46,8 +46,25 @@
 
 #include "lib/log/util_bug.h"
 
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+
 #include <string.h>
 
+#ifdef NOINHERIT_CAN_FAIL
+#define CHECK_PID
+#endif
+
+#ifdef CHECK_PID
+#define PID_FIELD_LEN sizeof(pid_t)
+#else
+#define PID_FIELD_LEN 0
+#endif
+
 /* Alias for CRYPTO_FAST_RNG_SEED_LEN to make our code shorter.
  */
 #define SEED_LEN (CRYPTO_FAST_RNG_SEED_LEN)
@@ -59,7 +76,7 @@
 /* The number of random bytes that we can yield to the user after each
  * time we fill a crypto_fast_rng_t's buffer.
  */
-#define BUFLEN (MAPLEN - 2*sizeof(uint16_t) - SEED_LEN)
+#define BUFLEN (MAPLEN - 2*sizeof(uint16_t) - SEED_LEN - PID_FIELD_LEN)
 
 /* The number of buffer refills after which we should fetch more
  * entropy from crypto_strongest_rand().
@@ -82,6 +99,11 @@ struct crypto_fast_rng_t {
   uint16_t n_till_reseed;
   /** How many bytes are remaining in cbuf.bytes? */
   uint16_t bytes_left;
+#ifdef CHECK_PID
+  /** Which process owns this fast_rng?  If this value is zero, we do not
+   * need to test the owner. */
+  pid_t owner;
+#endif
   struct cbuf {
     /** The seed (key and IV) that we will use the next time that we refill
      * cbuf. */
@@ -130,16 +152,32 @@ crypto_fast_rng_new(void)
 crypto_fast_rng_t *
 crypto_fast_rng_new_from_seed(const uint8_t *seed)
 {
+  unsigned inherit = INHERIT_RES_KEEP;
   /* We try to allocate this object as securely as we can, to avoid
    * having it get dumped, swapped, or shared after fork.
    */
   crypto_fast_rng_t *result = tor_mmap_anonymous(sizeof(*result),
-                                ANONMAP_PRIVATE | ANONMAP_NOINHERIT);
-
+                                ANONMAP_PRIVATE | ANONMAP_NOINHERIT,
+                                &inherit);
   memcpy(result->buf.seed, seed, SEED_LEN);
   /* Causes an immediate refill once the user asks for data. */
   result->bytes_left = 0;
   result->n_till_reseed = RESEED_AFTER;
+#ifdef CHECK_PID
+  if (inherit == INHERIT_RES_KEEP) {
+    /* This value will neither be dropped nor zeroed after fork, so we need to
+     * check our pid to make sure we are not sharing it across a fork.  This
+     * can be expensive if the pid value isn't cached, sadly.
+     */
+    result->owner = getpid();
+  }
+#elif defined(_WIN32)
+  /* Windows can't fork(), so there's no need to noinherit. */
+#else
+  /* We decided above that noinherit would always do _something_. Assert here
+   * that we were correct. */
+  tor_assert(inherit != INHERIT_RES_KEEP);
+#endif
   return result;
 }
 
@@ -211,6 +249,27 @@ static void
 crypto_fast_rng_getbytes_impl(crypto_fast_rng_t *rng, uint8_t *out,
                               const size_t n)
 {
+#ifdef CHECK_PID
+  if (rng->owner) {
+    /* Note that we only need to do this check when we have owner set: that
+     * is, when our attempt to block inheriting failed, and the result was
+     * INHERIT_RES_KEEP.
+     *
+     * If the result was INHERIT_RES_DROP, then any attempt to access the rng
+     * memory after forking will crash.
+     *
+     * If the result was INHERIT_RES_ZERO, then forking will set the bytes_left
+     * and n_till_reseed fields to zero.  This function will call
+     * crypto_fast_rng_refill(), which will in turn reseed the PRNG.
+     *
+     * So we only need to do this test in the case when mmap_anonymous()
+     * returned INHERIT_KEEP.  We avoid doing it needlessly, since getpid() is
+     * often a system call, and that can be slow.
+     */
+    tor_assert(rng->owner == getpid());
+  }
+#endif
+
   size_t bytes_to_yield = n;
 
   while (bytes_to_yield) {
diff --git a/src/lib/malloc/map_anon.c b/src/lib/malloc/map_anon.c
index 5dac5256a6..f4fda00bff 100644
--- a/src/lib/malloc/map_anon.c
+++ b/src/lib/malloc/map_anon.c
@@ -107,54 +107,34 @@ nodump_mem(void *mem, size_t sz)
 #endif
 }
 
-#ifdef TOR_UNIT_TESTS
-static unsigned last_anon_map_noinherit = ~0;
-/* Testing helper: return the outcome of the last call to noinherit_mem():
- * 0 if it did no good; 1 if it caused the memory not to be inherited, and
- * 2 if it caused the memory to be cleared on fork */
-unsigned
-get_last_anon_map_noinherit(void)
-{
-  return last_anon_map_noinherit;
-}
-static void
-set_last_anon_map_noinherit(unsigned f)
-{
-  last_anon_map_noinherit = f;
-}
-#else
-static void
-set_last_anon_map_noinherit(unsigned f)
-{
-  (void)f;
-}
-#endif
-
 /**
  * Helper: try to prevent the <b>sz</b> bytes at <b>mem</b> from being
  * accessible in child processes -- ideally by having them set to 0 after a
  * fork, and if that doesn't work, by having them unmapped after a fork.
  * Return 0 on success or if the facility is not available on this OS; return
  * -1 on failure.
+ *
+ * If we successfully make the memory uninheritable, adjust the value of
+ * *<b>inherit_result_out</b>.
  */
 static int
-noinherit_mem(void *mem, size_t sz)
+noinherit_mem(void *mem, size_t sz, inherit_res_t *inherit_result_out)
 {
-  set_last_anon_map_noinherit(0);
 #ifdef FLAG_ZERO
   int r = MINHERIT(mem, sz, FLAG_ZERO);
   if (r == 0) {
-    set_last_anon_map_noinherit(2);
+    *inherit_result_out = INHERIT_RES_ZERO;
     return 0;
   }
 #endif
 #ifdef FLAG_NOINHERIT
   int r2 = MINHERIT(mem, sz, FLAG_NOINHERIT);
   if (r2 == 0) {
-    set_last_anon_map_noinherit(1);
+    *inherit_result_out = INHERIT_RES_DROP;
   }
   return r2;
 #else
+  (void)inherit_result_out;
   (void)mem;
   (void)sz;
   return 0;
@@ -174,14 +154,25 @@ noinherit_mem(void *mem, size_t sz)
  * Memory returned from this function must be released with
  * tor_munmap_anonymous().
  *
+ * If <b>inherit_result_out</b> is non-NULL, set it to one of
+ * INHERIT_RES_KEEP, INHERIT_RES_DROP, or INHERIT_RES_ZERO, depending on the
+ * properties of the returned memory.
+ *
  * [Note: OS people use the word "anonymous" here to mean that the memory
  * isn't associated with any file. This has *nothing* to do with the kind of
  * anonymity that Tor is trying to provide.]
  */
 void *
-tor_mmap_anonymous(size_t sz, unsigned flags)
+tor_mmap_anonymous(size_t sz, unsigned flags,
+                   inherit_res_t *inherit_result_out)
 {
   void *ptr;
+  inherit_res_t itmp=0;
+  if (inherit_result_out == NULL) {
+    inherit_result_out = &itmp;
+  }
+  *inherit_result_out = INHERIT_RES_KEEP;
+
 #if defined(_WIN32)
   HANDLE mapping = CreateFileMapping(INVALID_HANDLE_VALUE,
                                      NULL, /*attributes*/
@@ -214,7 +205,7 @@ tor_mmap_anonymous(size_t sz, unsigned flags)
   }
 
   if (flags & ANONMAP_NOINHERIT) {
-    int noinherit_result = noinherit_mem(ptr, sz);
+    int noinherit_result = noinherit_mem(ptr, sz, inherit_result_out);
     raw_assert(noinherit_result == 0);
   }
 
diff --git a/src/lib/malloc/map_anon.h b/src/lib/malloc/map_anon.h
index 395145bd71..6c02cd6c16 100644
--- a/src/lib/malloc/map_anon.h
+++ b/src/lib/malloc/map_anon.h
@@ -31,11 +31,41 @@
  */
 #define ANONMAP_NOINHERIT (1u<<1)
 
-void *tor_mmap_anonymous(size_t sz, unsigned flags);
-void tor_munmap_anonymous(void *mapping, size_t sz);
+typedef enum {
+  /** Possible value for inherit_result_out: the memory will be kept
+   * by any child process. */
+  INHERIT_RES_KEEP=0,
+  /** Possible value for inherit_result_out: the memory will be dropped in the
+   * child process. Attempting to access it will likely cause a segfault. */
+  INHERIT_RES_DROP,
+  /** Possible value for inherit_result_out: the memory will be cleared in
+   * the child process. */
+  INHERIT_RES_ZERO
+} inherit_res_t;
 
-#ifdef TOR_UNIT_TESTS
-unsigned get_last_anon_map_noinherit(void);
+/* Here we define the NOINHERIT_CAN_FAIL macro if and only if
+ * it's possible that ANONMAP_NOINHERIT might yield inheritable memory.
+ */
+#ifdef _WIN32
+/* Windows can't fork, so NOINHERIT is never needed. */
+#elif defined(HAVE_MINHERIT)
+/* minherit() will always have a working MAP_INHERIT_NONE or MAP_INHERIT_ZERO.
+ * NOINHERIT should always work.
+ */
+#elif defined(HAVE_MADVISE)
+/* madvise() sometimes has neither MADV_DONTFORK and MADV_WIPEONFORK.
+ * We need to be ready for the possibility it failed.
+ *
+ * (Linux added DONTFORK in 2.6.16 and WIPEONFORK in 4.14. If we someday
+ * require 2.6.16 or later, we can assume that DONTFORK will work.)
+ */
+#define NOINHERIT_CAN_FAIL
+#else
+#define NOINHERIT_CAN_FAIL
 #endif
 
+void *tor_mmap_anonymous(size_t sz, unsigned flags,
+                         inherit_res_t *inherit_result_out);
+void tor_munmap_anonymous(void *mapping, size_t sz);
+
 #endif /* !defined(TOR_MAP_ANON_H) */
diff --git a/src/test/test_util.c b/src/test/test_util.c
index b4e702e1d7..da55fcac30 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -6130,10 +6130,12 @@ test_util_map_anon(void *arg)
   (void)arg;
   char *ptr = NULL;
   size_t sz = 16384;
+  unsigned inherit=0;
 
   /* Basic checks. */
-  ptr = tor_mmap_anonymous(sz, 0);
+  ptr = tor_mmap_anonymous(sz, 0, &inherit);
   tt_ptr_op(ptr, OP_NE, 0);
+  tt_int_op(inherit, OP_EQ, INHERIT_RES_KEEP);
   ptr[sz-1] = 3;
   tt_int_op(ptr[0], OP_EQ, 0);
   tt_int_op(ptr[sz-2], OP_EQ, 0);
@@ -6141,8 +6143,9 @@ test_util_map_anon(void *arg)
 
   /* Try again, with a private (non-swappable) mapping. */
   tor_munmap_anonymous(ptr, sz);
-  ptr = tor_mmap_anonymous(sz, ANONMAP_PRIVATE);
+  ptr = tor_mmap_anonymous(sz, ANONMAP_PRIVATE, &inherit);
   tt_ptr_op(ptr, OP_NE, 0);
+  tt_int_op(inherit, OP_EQ, INHERIT_RES_KEEP);
   ptr[sz-1] = 10;
   tt_int_op(ptr[0], OP_EQ, 0);
   tt_int_op(ptr[sz/2], OP_EQ, 0);
@@ -6150,7 +6153,7 @@ test_util_map_anon(void *arg)
 
   /* Now let's test a drop-on-fork mapping. */
   tor_munmap_anonymous(ptr, sz);
-  ptr = tor_mmap_anonymous(sz, ANONMAP_NOINHERIT);
+  ptr = tor_mmap_anonymous(sz, ANONMAP_NOINHERIT, &inherit);
   tt_ptr_op(ptr, OP_NE, 0);
   ptr[sz-1] = 10;
   tt_int_op(ptr[0], OP_EQ, 0);
@@ -6179,10 +6182,10 @@ test_util_map_anon_nofork(void *arg)
   char *ptr = NULL;
   size_t sz = 16384;
   int pipefd[2] = {-1, -1};
+  unsigned inherit=0;
 
   tor_munmap_anonymous(ptr, sz);
-  ptr = tor_mmap_anonymous(sz, ANONMAP_NOINHERIT);
-  int outcome = get_last_anon_map_noinherit();
+  ptr = tor_mmap_anonymous(sz, ANONMAP_NOINHERIT, &inherit);
   tt_ptr_op(ptr, OP_NE, 0);
   memset(ptr, 0xd0, sz);
 
@@ -6204,16 +6207,16 @@ test_util_map_anon_nofork(void *arg)
   char buf[1];
   ssize_t r = read(pipefd[0], buf, 1);
 
-  if (outcome == 2) {
+  if (inherit == INHERIT_RES_ZERO) {
     // We should be seeing clear-on-fork behavior.
     tt_int_op((int)r, OP_EQ, 1); // child should send us a byte.
     tt_int_op(buf[0], OP_EQ, 0); // that byte should be zero.
-  } else if (outcome == 1) {
+  } else if (inherit == INHERIT_RES_DROP) {
     // We should be seeing noinherit behavior.
     tt_int_op(r, OP_LE, 0); // child said nothing; it should have crashed.
   } else {
     // noinherit isn't implemented.
-    tt_int_op(outcome, OP_EQ, 0);
+    tt_int_op(inherit, OP_EQ, INHERIT_RES_KEEP);
     tt_int_op((int)r, OP_EQ, 1); // child should send us a byte.
     tt_int_op(buf[0], OP_EQ, 0xd0); // that byte should what we set it to.
   }
@@ -6221,11 +6224,17 @@ test_util_map_anon_nofork(void *arg)
   int ws;
   waitpid(child, &ws, 0);
 
-  if (outcome == 0) {
+#ifndef NOINHERIT_CAN_FAIL
+  /* Only if NOINHERIT_CAN_FAIL should it be possible for us to get
+   * INHERIT_KEEP behavior in this case. */
+  tt_int_op(inherit, OP_NE, INHERIT_RES_KEEP);
+#else
+  if (inherit == INHERIT_RES_KEEP) {
     /* Call this test "skipped", not "passed", since noinherit wasn't
      * implemented. */
     tt_skip();
   }
+#endif
 
  done:
   tor_munmap_anonymous(ptr, sz);