[tor-bugs] #24031 [Core Tor/Tor]: Protover.rs could use a better algorithm

[Tor Bug Tracker & Wiki]

#24031: Protover.rs could use a better algorithm
---------------------------------------------+-----------------------------
 Reporter:  nickm                            |          Owner:  isis
     Type:  defect                           |         Status:
                                             |  needs_review
 Priority:  Very High                        |      Milestone:  Tor:
                                             |  0.3.3.x-final
Component:  Core Tor/Tor                     |        Version:  Tor:
                                             |  0.3.3.1-alpha
 Severity:  Normal                           |     Resolution:
 Keywords:  rust 033-must protover security  |  Actual Points:  5
Parent ID:                                   |         Points:  1
 Reviewer:                                   |        Sponsor:  SponsorM-
                                             |  can
---------------------------------------------+-----------------------------
Changes (by isis):

 * status:  needs_revision => needs_review
 * actualpoints:   => 5
 * priority:  High => Very High
 * version:   => Tor: 0.3.3.1-alpha
 * sponsor:   => SponsorM-can
 * milestone:  Tor: 0.3.4.x-final => Tor: 0.3.3.x-final
 * keywords:  rust 033-must protover => rust 033-must protover security


Comment:

 I've cleaned up (mostly! sorry sorry!) my code into more understandable
 commits in my `bug24031_r4` branch.

 The changes to the unittests might be a little hard to follow, that commit
 is not quite pleasant (although the integration test changes in that same
 commit should be simple to follow to see where/where behaviour has
 changed, so I'd argue that the internal unittests changing precariously
 isn't as much of an issue?).  If test changes (or any changes) are
 difficult for the reviewer to understand, please feel free to ask
 questions, or make me split that commit up better.

 The other thing is that one test I've added intentionally fails:
 `protover_all_supported_should_include_version_we_actually_do_support`.
 The behavioural difference is this: if we take a protover string
 `"Link=3-999"` and ask if it is supported, when `"Link=1-5"` ''is''
 supported:

  * the C version returns `"Link=3-999"` (which is, IMHO, wrong, since we
 ''do'' support 3, 4, and 5.
  * the Rust version returns `"Link=6-999"`, which I believe to be the
 correct implementation of the spec?

 Upping the priority because potential DoS.

 Upping the actual points to 5, although only ~3 were spent on this ticket,
 and another 2 finding/fixing related bugs elsewhere.

 Marking as SponsorM-can, although that may be wrong; feel free to correct.

 Changing the version to reflect inclusion in 0.3.3, along with the
 appropriate keywords.

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