Using 'mytorexitnode.exit' request when mytorexitnode is both exit and client

Yesterday I ran an exit node called 'testnoderh' on my Tor instance and then sent a HTTP request through the same tor instance for 'www.google.com.testnoderh.exit'.

Tor didn't like that one bit. It built dozens of circuits very quickly and got nowhere with the request.

The debug log is 120MB unzipped, 2Mb zipped.

It's stored at: http://roberthogan.net/stuff/tor-goes-nuts.tar.bz2

added component::core tor/tor owner::mwenge priority::high resolution::fixed status::closed type::defect version::tor 0.2.2.12-alpha labels

My torrc was:

Nickname testnoderh ExitPolicy accept 129.79.247.195:* ExitPolicy reject : AllowDotExit 1 ORPort 9030 ControlPort 9051

'google.com' was a for-instance. Since I've supplied a full log and torrc for the actual failure I should be clear that the test requests were to www.cs.indiana.edu, which was permitted by the node's exit policy.

So here's my idea for a patch. I'm not sure if that can break in other cases, but it fixes the problem.

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 8808f56..f8b2b65 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -2408,12 +2408,16 @@ router_get_by_hexdigest(const char *hexdigest)
 routerinfo_t *
 router_get_by_digest(const char *digest)
 {
+  routerinfo_t * res = router_get_my_routerinfo();
+
   tor_assert(digest);
 
   if (!routerlist) return NULL;
 
   // routerlist_assert_ok(routerlist);
 
+  if (res && !memcmp(res->cache_info.identity_digest, digest, DIGEST_LEN))
+    return res;
   return rimap_get(routerlist->identity_map, digest);
 }

I chose this approach because adding ourselves to the routerlist seemed to be something we don't want to do, so we don't choose ourselves for circuits. If some exit is specifically requested however, we can still look it up this way.

Trac:
Status: new to needs_review

#641 (moved) the same exactly. "closed defect: Fixed" ?!

Trac:
Username: yetonetime

Well, either we closed the report erroneously two years ago or the bug was reintroduced. Does that change anything?

No need to change router_get_by_digest(), too late, to much affect.

Trac:
Username: yetonetime

Can you point out the problem; or do you have a suggestion for what to do instead?

Need to change build_state_get_exit_router(). The changes is safe but still it's a slighty late for such conditions, ri returned by build_state_get_exit_router() treats like element of global routerlist.

Most safe to change circuit_is_acceptable() and circuit_stream_is_being_handled() and then look at missed anything else.

Trac:
Username: yetonetime

As related:

  if (server_mode(get_options()) &&
      !strcasecmp(nickname, get_options()->Nickname))
    return router_get_my_routerinfo();

router_get_by_nickname(), returned routerinfo_t as element of routerlist nothing with compare with it.

Trac:
Username: yetonetime

Replying to Sebastian:

So here's my idea for a patch. I'm not sure if that can break in other cases, but it fixes the problem.

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 8808f56..f8b2b65 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -2408,12 +2408,16 @@ router_get_by_hexdigest(const char *hexdigest)
 routerinfo_t *
 router_get_by_digest(const char *digest)
 {
+  routerinfo_t * res = router_get_my_routerinfo();
+
   tor_assert(digest);
 
   if (!routerlist) return NULL;
 
   // routerlist_assert_ok(routerlist);
 
+  if (res && !memcmp(res->cache_info.identity_digest, digest, DIGEST_LEN))
+    return res;
   return rimap_get(routerlist->identity_map, digest);
 }

}}}

I had:

{{{ diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index e5e7d22..4f75a6d 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -3176,6 +3176,9 @@ build_state_get_exit_router(cpath_build_state_t *state) { if (!state || !state->chosen_exit) return NULL; +

• if (router_digest_is_me(state->chosen_exit->identity_digest))
• return router_get_my_routerinfo(); return router_get_by_digest(state->chosen_exit->identity_digest); }

which does the same I think. One odd problem I encountered was that, although it fixed the circuit-building DOS, it resulted in a circuit being chosen with the wrong exit. Tor had tried to build a four-hop circuit with my Tor instance as the exit but the patch resulted in it using a three-hop circuit exiting somewhere else.

I believe this is what yetonetime is alluding to. At least connection_ap_can_use_exit() relies on the presence of the exit in the routerlist with:

routerinfo_t *chosen_exit =
  router_get_by_nickname(conn->chosen_exit_name, 1);

It looks to me like supporting the scenario in the bug is a bit of a losing battle. Tor should probably fail gracefully I think and we don't have a patch for that yet.

Failing gracefully is not an option imo, because this is an anonymity attack if you operate an exit node and use the node as a client.

Replying to Sebastian:

Failing gracefully is not an option imo, because this is an anonymity attack if you operate an exit node and use the node as a client.

By failing gracefully I meant: 'You've requested to exit at your own node, request denied.'.

Yes, this is what I'm talking about. The website can make you attempt to use your own exit node and observe.

Just remove it:

  if (server_mode(get_options()) &&
      !strcasecmp(nickname, get_options()->Nickname))
    return router_get_my_routerinfo();

All is going on. Messages like "Requested exit point 'testnoderh' is not known." are well enough.

Trac:
Username: tractor

It must be backported too. 0.2.1.x allows dotexit by default, a some exit relays used as client could be affected by attack.

Trac:
Username: tractor

So, a bit of new info. We closed #641 (moved) because using .exit notation with your own exit works after you have seen your own descriptor in the consensus and added it to the routerlist. So this bug is only about the time between startup and becoming a part of the consensus for the first time.

What we should do here is treat the request like it requested an unknown relay, and fail quickly.

When I looked into the history, I noticed that for both router_get_by_nickname() and router_get_by_digest() we once had the ability to return our own routerinfo, independent of whether it had gotten into the consensus or not. This was introduced in 8efb2a957. In 9c67ae34f106, however, the ability to return your own routerinfo was removed from router_get_by_digest().

nickm and arma should look into which behaviour we actually want. Being inconsistent clearly causes problems here.

Are you reading posts before, or why am I writes that at all?

8efb2a957 is 2005 year. You can't return anything than routerlist's element by router_get_by_*, else it produces unknown edge cases that can happen with exist or a some new funcs or code.

Trac:
Username: tractor

The name of r14281(r14297) is "kludge". if router_get_by_nickname() has been worked as planned by design without non smartlist's elemens then #641 (moved) was never happened at all.

Trac:
Username: tractor