[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     |
-------------------------+------------------------------------

Comment (by teor):

 This is my 3rd comment in a row. Please read all 3 comments.

 Reviewing 3406980ed700cc7fcd95b9b3774256493805057b:

 Nitpick: why add a phase_str for "unknown" when you assert rather than use
 it?

 commit_log() should probably use safe_str() on the reveal value, as it is
 sensitive.
 Other debug messages also have this issue.

 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.

 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.

 sr_generate_our_commit() can use crypto_strongest_rand() rather than doing
 the hash itself.
 It can also use a const pointer.

 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.

 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.

 In sr_compute_srv, should we assert on crypto_digest256 failure? Or should
 we do something with the state?

 47ade62b1811c16c28d73ebe59f47daa16cd4522:

 AuthDirSharedRandomness needs a tor manual page entry.

 There are missed opportunities for const pointers in this commit.

 srv_to_ns_string() and get_majority_srv_from_votes() also encode the SRV
 as hex rather than base64.

 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;
   }
 }}}

 Nitpick: get_majority_srv_from_votes() uses `unsigned int current` for a
 boolean value. Tor typically uses int for boolean values.

 8e180aa061f0ea39968f05c85796b26f10a81744:

 extract_shared_random_commits() and sr_state_copy_reveal_info() and
 should_keep_commit() and possibly other functions could use const
 pointers.

 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);
 }}}

 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.

 extract_shared_random_srvs() can error out on the current SRV, and leave
 the previous SRV allocated and initialised. What happens then?

 verify_commit_and_reveal() also expects crypto_digest256() to return -1.
 It never will.

 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?)

 8f3f5667754037cccd758eb8631636e2e78fed93:

 Looks ok.

 3f2014d7aabe2e10163f4f44a8ca46fd5a91f04a:

 I'm not going to review the unit tests in detail. (We generally accept
 unit tests if they work, right?)

 src/test/sr_srv_calc_ref.py looks like it uses SHA256, and not SHA3-256.

 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)
 }}}

 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)
 }}}

 And I also see `12/720 TESTS FAILED. (1 skipped)`.

 The dgoulet/prop250_final_v4 branch also has similar unit test issues.

 Now on to the extra commits in dgoulet/prop250_final_v4:
 There are twelve extra commits in this branch.

 625e08167d9d8e855ddefa45f8d9902f65122f1f:
 0308e7bf92c3699fa7e88140ae5037d8bc0d480b:
 7bb6eac9fb6af2c13f61e8ddb9e9c741fa4736d1:
 fe710d15af7709f9d88ae9d8b5e451e5b849983b:

 I trust the compiler to warn about consts in the wrong places.

 18771227176e6f634540d483b8c1b1a6501aec14:
 75a243b9aad489929d8ed8273920fcf7fc295ebb:
 50ab3ae82b8248a45203c4ee9fc6be5d18522ac8:
 c0401326224a994dfe35e39da4aa8849ac053ee3:

 Look good.

 e5613fad328e78b2477d8e848bb163aa8dcb1e8b:

 Thanks for removing the duplicate code here.

 08beead18ed2bfdc8f09b17e7628ac9997222e3a:
 10ccf50d599896feaf48bf0aecf4a1c2da1fea45:

 Better safe than sorry.

 c219b23d47e9c742532354eedae10ca0509a1d78:

 One I didn't pick up, thanks for fixing it.

 I've finished reviewing the code in dgoulet/prop250_final_v4.
 Over to you to respond to my comments 25, 26 & 27.

 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.)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16943#comment:27>
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