Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5623 closed defect (fixed)

directory_initiate_command() can pick a directory mirror which we later refuse as being in ExcludeExitNodes

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

I set my torrc to be the same as in #5610, restarted my Tor, and it wanted to fetch a microdesc.

Apr 13 18:54:04.000 [info] launch_descriptor_downloads(): Launching 1 request for 1 router, 4 at a time
Apr 13 18:54:04.000 [debug] smartlist_choose_node_by_bandwidth_weights(): Choosing node for rule weight as directory based on weights Wg=0.307700 Wm=1.000000 We=0.000000 Wd=0.128000 with total bw 571593023.800000
Apr 13 18:54:04.000 [debug] directory_initiate_command_rend(): anonymized 0, use_begindir 1.
Apr 13 18:54:04.000 [debug] directory_initiate_command_rend(): Initiating microdescriptor fetch
Apr 13 18:54:04.000 [info] connection_ap_make_link(): Making internal direct tunnel to 188.138.82.143:443 ...
Apr 13 18:54:04.000 [debug] connection_add_impl(): new conn type Socks, socket -1, address (Tor_internal), n_conns 3.
Apr 13 18:54:04.000 [warn] Requested exit point '$2BC7B25AFFACBB861872248D3E19D77EE71CE1CE' is excluded or would refuse request. Closing.
Apr 13 18:54:04.000 [warn] Making tunnel to dirserver failed.

Looks like launch_descriptor_downloads() calls initiate_descriptor_downloads() which calls directory_get_from_dirserver() which calls router_pick_directory_server() which does not exclude exits.

I believe we don't want to exclude exits in this case. Quoting from ChangeLog,

        . "Exit", in the context of ExitNodes and ExcludeExitNodes, means
          a node that delivers user traffic outside the Tor network.

So I think the bug is in connection_ap_can_use_exit(). Probably where it says

  if (routerset_contains_node(options->_ExcludeExitNodesUnion, exit)) {
    /* Not a suitable exit. Refuse it. */
    return 0;
  }

without checking if conn->use_begindir.

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by arma

Description: modified (diff)

comment:2 in reply to:  description Changed 8 years ago by arma

Replying to arma:

without checking if conn->use_begindir.

I wonder if there's anything else we want to check there? I think no. That makes the suggested fix:

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index dd772b2..6b4f457 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -3368,8 +3368,12 @@ connection_ap_can_use_exit(const entry_connection_t *conn
     }
   }
 
-  if (conn->socks_request->command == SOCKS_COMMAND_CONNECT &&
-      !conn->use_begindir) {
+  if (conn->use_begindir) {
+    /* Internal directory fetches do not count as exiting. */
+    return 1;
+  }
+
+  if (conn->socks_request->command == SOCKS_COMMAND_CONNECT) {
     struct in_addr in;
     tor_addr_t addr, *addrp = NULL;
     addr_policy_result_t r;

I wonder if we want to check if routerset_contains_node(options->ExcludeNodes, exit) first? Or is that redundant with other checks?

comment:3 Changed 8 years ago by nickm

I wonder if we want to check if routerset_contains_node(options->ExcludeNodes, exit) first? Or is that redundant with other checks?

Should be redundant. Elsewhere in the function, right up to the end, we never return 1, only 0. And before we return 1, we check for routerset_contains_node(options->ExcludeExitNodesUnion,exit).

We should have a comment for that, I suppose: if it didn't jump out at you at your first scan through the function, it is obviously not obvious.

comment:4 Changed 8 years ago by nickm

Status: newneeds_review

comment:5 Changed 8 years ago by arma

Pushed a bug5623 branch to my arma.

From your comment I worry that you may be misinterpreting my question: the new behavior is that we *do* return 1 earlier in the function in this case.

Though after some more thought, I think this is not a reasonable place to spot-check whether there's some bug that allowed us to use a node in excludenodes.

comment:6 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Your arma is now part of my master, and by extension everybody's master.

wait, that sounds wrong

in other words, "merging; closing"

comment:7 Changed 8 years ago by nickm

Keywords: tor-client added

comment:8 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.