[tor-commits] [tor/master] fast_rng: if noinherit has failed, then check getpid() for bad forks

[asn@xxxxxxxxxxxxxx]

commit 785c3f84de958e90c45e82d6126a8c66958be4e1
Author: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
Date:   Wed Mar 6 11:03:42 2019 -0500

    fast_rng: if noinherit has failed, then check getpid() for bad forks
    
    getpid() can be really expensive sometimes, and it can fail to
    detect some kind of fork+prng mistakes, so we need to avoid it if
    it's safe to do so.
    
    This patch might slow down fast_prng a lot on any old operating
    system that lacks a way to prevent ram from being inherited, AND
    requires a syscall for any getpid() calls.  But it should make sure
    that we either crash or continue safely on incorrect fork+prng usage
    elsewhere in the future.
---
 src/lib/crypt_ops/crypto_rand_fast.c | 63 ++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/lib/crypt_ops/crypto_rand_fast.c b/src/lib/crypt_ops/crypto_rand_fast.c
index f1acc9faf..384c172c3 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_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,
-                                NULL);
+                                &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_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_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_KEEP.
+     *
+     * If the result was INHERIT_DROP, then any attempt to access the rng
+     * memory after forking will crash.
+     *
+     * If the result was INHERIT_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) {



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits