[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:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  TorCoreTeam201605, TorCoreTeam-      |        Version:  Tor:
  postponed-201604, review-group-1               |  unspecified
Parent ID:                                       |     Resolution:
 Reviewer:  asn                                  |  Actual Points:
                                                 |         Points:  2
                                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Hello,

 I did another review of this branch (minus the threading code as suggested
 by comment:45). I didn't find anything major, but here are some comments:

 - I get the following double init warning message when I start tor with
 this branch:
 {{{
 [warn] crypto_init_shake_prng(): Bug: Attempt to double-initialize the
 RNG! (on Tor 0.2.8.0-alpha-dev 3413cff1c862248d)
 }}}
   Here are some backtraces:
 {{{
 (gdb) bt
 #0  crypto_init_shake_prng () at src/common/crypto_rng.c:300
 #1  0x00005555556a5ef3 in crypto_seed_rng () at src/common/crypto.c:2702
 #2  0x00005555556a6071 in crypto_early_init () at src/common/crypto.c:315
 #3  0x00005555556a6161 in crypto_early_init () at src/common/crypto.c:324
 #4  0x0000555555592858 in tor_init (argc=argc at entry=3,
 argv=argv at entry=0x7fffffffe4e8) at src/or/main.c:2912
 #5  0x0000555555593e05 in tor_main (argc=3, argv=0x7fffffffe4e8) at
 src/or/main.c:3576
 #6  0x000055555558dfe9 in main (argc=<optimized out>, argv=<optimized
 out>) at src/or/tor_main.c:30
 }}}
 {{{
 (gdb) bt
 #0  crypto_init_shake_prng () at src/common/crypto_rng.c:300
 #1  0x00005555556a5ef3 in crypto_seed_rng () at src/common/crypto.c:2702
 #2  0x000055555558f099 in add_entropy_callback (now=<optimized out>,
 options=<optimized out>) at src/or/main.c:1634
 #3  0x00005555555a75b8 in periodic_event_dispatch (fd=fd at entry=-1,
 what=what at entry=1, data=data at entry=0x5555559851e0 <periodic_events+160>)
 at src/or/periodic.c:46
 #4  0x00005555555a7861 in periodic_event_launch
 (event=event at entry=0x5555559851e0 <periodic_events+160>) at
 src/or/periodic.c:108
 #5  0x000055555558e97c in initialize_periodic_events_cb (fd=<optimized
 out>, events=<optimized out>, data=<optimized out>) at src/or/main.c:1351
 #6  0x00007ffff768a584 in ?? () from /usr/lib/x86_64-linux-
 gnu/libevent-2.0.so.5
 #7  0x00007ffff76883dc in event_base_loop () from /usr/lib/x86_64-linux-
 gnu/libevent-2.0.so.5
 #8  0x00005555555922fc in run_main_loop_once () at src/or/main.c:2510
 #9  run_main_loop_until_done () at src/or/main.c:2556
 #10 do_main_loop () at src/or/main.c:2482
 #11 0x0000555555595805 in tor_main (argc=<optimized out>, argv=<optimized
 out>) at src/or/main.c:3599
 }}}

 - I see this pattern everytime we call `shake_prng_getbytes_raw()` in
 `crypto_rng.c`:
 {{{
   tor_mutex_acquire(&prng_mutex);
   shake_prng_getbytes_raw(prng, buf, sizeof(buf));
   tor_mutex_release(&prng_mutex);
 }}}
   Maybe we can move the mutex acquisition to `shake_prng_getbytes_raw()`,
 instead of assuming that the caller will do it? Or maybe we can assert
 that the mutex is acquired inside that function?

 - I feel that the code can be almost unittestable with some mocking. For
 example, we could call `shake_prng_getbytes_raw()` and then make sure that
 the `shake_prng_t` elements/pointers are as expected. I think bugs like
 the one from comment:31 could be detected with a test like this. The
 integration tests are also very useful, so maybe unittests can be
 delayed?...

 Marking ticket as `needs_revision` since the first comment should be
 addressed.

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