[tor-bugs] #17668 [Tor]: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we couldn't parse.

[Tor Bug Tracker & Wiki]

#17668: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we
couldn't parse.
------------------------------------------------+--------------------------
 Reporter:  arma                                |          Owner:  nickm
     Type:  defect                              |         Status:  accepted
 Priority:  High                                |      Milestone:  Tor:
Component:  Tor                                 |  0.2.7.x-final
 Severity:  Blocker                             |        Version:
 Keywords:  201512-deferred, TorCoreTeam201602  |     Resolution:
Parent ID:                                      |  Actual Points:
  Sponsor:                                      |         Points:
------------------------------------------------+--------------------------

Comment (by teor):

 Spec / Design Review:

 This addition to dir-spec.txt only makes sense if you've read the
 NoEdConsensus description, can we say "do not produce a majority consensus
 about a relay's Ed25519 key"?
 {{{
 +        * If consensus method 22 or later is used, and the votes do not
 +          produce a majority consensus about Ed25519 key (see 3.8.0.1
 below), the
 +          consensus must include a NoEdConsensus flag on the "s" line.
 }}}

 I'm not seeing how the NoEdConsensus flag actually affects tor's
 behaviour. So I assume the client/relay/authority modifications for using
 the NoEdConsensus flag to avoid certain ed25519 keya will be handled in a
 separate ticket?

 Code review:

 Things I think are wrong or incomplete:

 routers_make_ed_keys_unique claims to check IP:ORPort, but I don't see
 where we check it in that function.
 That comment is ambiguous, we should be checking IPv4:ORPort,
 IPv4:DirPort, IPv6:ORPort, and IPv6:DirPort for uniqueness. But I think we
 only want to do that check among routerstatuses with the Running flag
 (those we;ve checked reachable). Which means IPv4 ORPorts only for the
 moment, as we don't check anything else.
 (Otherwise, this opens up a denial of service opportunity where a relay
 claims to have a popular relay's IPv4:ORPort, IPv4:DirPort, IPv6:ORPort,
 or IPv6:DirPort.)
 Maybe this is best tackled by a different function in a different ticket.
 So let's fix the comment in this one.
 {{{
 +/** If there are entries in <b>routers</b> with exactly the same
 IP:OrPort or
 + * exactly the same ed25519 keys, remove the older one.  May alter the
 order
 + * of the list. */
 }}}

 This check preserves the existing digestmap entry if the published times
 are equal. But different authorities could choose different descriptors-
 with-identical-published-times, depending on the order they arrived at the
 authority.
 So let's find a consistent way of selecting a descriptor to omit if their
 published times are identical - how about comparing
 signed_descriptor_digest? (And if the digests are identical, it really
 doesn't matter which descriptor we omit, because they have the same
 content.)
 We should do the same tie-breaking in the IP:Port checks too.
 {{{
 +    if ((ri2 = digest256map_get(by_ed_key, pk))) {
 +      /* Duplicate; must omit one.  Omit the earlier. */
 +      if (ri2->cache_info.published_on < ri->cache_info.published_on) {
 +        digest256map_set(by_ed_key, pk, ri);
 +        ri2->omit_from_vote = 1;
 +      } else {
 +        ri->omit_from_vote = 1;
 +      }
 }}}

 Things I don't understand:

 I have no idea what "treat all routers with that identity as the same"
 means here - does it mean: "a new descriptor with this id replaces any
 older descriptors with this id"?
 {{{
 + * The rule is:
 + *    If an RSA identity key is listed by more than half of the
 authorities,
 + *    include it, and treat all routers with that identity as the same.
 + */
 }}}

 Should this read "no <ed,rsa> has been listed by more than half the
 authorities"?
 Should it read "include <NULL, rsa>"?
 {{{
 + *    Otherwise, if an <*,rsa> or <rsa> identity is listed by more than
 + *    half of the authorities, and no <ed,rsa> has been listed, include
 + *    it.
 }}}

 Typos:

 I think you mean `n'th` and `n'th` here (to match
 dircollator_get_votes_for_router), not `i'th` and `n'th`:
 {{{
 +  /* The i'th member of this array corresponds to the vote_routerstatus_t
 (if
 +   * any) received for this digest pair from the n'th voter. */
 }}}

 Can we decide whether to use i'th or n'th in all these added comments, or
 do they mean different things?

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