Opened 9 years ago

Closed 8 years ago

Last modified 6 years ago

#1859 closed defect (fixed)

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

Reported by: mwenge Owned by: mwenge
Priority: High Milestone:
Component: Core Tor/Tor Version: Tor: 0.2.2.12-alpha
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (46)

comment:1 Changed 9 years ago by mwenge

My torrc was:

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

comment:2 Changed 9 years ago by mwenge

'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.

comment:3 Changed 9 years ago by 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);
 }

comment:4 Changed 9 years ago by Sebastian

Status: newneeds_review

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.

comment:5 Changed 9 years ago by yetonetime

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

comment:6 Changed 9 years ago by Sebastian

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

comment:7 Changed 9 years ago by yetonetime

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

comment:8 Changed 9 years ago by Sebastian

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

comment:9 Changed 9 years ago by yetonetime

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.

comment:10 Changed 9 years ago by 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.

comment:11 in reply to:  3 Changed 9 years ago by mwenge

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.

comment:12 Changed 9 years ago by 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.

comment:13 in reply to:  12 Changed 9 years ago by mwenge

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.'.

comment:14 Changed 9 years ago by Sebastian

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

comment:15 Changed 9 years ago by tractor

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.

comment:16 Changed 9 years ago by 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.

comment:17 Changed 9 years ago by Sebastian

So, a bit of new info. We closed #641 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.

comment:18 Changed 9 years ago by tractor

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.

comment:19 Changed 9 years ago by 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 was never happened at all.

comment:20 Changed 9 years ago by tractor

Routerinfo_t returned by router_get_by_nickname() while it's not a part of routerlist have been just a bug on the fact.
For an attack will use the conditions under which this pseudo-element list is returned
at:

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

combining a role of client and an exit relay allows an attacker to identify a relay that victim used as OP.

The most simple scenario of such a case includes:
relay (0.2.1.x or 0.2.2.x with allowed dotexit) used as a client,
nickname of relay selected as (conflicts) that the auths assigns the Unnamed flag to it.

Such client will be the only one who can use own exit relay with Unnamed flag.

We can assume that the scenario is unlike in wild: does not affect clients in general and a small part of relay only which are probably no one will be used simultaneously with the OP. This is true.
But the mistake does not cease to be a mistake, an extreme edge case of very near to those who are could be with non zero chance susceptible to such attacks or to any a new bugs as a result of such behavior.

comment:21 in reply to:  20 ; Changed 9 years ago by mwenge

Replying to tractor:

Routerinfo_t returned by router_get_by_nickname() while it's not a part of routerlist have been just a bug on the fact.
For an attack will use the conditions under which this pseudo-element list is returned
at:

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

combining a role of client and an exit relay allows an attacker to identify a relay that victim used as OP.

You're right, but the above is a separate bug I think. (And an extremely good spot!) It will happen any time a someone running a default-configured Vidalia exit and a client clicks or uses a link ending 'Unnamed.exit'. They'll end up using their own exit as exit. Ouch.

If the solution to this bug doesn't fix it, it should be opened as a new bug. It probably merits separate consideration.

comment:22 Changed 9 years ago by nickm

I think there could maybe be cases when it makes sense to allow router_get_by_nickname() to bypass the routerlist: if you mention your own router by nickname in your configuration file, for instance.

But for .exit, it definitely doesn't.

So we either want to remove the offending code entirely, or split router_get_by_nickname() into two functions, one for local use and one for externally visible use.

There are 16 callers of router_get_by_nickname(); time to start reviewing them...

comment:23 Changed 9 years ago by nickm

circuituse.c, connection_edge.c : both are in reference to exit names or (bizarrely) extend_info.nickname.

control.c: used for circuit building or descriptor retrieval.

rendclient.c: used to look up routers from extend_info.
rendservice.c: Used to find router from introduce2 cell

router.c: Already special-cased to detect own name; used building MyFamily.

routerlist.c: Used in routerlist_add_family to add a router's family.

add_nickname_list_to_smartlist.c: Called lots of places, need to investigate.
routerset_get_all_routers:Called lots of places, need to investigate.

So next step is to review the callers of routerset_get_all_routers() and add_nickname_list_to_smartlist. The others would definitely benefit from removing the special-case.

comment:24 in reply to:  21 Changed 9 years ago by tractor

Replying to mwenge:

You're right, but the above is a separate bug I think. (And an extremely good spot!) It will happen any time a someone running a default-configured Vidalia exit and a client clicks or uses a link ending 'Unnamed.exit'. They'll end up using their own exit as exit. Ouch.

If the solution to this bug doesn't fix it, it should be opened as a new bug. It probably merits separate consideration.

router_get_by_nickname() returns NULL for Unnamed nickname, default-configured Vidalia exit relays non affects by this case. Only those relays with Unnamed flag in consensus.

There are could some another edges which probably missed, that was most simple for imagination.

comment:25 Changed 9 years ago by nickm

add_nickname_list_to_smartlist is used in routerlist_add_family().
routerset_get_all_routers() is used in getting entry nodes into the list of known entries in entry_guards_prepend_from_config.

So these two add routers from the user's configuration, whereas the others potentially add routers based on external information.

I think the right thing to do is:

  • Special-case those two functions to check for the own-nickname case
  • Change router_get_by_nickname to not check for the own-nickname case
  • Add a router_get_by_extend_info(), and use that everywhere we currently use router_get_by_nickname(extend_info->nickname)

tractor just noted that router_get_by_nickname checks for UNNAMED_ROUTER_NICKNAME early, so the Unnamed bug doesn't manifest quite so easily. Nevertheless, there are still ways to exploit this, so we should fix it.

comment:26 Changed 9 years ago by arma

