[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 Sebastian):

 60efce445b17d4b4153e91527887873812f5599f looks fine. Needs a tor-spec
 patch to go with it, tho.

 42750ab63c2a5dc2b5c0fd96335fd043b6beef32:

 General note: The style for opening and closing comments is inconsistent,
 sometimes they go on their own line, sometimes not. Do we care? I don't
 necessarily.

 {{{
 +/** Helper: add a single vote_routerstatus_t <b>vrs</b> to the collator
 +    <b>dc</b>, indexing it by  */
 }}}
 this wants expansion

 {{{
 +/** Sort the entries in <b>dc</b> according to <b>consensus_method</b>,
 so
 + * that the consensus process can iterate over them with
 + * <b>dircollator_n_routers</b> and dircollator_get_votes_for_router</b>.
 */
 }}}
 this wants a <b> before the dircollator_get_votes_for_router

 {{{
 +/**
 + * Collation function for ed25519 consensuses: collate the votes for each
 + * entry in <b>dc</b> by ed25519 key and by RSA key.
 + *
 + * 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.
 + */
 }}}
 This should probably explain what this means for conflicts of the ed25519
 keys?

 {{{
 +    /* If not enough authroties listed this exact <ed,rsa> pair,
 +     * don't include it. */
 }}}
 typo authorities

 Should we add asserts to the functions that say "this may only be called
 after dircollator_collate"? seems like we have the is_collated field for
 that purpose.

 {{{
 +  /** Map from <ed, RSA-SHA1> pair to an array similar to that used in
 +   * by_rsa_sha1 above. */
 }}}
 what happens for duplicate rsa keys? Should spell it out I think

 7cd091220c35c71025ab90cfa0bd18d206c29e8e:
 the function name is networkstatus_parse_vote_from_string() - that seems
 misleading if we're treating consensuses?
 I don't get it especially for stuff like this:
 {{{
      if (ns->type != NS_TYPE_CONSENSUS) {
        if (check_signature_token(ns_digests.d[DIGEST_SHA1], DIGEST_LEN,
                                  tok, ns->cert->signing_key, 0,
 -                                "network-status vote")) {
 +                                "network-status document")) {
 }}}

 3941fa64ccf10dda5ac224bb1160564581c0e213:
 I'm unsure this is sufficient. Can't we still construct a consensus from a
 situation where (consider attacking relays):
 one dirauth publishes two rsa keys without ed keys, four dirauths publish
 one of the rsa keys with an ed key, the other four publish the other rsa
 key with the same ed key? Once I'm convinced this cannot happen I'll
 review the logic of this commit, too

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