Opened 16 months ago

Last modified 7 weeks ago

#26368 needs_revision defect

Consider circuit isolation when closing redundant intro points

Reported by: sysrqb Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-client, tbb-needs, 041-can
Cc: gk, mahrud 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 (31)

comment:1 Changed 16 months ago by gk

Cc: gk added

comment:2 Changed 16 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 16 months ago by arma

Cc: mahrud added

comment:4 Changed 15 months ago by nickm

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

comment:5 Changed 15 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 14 months ago by neel

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

comment:7 Changed 13 months ago by neel

Status: assignedneeds_review

comment:8 Changed 13 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:9 Changed 13 months 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 13 months ago by neel

I have pushed these changes.

comment:11 Changed 13 months ago by neel

Status: needs_revisionneeds_review

comment:12 in reply to:  10 Changed 13 months 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 13 months ago by neel

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

comment:14 Changed 13 months ago by dgoulet

Reviewer: dgoulet
Status: needs_revisionmerge_ready

I believe this is now accurate! Thanks neel!

comment:15 Changed 13 months 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 13 months 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 13 months ago by nickm

Priority: MediumHigh

comment:18 Changed 13 months 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 12 months 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 12 months 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 12 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final

comment:22 Changed 12 months ago by neel

I have pushed the new branch. Same PR.

I don't believe I can write a unit test because of circuit_mark_for_close(). This function is mocked in other unit tests, but the mocked function is a dummy function that basically eliminates the functionality which needed by rend_client_close_other_intros() (which is closing circuits without isolation flags).

A mocked function for circuit_mark_for_close() looks something like this (code snippet from src/test):

/* Mock function because we are not trying to test the close circuit that does
 * an awful lot of checks on the circuit object. */
static void
mock_circuit_mark_for_close(circuit_t *circ, int reason, int line,
                            const char *file)
{
  (void) circ;
  (void) reason;
  (void) line;
  (void) file;
  return;
}

comment:23 Changed 12 months ago by neel

Status: needs_revisionneeds_review

comment:24 Changed 12 months ago by dgoulet

Status: needs_reviewneeds_revision

I discussed this with Nick a bit on IRC and here is the reality with isolation flags. Here is the gist quoting him:

<+nickm> so, the isolation flags aren't really meaningful on their own...
<+nickm> An isolation flag ISO_FOO on a stream means "this stream must be isolated from all other streams with a different FOO"
<+nickm> but if stream 1 has ISO_FOO set, and stream 2 has a different value for "foo", then they can't share a circuit
<+nickm> If stream 1 has ISO_DESTPORT set, it can still share a circuit with stream 2 if stream 2 has the same value for its destport

That being said, Nick also pointed out to me this wonderful function: connection_edge_compatible_with_circuit() that should tell you if you can use the circuit or not for the given connection (in this case the SOCKS conn).

Sorry for all this confusion... it wasn't clear to me either how to proceed.

About the unit tests, you can do your own MOCKed function instead of using the one already in the unit test. Just add a new one with a meaningful name and do whatever you want in there.

comment:25 Changed 12 months ago by teor

Keywords: 035-roadmap-proposed removed

Remove 035-roadmap-proposed from items in 0.3.6.

comment:26 Changed 11 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:27 Changed 9 months ago by neel

Cc: neel removed
Owner: neel deleted
Status: needs_revisionassigned

comment:28 Changed 9 months ago by neel

Status: assignedneeds_revision

comment:29 Changed 8 months ago by dgoulet

Keywords: 040-can added
Priority: HighLow

Bug triage of 0.4.0 tickets. These are now in the "CAN" section. Lower priority than "040-must".

comment:30 Changed 5 months ago by nickm

Keywords: 041-can added; 040-can removed
Milestone: Tor: 0.4.0.x-finalTor: unspecified

comment:31 Changed 7 weeks ago by cypherpunks

Why was it removed from the milestones?

Tor should only close circuits if the socks request isolation bits match.

What is unclear here?
Right now FPI can be broken by embedding an onion site.

Note: See TracTickets for help on using tickets.