[tor-bugs] #27194 [Core Tor/Tor]: Reject protover extra commas in protover (was: handling extra commas in protover)

[Tor Bug Tracker & Wiki]

#27194: Reject protover extra commas in protover
-------------------------------------------------+-------------------------
 Reporter:  cyberpunks                           |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  unspecified
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.2.9.4-alpha
 Severity:  Normal                               |     Resolution:
 Keywords:  needs-consensus-method, rust,        |  Actual Points:  0.2
  security-low, 029-backport, 034-backport,      |
  035-backport, 040-backport, 041-backport, 032  |
  -unreached-backport, 035-deferred-20190115,    |
  041-proposed, 033-backport-unreached           |
Parent ID:                                       |         Points:  0.2
 Reviewer:  ahf                                  |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * keywords:
     rust, security-low, 029-backport, 034-backport, 032-unreached-
     backport, 035-deferred-20190115, 041-proposed, 033-backport-unreached
     =>
     needs-consensus-method, rust, security-low, 029-backport,
     034-backport, 035-backport, 040-backport, 041-backport, 032-unreached-
     backport, 035-deferred-20190115, 041-proposed, 033-backport-unreached
 * points:   => 0.2
 * actualpoints:   => 0.2


Comment:

 Here's the impact of this change:
 * protover_compute_vote() may start producing a different consensus when
 it encounters patters that used to be accepted, because it ignores
 recommended/required lines that it considers malformed. This change
 requires a new consensus method, or rejecting the entire vote.
 * protover_compute_vote() calls contract_protocol_list(), which calls
 parse_single_entry(), but only on entries that have already been parsed
 and rejected. So the BUG() should be impossible.
 * parse_protocol_list() starts rejecting votes and descriptors that
 contain patterns that used to be accepted. That's ok.

 Replying to [comment:19 cypherpunks3]:
 > Replying to [comment:15 ahf]:
 > > Maybe we need to merge #27197 and this at the same time?
 >
 > Given that this is true, why is this 'needs_revision'?

 The ticket was marked needs_revision because the it broke the rust builds
 on CI, and when that happened, it was unclear how to fix it. Then, it
 stayed in needs_revision because no-one created working branches for 0.2.9
 and 0.3.3.

 Now 0.3.3 is not maintained any more, so here are some draft branches for
 0.2.9 and 0.3.4:

 0.2.9, which has no rust:
 https://github.com/torproject/tor/pull/768

 0.3.4, including the rust fix from #27197, which is already in 0.3.5 and
 later:
 https://github.com/torproject/tor/pull/769

 This change impacts the consensus, so we need to do one of these things:
 1. define a new consensus method, create two versions of
 parse_single_entry(), and call the fixed version only when creating a
 consensus with the new consensus method, or
 2. in networkstatus_parse_vote_from_string(), reject votes that have
 malformed:
   * recommended/required_client/server_protocols
   * relay protocol lines, as parsed by
 routerstatus_parse_entry_from_string()

 When we make this change, we'll be introducing a version distinguisher.
 That's ok for most directory users, because the authorities check their
 documents. But bridge clients can be probed by their bridges to see if
 they have this fix. We might need to accept that risk.

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