[tor-bugs] #16943 [Tor]: Implement prop250 (Random Number Generation During Tor Voting)
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Feb 16 18:34:37 UTC 2016
#16943: Implement prop250 (Random Number Generation During Tor Voting)
-------------------------+------------------------------------
Reporter: asn | Owner:
Type: enhancement | Status: needs_review
Priority: High | Milestone: Tor: 0.2.8.x-final
Component: Tor | Version:
Severity: Normal | Resolution:
Keywords: tor-hs | Actual Points:
Parent ID: #8244 | Points: large
Sponsor: SponsorR |
-------------------------+------------------------------------
Changes (by dgoulet):
* status: needs_revision => needs_review
Comment:
Replying to [comment:27 teor]:
> This is my 3rd comment in a row. Please read all 3 comments.
>
Ok, FYI I've autosquash the fixup from last comment. It took me a while to
have
it done cleanly but now it's done. You have a series of fixup now for this
comment.
See branch: `prop250_final_v5`
> Reviewing 3406980ed700cc7fcd95b9b3774256493805057b:
>
> Nitpick: why add a phase_str for "unknown" when you assert rather than
use it?
`get_phase_str()` uses the enum value as the index in the `phase_str`
array.
"unknown" is there as a taint if we ever use an uninit phase value.
>
> commit_log() should probably use safe_str() on the reveal value, as it
is sensitive.
> Other debug messages also have this issue.
I've refactor that a bit better to print the encoded value instead so we
can
match them to strings in vote or other logs that do print the encoded
part.
>
> It also logs commits and reveals in hex, but votes use base64. This will
make them difficult to compare. generate_srv() also does this with the
SRV.
Fixed (for both this and the `commit_log`)
>
> I see you fixed the commit_decode() function comment to `base64-encode(
TIMESTAMP || H(REVEAL) )`.
>
> reveal_encode() and commit_encode() are duplicate code, with similar
code drift issues ti the *decode() functions. See comment 25 for a
suggestion on this.
I've discussed this with asn and after coding two different ways of
achieving
this, we don't feel super comfortable for two reasons. First is that both
options don't make it very clean tbh. Second, reveal and commit values are
two
different logical object and they just happen to have the same encoding
format
so we are fine leaving them like that since it's little code duplication
but
also simply different things...
>
> sr_generate_our_commit() can use crypto_strongest_rand() rather than
doing the hash itself.
> It can also use a const pointer.
Fixed! (3 different commits to be squashed on different master commit)
>
> sr_compute_srv() places a hard limit of 254 (SR-voting) authorities in a
tor network.
> {{{
> tor_assert(reveal_num < UINT8_MAX);
> }}}
> Is this OK?
> It should be documented somewhere.
This is indeed a limitation. Proposal 250 does force INT_1(REVEAL_NUM).
I've
pushed an update to torspec mentionning the limitation.
>
> In sr_generate_our_commit() and sr_compute_srv(), crypto_digest256 never
returns < 0. It returns 1 on failure.
> In generate_srv(), you don't check the return value of crypto_digest256.
> You might want to check how you handle the return values of all your
crypto_* calls.
Good catch! Fixed! (two fixup commits)
>
> In sr_compute_srv, should we assert on crypto_digest256 failure? Or
should we do something with the state?
Hrm... assert sounds a bit scary to just kill an authority. Most of tor
code
just ignores the return value. So I'm guessing having an empty SRV is
valid
here in case of an epic error like that and the authority should recover
after
a while.
>
> 47ade62b1811c16c28d73ebe59f47daa16cd4522:
>
> AuthDirSharedRandomness needs a tor manual page entry.
Fixed in b37214db72d42af8b25f7f87a5d3c6894c032115.
>
> There are missed opportunities for const pointers in this commit.
All fixed (hopefully).
>
> srv_to_ns_string() and get_majority_srv_from_votes() also encode the SRV
as hex rather than base64.
Fixed.
>
> get_majority_srv_from_votes() could be significantly simplified by
adding to container.c:
> {{{
> /** Return the most frequent member of the sorted list of DIGEST256_LEN
> * digests in <b>sl</b>. If count_out is non-NULL, set it to the count
> * of the most frequent member. */
> const uint8_t *
> smartlist_get_most_frequent_digest256_(smartlist_t *sl, int *count_out)
> {
> return smartlist_get_most_frequent_(sl, compare_digests256_,
count_out);
> }
> }}}
>
> Then using:
> {{{
> int count = 0;
> value = smartlist_get_most_frequent_digest256_(sr_digests, &count);
> /* Was this SRV voted by enough auths for us to keep it? */
> if (!should_keep_srv(count)) {
> goto end;
> }
> }}}
Indeed. I've created `smartlist_get_most_frequent_digest256_` and changed
around that function. Fixed.
>
> Nitpick: get_majority_srv_from_votes() uses `unsigned int current` for a
boolean value. Tor typically uses int for boolean values.
Should we really continue that "tradition"? :)
>
> 8e180aa061f0ea39968f05c85796b26f10a81744:
>
> extract_shared_random_commits() and sr_state_copy_reveal_info() and
should_keep_commit() and possibly other functions could use const
pointers.
Fixed.
>
> Why pick the microdesc consensus to update the state? Any particular
reason?
> Should we fall back to the NS consensus if there's no microdesc
consensus or SR values?
> {{{
> /* A new consensus has been created: pass it to the shared random
subsystem
> to update the SR state. */
> sr_act_post_consensus(pending[FLAV_MICRODESC].consensus);
> }}}
It's the consensus that client uses so we use that consensus to set our
SRV
value post consensus thus having the same view as client. Does that make
sense?
You think we should use `FLAV_NS`?
>
> extract_shared_random_commits() does `tor_assert(tok->n_args >= 3);` on
untrusted input. That lets a single authority crash all the other
authorities by including a line with only 2 arguments. Let's skip bad
commits instead.
>
> Let's also make sure we don't assert() on untrusted input in other
parsing functions.
Indeed, very bad. Fixed!
>
> extract_shared_random_srvs() can error out on the current SRV, and leave
the previous SRV allocated and initialised. What happens then?
`sr_info` in a the networkstatus_t structure is allocated with the ns so
on
error, the parsing stops and the object dissapear on its own.
{{{
if (extract_shared_random_srvs(ns, tokens) < 0) {
log_warn(LD_DIR, "SR: Unable to parse SRV(s)");
goto err;
}
}}}
>
> verify_commit_and_reveal() also expects crypto_digest256() to return -1.
It never will.
Fixed.
>
> In should_keep_commit(), we warn using LD_BUG when another authority
gets the protocol wrong. This could be misleading, and leave authority
operators thinking they have a tor version with a bug. (Or do we do this
elsewhere?)
True, should be a `LD_DIR` here. Fixed
> src/test/sr_srv_calc_ref.py looks like it uses SHA256, and not SHA3-256.
Yah that python one is out of date and SHA3 is not in python hashlib as
far as
I know so fix to that thing is pending until we can do it in python...
>
> I get the following crashes when I run the unit tests on OS X 10.11.3,
Apple LLVM version 7.0.2 (clang-700.1.81), x86_64.
>
> Five times:
> {{{
> shared-random/utils: [forking] Feb 10 16:13:46.122 [err]
tor_assertion_failed_: Bug: src/or/shared-random.c:854:
sr_generate_our_commit: Assertion my_rsa_cert failed; aborting. (on Tor
0.2.8.1-alpha-dev )
> }}}
>
> {{{
> 2 libsystem_c.dylib 0x00007fff975b06e7 abort + 129
> 3 test 0x0000000101a8af29
sr_generate_our_commit + 825 (shared-random.c:854)
> 4 test 0x0000000101a8cf11 sr_state_update
+ 177 (shared-random-state.c:1056)
> 5 test 0x0000000101a8d56d sr_state_init +
349 (shared-random-state.c:1231)
> 6 test 0x00000001018e43c3
test_a_networkstatus + 339 (test_dir.c:1800)
> 7 test 0x00000001019dfadd
testcase_run_one + 845 (tinytest.c:106)
> 8 test 0x00000001019dffd3 tinytest_main +
499 (tinytest.c:432)
> 9 test 0x00000001019df43f main + 639
(testing_common.c:298)
> }}}
I think it's fixed. Since I can't reproduce, I hope you can confirm.
>
> One time:
> {{{
> shared-random/state_transition: [forking] Feb 10 16:13:46.389 [err]
tor_assertion_failed_: Bug: src/or/shared-random-state.c:210:
commit_add_to_state: Assertion saved_commit == NULL failed; aborting. (on
Tor 0.2.8.1-alpha-dev )
> }}}
>
> {{{
> 2 libsystem_c.dylib 0x00007fff975b06e7 abort + 129
> 3 test 0x0000000101a8e00f
commit_add_to_state + 175 (shared-random-state.c:210)
> 4 test 0x0000000101a8c970
sr_state_add_commit + 48 (shared-random-state.c:941)
> 5 test 0x0000000101982dfe
test_state_transition + 574 (test_shared_random.c:976)
> 6 test 0x00000001019dfadd
testcase_run_one + 845 (tinytest.c:106)
> 7 test 0x00000001019dffd3 tinytest_main +
499 (tinytest.c:432)
> 8 test 0x00000001019df43f main + 639
(testing_common.c:298)
> }}}
This one is a bit more confusing to me. I don't see any possible way how
that
assert can be triggered apart from _already_ having a commit in our state
when
adding it. You'll have to tell me if you hit it again after the fixes and
probably we'll have to dig deeper.
>
> And I also see `12/720 TESTS FAILED. (1 skipped)`.
[snip]
>
> I'd still like to run the SRV code in chutney, is there a chutney
template for that?
> (I think I'll need to wait for the unit tests to be fixed first.)
You can with that branch. We'll simply use the default values for the
super
majority. grep for prefix "SR:" logs and see sr-state file.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16943#comment:31>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list