Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16260 closed defect (fixed)

HS can repick an expired intro points or one that we've already picked

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR


Let's assume here we are about to cycle our intro points in rend_services_introduce(), we do a dance and at the end we choose 3 new ones. To do that, we use router_choose_random_node() that picks a random node using the torrc ExcludeNodes list but also an exclude_nodes list that we've populated before which contains expired intro points.

After that, we happily launch an intro circuit by calling rend_service_launch_establish_intro() and then circuit_launch_by_extend_info(). In there, a circuit can be cannibalized and if so, the exit node is NOT extended to our intro points that we picked earlier because of our purpose:

    /* it's ready right now */

The IP is then changed to the one we just cannibalized once we have our circuit. This is not desirable for two reasons:

1) We ignore the exclude nodes list thus we can end up picking an expired IP.
2) We can use to the same IP multiple times for a single descriptor.

All of the above, I was able to reproduce in a chutney network that is having a set of IPs that expired in the previous period and a set of IPs that are all the same.

It's not that terrible in the real network because the probability of that happening is roughly 1/<relay count> which is < 1% currently, still worth fixing imo.

Possible solution for this:

i) Use a 4th hop for the IP circuit meaning we allow cannibalization but we extend the circuit to the intro point.

ii) Disable cannibalization for intro point thus always creating a fresh circuit ending up at the right place.

My pick here would be i) because I think a 4th hop is cheaper than a creating a new circuit.

Comments welcome!

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by dgoulet

Status: newneeds_review

Discussion with asn on IRC, i) sounds like a good choice. We tested it also for #8239 (through big branch in #4862). It's working well so time for feedback and review!

See branch: bug16260_027_01

comment:2 Changed 5 years ago by nickm

The big conditional we removed, the one starting with

-  if (tor_memneq(intro->extend_info->identity_digest,
-      launched->build_state->chosen_exit->identity_digest, DIGEST_LEN)) {
-    char cann[HEX_DIGEST_LEN+1], orig[HEX_DIGEST_LEN+1];

I'm guessing you removed that because it can't happen, right? Should that become an assertion or something in that case?

comment:3 Changed 5 years ago by dgoulet

Following discussion with asn on IRC, an assert has been added to make sure we have the right exit node.

See branch: bug16260_027_02

comment:4 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay, merged!

comment:5 Changed 5 years ago by arma

I know I'm late to the party, but: doesn't the performance hit of always having an extra hop, for every introduction attempt, vastly outweigh the performance hit of making a new circuit that once?

(We could just believe my intuition here; or we could use Shadow or the like to actually find out.)

comment:6 Changed 5 years ago by dgoulet

Hrm, it could be but intro points are used for very little stuff that is setup the RP and after a few cells we are done. With lots of clients, it could slow down the "establish a connection to HS dance", yes.

I want to point out also that the latest Usenix paper (not yet public) does mention that cannibalization is a useful counter measure to circuit fingerprinting. However, they also propose mitigation for newly created HS circuit (IP/RP/HSDir) so I could be convinced that a new circuit is better but my instinct tells me that it shouldn't be that crazy performance hit. Would be a good use case for OnionPerf!

comment:7 Changed 5 years ago by arma

Status from Roger: I think the performance penalty from a 4-hop intro point is never worth it. So we could either a) only cannibalize a circuit if it ends at the intro point we picked, and otherwise build a new one, or b) simply not even check for cannibalization, since it'll almost never work, or c) check, even before we pick our new intro point, whether there's an adequate circuit to use, and if so, use it and pretend that's the intro point we would have picked.

I would argue for 'a' or 'b' since they're simpler and don't involve subtle anonymity questions. In particular, here is the subtle anonymity question for 'c': let's say we built a preemptive circuit 50 minutes ago, and since then we fetched a new consensus document with wildly different weights for the last hop on that circuit. If we re-used it as our intro point, we would end up with different behavior than if we had picked a new intro point with our new weights. How bad is that? For almost all cases it will be roughly the same. For a few cases we'll leak...what...whether we had a preemptive circuit and used it? It's possible that we just lost our network connection for a while, leading to all our preemptive circuits being closed. So in a small fraction of cases we'd leak whether that was true? Is that a big deal?

David argues for cannibalization because of the upcoming Usenix Security paper that describes cannibalization in this case as a security improvement. I haven't read the paper yet, but my intuition says that there are many ways to distinguish hidden service circuits from other types of circuits at the client side, since we never aimed to protect that. But (uninformed) intuition is a terrible thing to use when making these decisions.

Anyway, there we are. :)

comment:8 Changed 5 years ago by arma

Maybe the next step is to open a new ticket, "most intro points are 4 hops when they can be only 3"?

comment:9 in reply to:  8 Changed 5 years ago by asn

Replying to arma:

Maybe the next step is to open a new ticket, "most intro points are 4 hops when they can be only 3"?

Made #16646.

comment:10 Changed 5 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR

comment:11 Changed 5 years ago by nickm

Keywords: 2016-bug-retrospective added

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.