[tor-bugs] #15621 [Core Tor/Tor]: Kill the pre-version 3 intro protocol code with fire.

[Tor Bug Tracker & Wiki]

#15621: Kill the pre-version 3 intro protocol code with fire.
---------------------------------------+-----------------------------------
 Reporter:  yawning                    |          Owner:  dgoulet
     Type:  enhancement                |         Status:  needs_review
 Priority:  Medium                     |      Milestone:  Tor:
Component:  Core Tor/Tor               |  0.2.9.x-final
 Severity:  Normal                     |        Version:
 Keywords:  tor-hs, TorCoreTeam201604  |     Resolution:
Parent ID:  #6418                      |  Actual Points:
 Reviewer:  asn                        |         Points:  small
                                       |        Sponsor:  SponsorR-can
---------------------------------------+-----------------------------------
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:16 nickm]:
 >  * NM1: I don't like the looks of the hardcoded `(1<<3)` refereneces;
 can these become a macro or move into a `hs_protocols_supported()`
 function or something? Similarly the "if version != 3" one.

 For supported protocols, see commit:
 0c0c76095c370817051d2b46596b29cf59cefba6

 As for the `version != 3` where the number is hardcoded. Every code site
 where it's hardcoded, we are in a code path _dedicated_ for v3. The
 functions name are sometimes misleading like we use the _v2 parse intro
 function for the v3... Unless we refactor cleanly, I think it's "ok" to
 leave them like that considering the potential refactor that 224 will
 bring for those functions (if any). Plausible?

 >  * NM2: Should the union in rend_intro_cell_s go away? Unions can be
 scary.  This could be another ticket.

 Yup it could! Although, as a minimal change for killing version protocol <
 3, I would make this one another ticket. Chances are though that 224 will
 _not_ touch this so this union will only see v3 in the future.

 >  * NM3: err_msg_out isn't used in rend_service_parse_intro_unsupported.
 Should it be?  There's probably *some* message to give there, right?

 That function is actually never reached because of the version check in
 `rend_service_parse_intro_plaintext` prior to the use of
 `intro_version_handlers`. And on error, it also returns an undocumented
 status code so tbh I'm not even sure that this is needed anymore. Should
 we rip it off?

 >  * NM4: I am concerned about fingerprinting attacks here. Do we have a
 good story about fingerprinting hidden services and their users as they
 upgrade, and why that will/won't matter?

 Hrm... so the fingerprinting attack here is basically knowing that this HS
 did upgrade to 029 or is there something else? It's doable but what did we
 do when we moved to v0->v1 and v1->v2 ?

 Note that here it will _only_ indicate the the HS is at >= 0.2.9.0, no the
 exact version. Not sure what it can give to the attacker other than try to
 find an attack on that version? If this is a real issue, we'll have a hard
 time with users on 224 :).

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