[tor-bugs] #17799 [Core Tor/Tor]: Hash All PRNG output before use

[Tor Bug Tracker & Wiki]

#17799: Hash All PRNG output before use
-------------------------------+----------------------------------------
 Reporter:  teor               |          Owner:  nickm
     Type:  defect             |         Status:  needs_revision
 Priority:  Medium             |      Milestone:  Tor: 0.2.9.x-final
Component:  Core Tor/Tor       |        Version:  Tor: unspecified
 Severity:  Normal             |     Resolution:
 Keywords:  TorCoreTeam201604  |  Actual Points:
Parent ID:                     |         Points:  small/medium-remaining
 Reviewer:  asn                |        Sponsor:
-------------------------------+----------------------------------------

Comment (by nickm):

 Replying to [comment:21 yawning]:
 > Casual review/thoughts:
 >
 >  * `REPLACE_OPENSSL_RAND` probably should be enabled by default once we
 actually deploy this, because this isn't any worse than OpenSSL's current
 RNG, and is likely better, with a compile time flag to not do this.

 Ack; let's add a new ticket.

 >  * (Abstraction) Should we have a generic ability to allocate
 `mlock()`ed pages useable by other places of code? (I think yes, because
 common rlimit configs allow for up to 64 KiB worth of locked pages, and we
 can keep long term keying material in such for example).

 Agreed; this should be a separate ticket.  It needs to be done carefully,
 though, so that there isn't just one address range in memory where all the
 sensitive stuff lives.

 >  * (Paranoia) The code should call `getrlimit()` to ensure that the
 `mlock()` has a chance of succeeding.  Failure in either case should be
 loud, hard, and always fatal on platforms where locked memory is
 supported.  A config option to allow using a non-mlocked() backing store
 perhaps.

 Hm. I think I'm okay with just tor_assert(r==0) on the mlock call.  We can
 give a nicer warning if anybody ever sees it; I suspect nobody will.

 >  * (Paranoia) Is there any reason (that isn't performance) to not hit up
 the syscall entropy source on supported platforms on each refill call?

 Not really.

 >  * (Paranoia) The `memset()` call in `shake_prng_getbytes_raw()` should
 be a `memwipe()`.  Since this routine is called with the mutex held,
 wiping the consumed portion of the buffer probably can be moved out of the
 while() loop to above the invariant check (Tiny optimization).

 I don't think that the compiler can eliminate that memset, but sure.  I've
 added a fixup commit.

 >  * (Bug) Child side of a `fork()` does not inherit `mlock()` status.
 Need to re-`mlock()` in the post fork handler.

 Huh, I didn't know that.  Added another fixup commit for that.

 >  * We should run this through dieharder/testU01/the NIST suite or
 similar, just to say we did.  Most CSPRNGs (even broken/horribad ones)
 will pass both tests, but it's better than nothing.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17799#comment:25>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online