[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #17799 [Core Tor/Tor]: Hash All PRNG output before use
#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@entry=3,
argv=argv@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@entry=-1,
what=what@entry=1, data=data@entry=0x5555559851e0 <periodic_events+160>)
at src/or/periodic.c:46
#4 0x00005555555a7861 in periodic_event_launch
(event=event@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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs