[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_review
 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 cypherpunks):

 Replying to [comment:35 cypherpunks]:
 > Edit: I removed a hunk where I think I may have spoken too early:
 `_postfork()` seems weird, the only explanation I can give to what you do
 with `the_prng` there is that you're assuming that, at that point, there's
 still a single thread (because of `fork()`), so you do similar to what you
 do in `_init()`. Same comments should apply in that regard.  However,
 you're still grabbing the lock, which is not needed if there's a single
 thread. But more importantly, locks and `fork()` do not mix well. I'm
 going to have to look at that again.

 I tried to reach a conclusive assessment in this regard, but I realised I
 don't understand the intended use-cases of this module in relation to
 forking and threading, so I cannot.  So instead I will just tell you what
 bothers me:

         1. The module is trying to be thread-safe.  I assume, then, that
 it's in fact going to be used in a multi-threaded environment.

         2. The module also anticipates the posiblity of forking the
 process *and* having *both* copies (parent and child) continue to run the
 same executable.  Otherwise, `_postfork`'s efforts would be for naught: if
 the child is going to exec or the parent is going to exit, there's no
 worry of duplicated PRNG state.

 So threading and forking, what could go wrong?  Well, fork(2) tells us:

         * The child process is created with a single thread — the one that
 called fork().  The entire virtual address space of the parent is
 replicated in the child, including the states of mutexes, condition
 variables, and other pthreads objects; the use of pthread_atfork(3) may be
 helpful for dealing with problems that this can cause.

 As it is, `_postfork()` could deadlock while trying to grab `prng_mutex`.
 This mutex is not exposed to other compilation units, so is not possible
 to prevent the problem "from the outside" (except by adding an external,
 redundant, layer of locking, which is silly).

 Unless it's implied that you use forking xor you use threading.  This is
 what I can't say, since I don't know the use-cases.  (And if this is the
 case, it should be enforced by the code: currently it isn't.)

 If instead forking and threading have to be contemplated in combination
 (particularly forking after having created threads) then the solution, for
 this module, would involve doing something like:

 {{{
 _init()
 {
         ...
         pthread_atfork(acquire_locks, release_locks, reinit_locks)
         ...
 }
 }}}

 But if you look at this for more than 3 seconds, you start to wonder:  Why
 is `_postfork` part of the API at all?  Why not simply pass it to
 `pthread_atfork`?  That way it even gets called automatically.

 {{{
 _init()
 {
         ...
         pthread_atfork(acquire_locks, release_locks, _child_postfork)
         ...
 }

 _child_postfork()
 {
         reinit locks
         relock mmapped region or remap
         reseed prng
         reset pid
         ...
 }
 }}}

 And if you look at `_child_postfork` for more than 2.46 seconds, you
 realise that it is doing almost the same as `_init`.  The canonical
 ADT/OOP behaviour here would be to simply destroy the previous object and
 create a new one.

 Still the issue of forking and threading is more complicated.  Above I
 said that would be the solution /for this module/.  The problem is that
 when you fork *every* address mapping is copied.  So this problem with
 locks in inconsistent/indeterminate state can come from any side.  Think
 about libraries: libssl? libevent? libc even?

 So, what ho?

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