[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