[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #16943 [Tor]: Implement prop250 (Random Number Generation During Tor Voting)



#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs