[tor-bugs] #18318 [Tor]: Make sure keys and IP:Ports are unique in a consensus

[Tor Bug Tracker & Wiki]

#18318: Make sure keys and IP:Ports are unique in a consensus
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  nickm
     Type:  defect                               |         Status:
 Priority:  Very High                            |  needs_revision
Component:  Tor                                  |      Milestone:
 Severity:  Blocker                              |        Version:
 Keywords:  TorCoreTeam201602, must-fix-         |     Resolution:
  before-028-rc                                  |  Actual Points:
Parent ID:  #17668                               |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by nickm):

 Replying to [comment:9 dgoulet]:
 > >  There is now an `ed25519_voting_fixes` branch in torspec too.
 >
 > This branch diff with master is _empty_. Maybe you forgot to
 commit/push? :)

 As above actually see the torspec branch ed25519_voting_again.

 > Code review:
 >
 > * 3941fa64ccf10dda5ac224bb1160564581c0e213
 >
 >  `routers_make_ed_keys_unique()`: the digestmap `by_ed_key` is not
 freed.

 Calling this `DG1`. Fix in babd9df586312f

 >  `routers_make_ed_keys_unique()`: can't you use `SMARTLIST_DEL_CURRENT`
 directly in the first loop instead of flagging it then removing it?

 No because sometimes we flag ri and sometimes we flag ri2. Calling this
 `DG2`. Adding comments for this in babd9df586312f

 >  Actually, maybe I'm not understanding the whole picture here, but
 that's called in `dirserv_generate_networkstatus_vote_obj()` then few
 lines below `rl` is used with `dirserv_count_measured_bws();` and not the
 list with unique entries `routers`. Is it OK here to use a list of non
 unique entries? (same goes for `dirserv_compute_performance_thresholds`)

 Oh, very interesting.  Calling this `DG3`.  Will work on a fix.

 > * 03d484f2806e0df07d1813d3e69ec4840a42be60
 >
 >  `if (! vrs->has_ed25519_listing)`, has extra space.

 If you mean the one between the `!` and the `vrs`, I don't mind a single
 space there.

 >  Below, we just warn and we continue to try to create a valid consensus
 even though there is an obvious issue? If that bug is hit, will it create
 an invalid consensus?
 >
 > {{{
 > +      if (ed_consensus > 0) {
 > +        tor_assert(consensus_method >=
 MIN_METHOD_FOR_ED25519_ID_VOTING);
 > +        if (ed_consensus <= total_authorities / 2) {
 > +          log_warn(LD_BUG, "Not enough entries had ed_consensus set;
 how "
 > +                   "can we have a consensus of %d?", ed_consensus );
 > +        }
 > +      }
 > }}}
 >

 IMO this bug can't actually be hit.  But it wouldn't mean that the
 consensus was invalid; it would mean that the algorithm in dircollate.h
 would be broken.

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