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

[Tor Bug Tracker & Wiki]

#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):

 Replying to [comment:21 dgoulet]:
 > After proposal review that happened few days ago on #tor-dev, we made
 some adjustments to the specification and code.
 >
 > Please find the latest code here: `prop250_final_v3`
 >
 > Proposal 250 has been updated with the latest in torspec.git

 Code review:

 General comments:

 Every other multi-word file name in src/or uses underscores.

 If a function moves data from `(dtype* dst, const stype* src)`, can the
 arguments be in that order, and can the src argument be const?

 There are a lot of missed opportunities for const pointer arguments.

 e282227ebc03ae71f1cd6490a2adf2058140c66e:

 If you use `#ifdef WORDS_BIGENDIAN` (from orconfig.h), rather than a
 runtime check against htonl, some compilers will produce better code.

 72baec6805aaab8d62f2deed8fbc658c82a7d086:

 shared-random-state.c:

 If we use an asymmetric magic number, we can detect when the two halves
 are swapped.
 {{{
 #define SR_DISK_STATE_MAGIC 42424242
 }}}

 A hard-coded `SHARED_RANDOM_N_ROUNDS` is going to make it really hard to
 test hidden services quickly using chutney. (We'll always be testing them
 using the default initial shared random value.) Can we make this
 configurable in test networks?
 {{{
 #define SHARED_RANDOM_N_ROUNDS 12
 }}}

 get_start_time_of_current_round() doesn't take
 TestingV3AuthVotingStartOffset into account. Why not use:
 {{{
 time_t next_start = voting_schedule.interval_starts;
 time_t curr_start = dirvote_get_start_of_next_interval(next_start -
 interval - 1,interval,
                       options->TestingV3AuthVotingStartOffset);
 }}}

 get_next_valid_after_time() duplicates code in
 dirvote_recalculate_timing().
 Why not use `voting_schedule.interval_starts`?
 (You'd need to make sure dirvote_recalculate_timing() had been called
 before either of these functions used `voting_schedule.interval_starts`.)

 Nitpick: why 2? Can it be a named constant like `SHARED_RANDOM_N_PHASES`?
 get_state_valid_until_time, get_sr_protocol_phase
 {{{
 int total_rounds = SHARED_RANDOM_N_ROUNDS * 2;
 }}}

 get_state_valid_until_time() also ignores TestingV3AuthVotingStartOffset.
 I think this should fix it:
 {{{
 current_round = (beginning_of_current_round / voting_interval) %
 total_rounds;
 }}}

 get_sr_protocol_phase() also ignores TestingV3AuthVotingStartOffset.
 Perhaps passing around the time isn't the best way to do this? It seems
 like if we can make this same mistake in multiple places, then we should
 refactor the round number calculation so it's in a single function.

 Does get_sr_protocol_phase() need to check it's getting an actual
 valid_after time, rather than now?

 Why do we have disk_state_validate() and disk_state_validate_cb()?
 Should one call the other? Should they check _magic?
 Should they check ValidAfter < ValidUntil?

 disk_state_parse_commits() fails to `SMARTLIST_FOREACH(args, char *, cp,
 tor_free(cp));` if commit is NULL.

 disk_state_parse_sr_values() contains duplicate code in the if/else block.
 The function comment should say it parses both SharedRandPreviousValue and
 SharedRandCurrentValue.

 I think disk_state_parse_sr_values() might be able to leak memory if
 disk_state_parse_srv() fails. At the very least, it leaves an allocated
 but zero pointer, which could confuse the rest of the code.

 disk_state_parse_sr_values() can assert its arguments are not NULL.

 disk_state_put_commit_line() can have commit as a const pointer, and in
 the opposite order for consistency.

 In disk_state_put_commit_line(), do we need to memwipe reveal_str?
 Or are we about to make it public anyway?

 disk_state_put_srv_line() can have srv as a const pointer, and in the
 opposite order for consistency.

 I think disk_state_reset() is the moral equivalent of disk_state_free()
 followed by disk_state_new(). Why are they different functions? (Apart
 from the memory thing.)

 We don't seem to check for errors in disk_state_update(). I think this is
 ok, but I'm not sure exactly why.

 In disk_state_save_to_disk() and children (and other functions), should we
 memwipe any temporary memory that contains the reveal value, or the
 encoded reveal value? (before it is revealed)
 We do this some places, but not others.

 Can data in state_query_get_() be a const pointer?
 Some of the sr_state_set_* functions can take const pointers.

 shared-random-state.h:

 I think this comment is in the wrong place, shouldn't it correspond to
 `SHARED_RANDOM_STATE_PRIVATE`?
 {{{
 /* Private methods (only used by shared-random.c): */
 }}}

 shared-random.c:

 {{{
 char b64_decoded[SR_COMMIT_LEN + 2];
 }}}
 Can we either fix #17868 as part of this branch, or add a #define for the
 rounding-up that has to occur when the base64 value isn't padded?
 {{{
 #define B64_DECODE_PAD(len) ((len) + ((len) % 3))
 char b64_decoded[B64_DECODE_PAD(SR_COMMIT_LEN)];
 }}}

 Is TIMESTAMP first, or H(REVEAL)?
 commit_decode says one thing in the comments, and another in the code.

 Can we assert that `decoded_len == SR_COMMIT_LEN`? Is that useful?

 reveal_decode() duplicates commit_decode(). Can we refactor to avoid that?

 In sr_parse_srv(), why do we use base16 for the SRV, but base64 for the
 commits?

 In sr_parse_commit(), we ignore extra arguments after the fourth. Is this
 intentional?
 A comment either way would help.

 I have 5 more commits to review. (Please add fixups to the end of the
 branch so I don't lose my place.)

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