Milestone: Tor: 0.2.2.x-final
Priority: normalminor

Moving back to 0.2.2.x as a milestone, so it looks like it's potentially security-related; but leaving as 'minor', so we can punt it as needed.

comment:27 Changed 9 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

Moving out of needs_review for now; a new patch is needed.

comment:28 Changed 9 years ago by mwenge

Owner: changed from nickm to mwenge
Status: acceptedassigned

comment:29 Changed 9 years ago by mwenge

(Summary of all of the above)

There are two problems in this bug:

  1. When an OP makes a .exit request specifying itself as the exit, and the exit is not yet listed, Tor gets all the routerinfos needed for the circuit but discovers in circuit_is_acceptable() that its own routerinfo is not in the routerdigest list and cannot be used. Tor then gets locked in a cycle of repeating these two steps. When gathering the routerinfos for a circuit, specifically when the exit has been chosen by .exit notation, Tor needs to apply the same rules it uses later on when deciding if it can build a circuit with those routerinfos.
  1. A different bug arises in the above situation when the Tor instance's routerinfo *is* listed in the routerlist, it shares its nickname with a number of other Tor nodes, and it does not have 'Named' rights to its nickname. So for example, if (i) there are five nodes named Bob in the network, (ii) I am running one of them but am flagged as 'Unnamed' because someone else claimed the 'Bob' nickname first, and (iii) I run my Tor as both client and exit the following can happen to me:
    • I go to www.evil.com
    • I click on a link www.evil.com.bob.exit
    • My request will exit through my own Tor node rather than the 'Named' node Bob or any of the others.
    • www.evil.com now knows I am actually browsing from the same computer that is running my 'Bob' node

As yetonetime points out all this nastiness happens because of:

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

in router_get_by_nickname().

So to solve both issues we need to ensure:

  • When fulfilling a .exit request we only choose a routerinfo if it exists in the routerlist, even when that routerinfo is ours.
  • When getting a router by nickname we only return our own router information if it is not going to be used for building a circuit.

comment:31 Changed 9 years ago by nickm

Reviewing:

  • In the future, please use an nice narrow linewidth for your commit messages. Pretend it's a text email. :)
  • In circuit_discard_optional_exit_enclaves, instead of get_by_hexdigest you need to say router_get_by_digest(): a hex digest is only going to work if it's hex-encoded, which extend-info identity_digest isn't.
  • Same problem for rend_client_get_random_intro: you're doing a get_by_hexdigest on identity_digest. Also, identity_digest might not be set yet! The point of that block is to go from a name for the intro point (which might be all we have from some hidden server descriptor versions) to its extend_info.
  • Also in rend_client_get_random_intro: That one was a get_by_nickname because, before 0.1.2.18/0.2.0.7-alpha, hidden services could list their introduction points by nickname. I think it might be okay to break hidden services running from such old Tors, but I'm not certain.

comment:32 in reply to:  31 Changed 9 years ago by Sebastian

Replying to nickm:

  • Also in rend_client_get_random_intro: That one was a get_by_nickname because, before 0.1.2.18/0.2.0.7-alpha, hidden services could list their introduction points by nickname. I think it might be okay to break hidden services running from such old Tors, but I'm not certain.

I agree that it is ok; because these old Tors will only publish to tor26 (the only remaining hs authority). This means that 0.2.2.x can't reach these hidden services anyways.

comment:33 Changed 9 years ago by mwenge

Updated my branch per nickm and Sebastian's comments.

comment:34 Changed 9 years ago by nickm

Looking good to me. I'll see if I can get arma to have a look too. Else, let's merge it.

tractor: do you see anything still missing here with mwenge's updated branch?

comment:35 Changed 9 years ago by nickm

arma says he won't have a chance to review for a while, so I'll go ahead.

I think there is a remaining bug, as noted above:

Also, identity_digest might not be set yet! The point of that block is to go from a name for the intro point (which might be all we have from some hidden server descriptor versions) to its extend_info

Fixing, merging to 0.2.2, fwd-porting to master. Should we backport this to 0.2.1? I think "maybe".

comment:36 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.1.x-final

comment:37 Changed 9 years ago by arma

"bugfix on 0.2.0-alpha."? that's not a version. :(

comment:38 Changed 9 years ago by arma

Nick concluded "If we say that the first version to special-case our own nickname was the one with the bug, that's git 8efb2a95, aka svn r3702, which works out to 0.1.0.1-rc"

comment:39 Changed 9 years ago by Sebastian

A good point from irc:

<piebeer> Why fixes bug 1859 is minor at least for maint-0.2.1? The only one stable is 0.2.1.x (with dotexit), btw.

comment:40 Changed 9 years ago by nickm

Priority: minormajor

Bumping the priority. In practice, we haven't been using priorities as much as we should; the milestone is more significant. Still, twiddling the priority doesn't hurt anything.

comment:41 Changed 9 years ago by nickm

Attempted backport in branch "bug1859_021"; needs review.

comment:42 Changed 9 years ago by cypherpunks

Are you warned operators of 0.2.1.x do not use themself as OP if they have non unique nickname by the chance?

comment:43 Changed 8 years ago by arma

Nick looked at this and thought it was ok? Then I'm fine putting it in.

I still haven't looked at it enough to have any strong guesses about whether it will break something unexpected. For example, I worry about some edge case that lets you crash a Tor by giving it a link to follow. But then, it's a privacy issue as-is, and 0.2.1 is the branch where dot-exit is enabled by default, which is why we're still considering this for backport at all. (How different is the backported version from the one that's been in 0.2.2 for a while?)

comment:44 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

AFAICT, the backported stuff is all in 0.2.2.x and has been for a while. I'm merging.

comment:45 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:46 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.