[tor-bugs] #17799 [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_review
 Priority:  Medium             |      Milestone:  Tor: 0.2.9.x-final
Component:  Tor                |        Version:  Tor: unspecified
 Severity:  Normal             |     Resolution:
 Keywords:  TorCoreTeam201604  |  Actual Points:
Parent ID:                     |         Points:  small/medium-remaining
 Reviewer:  asn                |        Sponsor:
-------------------------------+----------------------------------------

Comment (by 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.

  * (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).

  * (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.

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

  * (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).

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

  * 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:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online