Opened 3 years ago

Last modified 6 months ago

#15621 accepted enhancement

Kill the pre-version 3 intro protocol code with fire.

Reported by: yawning Owned by: dgoulet
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs technical-debt deprecation
Cc: Actual Points:
Parent ID: #6418 Points: 1
Reviewer: asn Sponsor: SponsorR-must

Description

We still have code for dealing with version 0, 1, and 2 of the HS intro protocol.

From rend-spec.txt:

As of Tor 0.2.0.7-alpha and 0.1.2.18, clients switched to using the v2 intro format.

From the release notes:

Bugfix on 0.2.1.6-alpha, when the v3 intro-point protocol (the first one which sent a timestamp field in the INTRODUCE2 cell) was introduced;

Anything that generates INTRODUCE cells with these versions are long dead, so the code for handling this protocol version should be removed.

Child Tickets

Change History (31)

comment:1 Changed 3 years ago by nickm

The only reason we haven't removed them so far is the possibility of identifying the version of Tor running a hidden service based on which versions it won't accept. Not sure if that's a compelling reason.

comment:2 Changed 2 years ago by nickm

Keywords: TorCoreTeam201507 added

comment:3 Changed 2 years ago by nickm

Cc: dgoulet added

I would be fine removing this code. David, I know you're interested in in hacking on this code; is this an area you'd like to hack on?

comment:4 Changed 2 years ago by dgoulet

Keywords: TorCoreTeam201508 added; TorCoreTeam201507 removed
Owner: set to dgoulet
Status: newassigned

comment:5 Changed 2 years ago by nickm

Keywords: TorCoreTeam201509 added; TorCoreTeam201508 removed

comment:6 Changed 2 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:7 Changed 2 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

Move a few tickets out of 0.2.8. I would take a good patch for most of these if somebody writes one. (If you do, please make the ticket needs_review and move it back into maint-0.2.8 milestone. :) )

comment:8 Changed 2 years ago by arma

Severity: Normal

Is this a subset of #6418? And there, we decided to phase out the old unused parts of the HS protocol while we roll out the new one, to minimize partitioning issues?

Does that mean we should close this one as a duplicate of #6418?

comment:9 Changed 2 years ago by teor

Parent ID: #6418

comment:10 Changed 23 months ago by dgoulet

Status: assignedneeds_review

I've made a branch for this one that kills v0/v1/v2 protocol version. Not only that but makes client and HSDir cache _only_ store v3 descriptors for now on. I might of miss some part since version check are spread around the code...

This works in chutney perfectly.

Branch: ticket15621_028_01

comment:11 Changed 21 months ago by dgoulet

Keywords: TorCoreTeam201509 removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Sponsor: None
Type: enhancementdefect
Version: Tor: unspecified

comment:12 Changed 21 months ago by dgoulet

Cc: dgoulet removed
Points: small
Sponsor: NoneSponsorR-can
Type: defectenhancement

comment:13 Changed 21 months ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:14 Changed 20 months ago by asn

Reviewer: asn

comment:15 Changed 20 months ago by asn

Status: needs_reviewmerge_ready

Patch looks good to me.

Glad to see all that code getting deleted.

comment:16 Changed 20 months ago by nickm

Status: merge_readyneeds_revision
  • 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.
  • NM2: Should the union in rend_intro_cell_s go away? Unions can be scary. This could be another ticket.
  • 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?
  • 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?

comment:17 in reply to:  16 ; Changed 20 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to 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 :).

comment:18 in reply to:  17 Changed 20 months ago by arma

Replying to dgoulet:

  • 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?

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 :).

That was why we originally (comment:8) had in mind to do one switch, rather than several switches. That is, to do this change when we roll over to the next-gen design so there's only one partition.

That said, I am increasingly a fan of just getting the coding done and not worrying too much about intermediate states for users (if that worrying means slowing everything else down). But I can definitely see both sides.

comment:19 Changed 20 months ago by nickm

Ack on NM1...NM2. I think that for NM3, you're right, but we should add a tor_fragile_assert() there to make intent more clear. (Or even tor_assert_nonfatal_unreached().)

For NM4, the fingerprinting issue: The case I'm worried about is where we learn that an HS upgraded at the same time that some other view of the HS upgraded. (eg, it's being used as a client too, and they're trying to keep the client identity separate from the HS identity, but both changes are observable.) Am I being too paranoid here?

comment:20 in reply to:  19 Changed 20 months ago by dgoulet

Status: needs_reviewneeds_information

Replying to nickm:

Ack on NM1...NM2. I think that for NM3, you're right, but we should add a tor_fragile_assert() there to make intent more clear. (Or even tor_assert_nonfatal_unreached().)

Yup. I see that as a refactoring also since this could imply to drop that intro version handler array because it's unused but we could actually make it useful also.

For NM4, the fingerprinting issue: The case I'm worried about is where we learn that an HS upgraded at the same time that some other view of the HS upgraded. (eg, it's being used as a client too, and they're trying to keep the client identity separate from the HS identity, but both changes are observable.) Am I being too paranoid here?

Hrm... so yes, it's somehow a possibility. I'm unsure how you can tell if a client has also upgraded to 029+ thus correlate but if there is a way, it's a tiny bit possible.

Middle ground here. arma proposed to merge this patch only when 224 gets in so we do one big transition. If we are OK with that, for which I'm ok with, let's push this ticket to 0.2.??? (224 won't be in until 0.2.10/0.3.0 imo)?

comment:21 Changed 20 months ago by nickm

Other middle ground: completely remove all client-side and relay-side support for the old protocols right now. Put the hidden-service-side support behind a consensus option. Flip the switch there at some point.

comment:22 Changed 20 months ago by nickm

(Would it be okay to do the "middle ground" I describe above, or way too hard/way too much work?)

comment:23 Changed 20 months ago by arma

Another linkability attack: somebody runs two onion services, either on the same Tor instance or on two machines that are upgraded at once. An observant person can notice that these two onion services dropped v2 support at the same time.

David and I discussed, and we think the middle ground patch will be more work, and we should just hold off on merging this until we do the 224 roll-out too. (It's a small patch, and if we break it we can fix it, and if we don't break it then win.)

comment:24 Changed 20 months ago by nickm

Keywords: TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

Okay, then I'm calling it postponed-from-april.

comment:25 Changed 19 months ago by isabela

Points: small1

comment:26 Changed 19 months ago by dgoulet

Keywords: TorCoreTeam-postponed-201604 removed
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???
Status: needs_informationneeds_revision

Moving this out of 029 because we seem to want this to be rolled out once 224 is rolled out with a consensus params. No chance for this to be in 029. Adding prop224 keyword so we know about it once we deploy next-gen HS.

comment:27 Changed 14 months ago by dgoulet

Sponsor: SponsorR-canSponsorR-must
Status: needs_revisionaccepted

comment:28 Changed 13 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:29 Changed 12 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:30 Changed 7 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:31 Changed 6 months ago by nickm

Keywords: technical-debt deprecation added
Note: See TracTickets for help on using tickets.