Opened 4 months ago

Last modified 2 days ago

#26368 needs_revision defect

Consider circuit isolation when closing redundant intro points

Reported by: sysrqb Owned by: neel
Priority: High Milestone: Tor: 0.3.6.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-client, 035-roadmap-proposed, tbb-needs
Cc: gk, mahrud, neel Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

When tor receives more than one request for connecting to an onion service within a short amount of time, and these requests are circuit-isolated (by sockauth, or something else), tor will launch multiple connections to an intro point (one for each isolated request). When the first introduction succeeds (intro acks), tor closes any circuits it considers redundant (rend_client_close_other_intros). Tor should only close circuits if the socks request isolation bits match.

Child Tickets

Change History (21)

comment:1 Changed 4 months ago by gk

Cc: gk added

comment:2 Changed 4 months ago by teor

Component: Core TorCore Tor/Tor
Keywords: tbb-wants 035-proposed tor-hs tor-client added
Milestone: Tor: unspecified

comment:3 Changed 4 months ago by arma

Cc: mahrud added

comment:4 Changed 3 months ago by nickm

Keywords: 035-roadmap-proposed added; 035-proposed removed

comment:5 Changed 3 months ago by teor

Keywords: tbb-needs added; tbb-wants removed

Prefer the more common tbb-needs to tbb-wants.
There doesn't appear to be any difference in how much TBB needs based on the flag.

comment:6 Changed 6 weeks ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:7 Changed 6 weeks ago by neel

Status: assignedneeds_review

comment:8 Changed 6 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:9 Changed 5 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Hmm not sure this is accurate. I think we need to match _both_ that is the .onion and the isolation flags.

If we happen to request 5 different .onions, we need to only close intro points that are for the same .onion on the intro circuit.

comment:10 Changed 5 weeks ago by neel

I have pushed these changes.

comment:11 Changed 5 weeks ago by neel

Status: needs_revisionneeds_review

comment:12 in reply to:  10 Changed 5 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Replying to neel:

I have pushed these changes.

Oh not the dest_address but rather the .onion address that is the rend_pk_digest that was originally removed.

In other words, the .onion address of the SOCKS request *and* the flags need to be matched. Else, we'll close things that we shouldn't I think.

comment:13 Changed 5 weeks ago by neel

I now compare the flags and rend_pk_digest and have pushed that.

comment:14 Changed 5 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_revisionmerge_ready

I believe this is now accurate! Thanks neel!

comment:15 Changed 4 weeks ago by nickm

Status: merge_readyneeds_revision

Hi! Thanks for this patch!

I have a request and a question and a concern:

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

The question: Does this apply to v3 onion services too? Should it?

The concern: I don't know if checking the "mixed" flags for equality is what we actually want here. (The flag "ISO_XYZ" is set in isolation_flags_mixed when the circuit have been used to connect streams with two different values for the "XYZ" parameter. Why are we looking at that?)

comment:16 in reply to:  15 Changed 4 weeks ago by teor

Replying to nickm:

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

I opened #27761 to catch bug fix changes files on future releases. But #27761 might not have caught this changes file, because bugfixes *can* happen on the most recent release.

comment:17 Changed 3 weeks ago by nickm

Priority: MediumHigh

comment:18 Changed 2 weeks ago by neel

Status: needs_revisionneeds_review

I have pushed an updated version which checks the individual isolation flags. However, it requires nested for loops and a goto statement. It is committed already in the same PR (https://github.com/torproject/tor/pull/311).

I have also backed up the previous branch (rejected) branch here: https://github.com/neelchauhan/tor/tree/b26368_old. The reason why I backed it up was because I squash commits is to neaten up my commit history for a patch.

comment:19 Changed 6 days ago by dgoulet

Milestone: Tor: 0.3.5.x-finalTor: unspecified
Status: needs_reviewneeds_revision

Neel, I left some comments.

TBH, I'm still unsure at this point of the right way to do this. These isolation flags are very confusing with regards on how they are used.

I left some pointers in the PR but I think we _really_ need a unit test here to 1) identity what we are trying to fix, 2) Make sure we are fixing it.

Also, we need to figure out how does this affect HS v3?

I'm moving this out of 035 for now to give us some time to understand it all. It could come back in 035 if we manage to fix this before stable is released in December.

comment:20 Changed 3 days ago by neel

Copied from GitHub: When I check if a oc has at least one isolation flag, should I check for *any* isolation flag, or a specific one?

If it is the former, I am thinking about something like this:

diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c
index 10b67ceda..dc2d33281 100644
--- a/src/feature/rend/rendclient.c
+++ b/src/feature/rend/rendclient.c
@@ -361,10 +361,21 @@ rend_client_close_other_intros(const uint8_t *rend_pk_digest)
       origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(c);
       if (oc->rend_data &&
           rend_circuit_pk_digest_eq(oc, rend_pk_digest)) {
-        log_info(LD_REND|LD_CIRC, "Closing introduction circuit %d that we "
-                 "built in parallel (Purpose %d).", oc->global_identifier,
-                 c->purpose);
-        circuit_mark_for_close(c, END_CIRC_REASON_IP_NOW_REDUNDANT);
+        int has_isolation_flag = 0;
+        for (edge_connection_t *oc_stream = oc->p_streams; oc_stream != NULL;
+             oc_stream = oc_stream->next_stream) {
+          if (EDGE_TO_ENTRY_CONN(oc_stream)->entry_cfg.isolation_flags) {
+            has_isolation_flag = 1;
+            break;
+          }
+        }
+
+        if (!has_isolation_flag) {
+          log_info(LD_REND|LD_CIRC, "Closing introduction circuit %d that we "
+                   "built in parallel (Purpose %d).", oc->global_identifier,
+                   c->purpose);
+          circuit_mark_for_close(c, END_CIRC_REASON_IP_NOW_REDUNDANT);
+        }
       }
     }
   }

The code in this comment seems much simpler than the code in the branch. Would that be okay? If so, the above code will replace the code in this branch.

Also if the above code is okay, will we still need a unit test?

If it is the latter, which flag should I check for?

comment:21 Changed 2 days ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Note: See TracTickets for help on using tickets.