Opened 8 years ago

Closed 8 years ago

Last modified 3 years ago

#1751 closed task (implemented)

Project: Make it harder to use exits as one-hop proxies

Reported by: nickm Owned by:
Priority: Medium Milestone: Deliverable-Sep2010
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

Proposal 163 outlined some ways to detect clients, and later emails elaborated on the issue, clarifying some parts and exposing hopeless muddiness in others.

There's an initial implementation of a variant of the concept in Tor now, under the RefuseUnknownExits configuration option. Unfortunately, turning it on seems to give many positives. We haven't investigated whether they're false positives or just jerks using Tor as a one-hop proxy.

Child Tickets:

#1160
AllowSingleHopExits setting can be bypassed by client


Child Tickets

TicketStatusOwnerSummaryComponent
#1160closedAllowSingleHopExits setting can be bypassed by clientCore Tor/Tor

Change History (30)

comment:1 Changed 8 years ago by nickm

Description: modified (diff)

comment:2 Changed 8 years ago by nickm

Description: modified (diff)

comment:3 Changed 8 years ago by arma

I need somebody to run an exit relay at log-level debug and safelogging 0 for a while, with RefuseUnknownExits enabled. Then I'll have a better sense of what I need to learn in order to debug the false positives.

(Don't share your log publically, since destinations + timings are sensitive.)

comment:4 Changed 8 years ago by mikeperry

Can you specify which log messages you care about? Is it all debug, or can I reduce them somehow first?

comment:5 Changed 8 years ago by arma

     log_notice(LD_PROTOCOL,
                "Attempt by %s to open a stream %s. Closing.",

That's actually the one I want to see first. It's only if there are some weird outputs that I would need to see more.

But the trick is that I don't know what I'm looking for. My plan is to run the IPs that it logs through exonerator and see what comes out.

comment:6 in reply to:  5 Changed 8 years ago by karsten

Replying to arma:

But the trick is that I don't know what I'm looking for. My plan is to run the IPs that it logs through exonerator and see what comes out.

ExoneraTor is not quite the right tool here, because it needs detailed information about the timeframe to search. There's a new Relay Search page on metrics.tpo that is much faster in searching the descriptor archives and might be more suited for this task. Please let me know if it's missing any features you need for looking up IPs.

comment:7 Changed 8 years ago by Sebastian

I have a refuseunknown branch to at least document the RefuseUnknownExits branch in the manpage, since the code is in master.

comment:8 Changed 8 years ago by arma

Karsten has made available a relay search page:
https://metrics.torproject.org/relay-search.html

The next step is that we need some people to run exit relays with this feature on and safelogging off, and run their positives through the relay search page to get a sense of how many are 'real'.
For the ones that come up as false positives, we should try to figure out if there's a trend.

In theory publishing the false positives here should not be dangerous. They're Tor relays after all. Publishing the correct positives is probably rude though.

comment:9 Changed 8 years ago by Sebastian

I've been running fluxe3 for a few weeks now with RefuseUnknownExits turned on, and haven't gotten a single positive in the past weeks (after I had quite a few before). I didn't disable safelogging, so my theory is that I had a tortunnel user who decided to go away because my node didn't work anymore. I'd be happy to help analyze some other data though, if someone can make it available.

comment:10 Changed 8 years ago by jn

On two different exit relays I got a burst of eleven request from the same IP-address at almost the same time. The address is not found on the relay search page.
RefuseUnknownExits has been enabled for a long time on both exit nodes so most abusers could have learned by now that these relays is not working.

comment:11 Changed 8 years ago by nickm

Status: newneeds_review

Looks like we should merge the patch, make RefuseUnknownExits on by default (if it isn't), and call this closed. Putting this in needs_review just in case.

comment:12 Changed 8 years ago by Sebastian

We might want to enable the consensus logic, or decide that it isn't worth it and just enable it by default while keeping the torrc option. I prefer the latter. If we make that decision now, then we don't have to merge my manpage patch because we need to reword it

comment:13 Changed 8 years ago by murble

I've also been running for a few days with RefuseUnknownExists, on my exit (https://torstatus.all.de/router_detail.php?FP=47cb023356abd6c4b61ae950605566bcdf79c6c9)

None of the requests have been found in the tor relay search, and 66 of 67 over the past 24 hours have come from a single IP in bursts at various times of the day.

comment:14 in reply to:  12 Changed 8 years ago by arma

Replying to Sebastian:

We might want to enable the consensus logic, or decide that it isn't worth it and just enable it by default while keeping the torrc option. I prefer the latter. If we make that decision now, then we don't have to merge my manpage patch because we need to reword it

We need to add a kill switch in the consensus if we turn this feature on by default. Otherwise, we won't be able to move away from a clique network topology without all of our exit relays refusing to exit to many users.

comment:15 Changed 8 years ago by arma

So here's the next question to ponder. Once upon a time, we added a feature where if a relay refuses to exit to an address that we think his exit policy supports, we call
policies_set_router_exitpolicy_to_reject_all() which sets our view of his exit policy to reject *:*.

That was back when we got a new directory a couple of times an hour. So it was a temporary mod, to avoid using that relay until it's overwritten on our next directory update.

Then we made it so we only fetch new relay descriptors every 18 hours or so. And we kept the same hack.

So if an exit refuses you, you avoid him until you get a new descriptor -- on average 9 hours from now. So if we have a really high false positive rate on refuseunknownexits, clients could end up avoiding a large fraction of the network -- or even in extreme cases all of the network. Is "until we get a new descriptor" too long to wait?

We could send back some other end reason -- any of the ones listed in edge_reason_is_retriable(). That would send the user to some other circuit without marking that exit as a non-exit for hours afterward.

My main reason for picking reason exitpolicy here was if an exit relay for some reason has a higher false positive rate than the average exit relay. Then clients would avoid it more thoroughly.

Hm.

comment:16 Changed 8 years ago by Sebastian

Sounds like we should add a new reason here (edge_reason_is_retriable contains END_STREAM_REASON_MISC, so we don't end up hurting old Tors); and newer Tors could handle it in a way so that they log a protocol warning or something to let us know. I don't think we need to worry about having a higher false positive rate than on average. If we do, soat should catch it hopefully, as this would be more of a sign of a broken relay.

Relatedly we should include microdescriptors when thinking about how often descriptors are fetched, because with microdescs we're going to fetch them as soon as they changed and not for a long time if they didn't change.

comment:17 Changed 8 years ago by nickm

Adding a reason for this seems like a fine thing to me.

For configuration, a tristate torrc option whose default value is "do whatever the consensus says" seems right.

comment:18 in reply to:  17 Changed 8 years ago by arma

Replying to nickm:

Adding a reason for this seems like a fine thing to me.

Yet another case where we wish we'd sorted out and implemented that proposal to reserve half the reason space as retriable reason codes.

Oh hey, I just checked the code again, and apparently I already chose END_STREAM_REASON_MISC, not EXITPOLICY, as the reason code when we send back the end cell. I'm so smart. Nevermind my above comment about EXITPOLICY then.

Does that mean the current plan is to use MISC for now (and for the next couple years), but teach clients to recognize some other reason, and eventually plan to move to sending that reason, at which point clients will be able to discover that they're being refused due to thinking they're trying to cheat?

For configuration, a tristate torrc option whose default value is "do whatever the consensus says" seems right.

That does seem slightly smarter than what I was going to do, which was a boolean config option which turns it on no matter what when 1, and uses whatever's in the consensus when 0. If you have some implementation ideas in mind, please do run with them.

comment:19 Changed 8 years ago by nickm

See branch bug1751_enabling in my public. It is suspect, since I am tired.

comment:20 Changed 8 years ago by arma

arma@last-request:~/torgit/tor$ make check-spaces
./contrib/checkSpace.pl -C                    \
                src/common/*.h                        \
                src/common/[^asO]*.c src/common/address.c \
                src/or/[^e]*.[ch] src/or/eventdns_tor.h \
                src/test/test*.[ch] src/tools/*.[ch]
 DoubleNL:src/or/config.c:1243
Last edited 3 years ago by arma (previous) (diff)

comment:21 Changed 8 years ago by arma

trivial other things:

+  /* We need an up-to-date view of network info if we're going to try to
+   * block unknown exits. */

"...if we're going to try to block exit attempts from unknown relays."

In connection_exit_begin_conn() it could sure use a

  or_options_t *options = get_options();

at the top.

comment:22 Changed 8 years ago by arma

+  /** Whether we should drop exit streams from Tors that we don't know are
+   * relays.  One of "0" (never refuse), "1" (always refuse), or "auto" (do
+   * what the consensus says). -RD */

No need to leave my signature here. :)

Also, we should clarify here what it does when the consensus doesn't say what to do.

+  const char *RefuseUnknownExits;

This is the first or_options_t element that is a const char?

comment:23 Changed 8 years ago by arma

  int ConnLimit; /**< Demanded minimum number of simultaneous connections. */
  int _ConnLimit; /**< Maximum allowed number of simultaneous connections. */

vs

+  const char *RefuseUnknownExits;
+  /** Parsed version of RefuseUnknownExits. -1 for auto. */
+  int RefuseUnknownExits_;

Pick a convention: we want the underscore before or after? (I have a mild preference for before, but maybe that's simply because that's what you picked last time and I got used to it.)

comment:24 Changed 8 years ago by arma

+  } else if ((consensus = networkstatus_get_latest_consensus()) != NULL) {
+    return networkstatus_get_param(consensus, "refuseunknownexits", 1);
+  } else {
+    return 1;
+  }

You're in luck! You can hand networkstatus_get_param() NULL and it will look up the consensus, returning the default if no consensus.

Also, that means you should do something about referencing <b>consensus</b> in the function comment (I thought that was only for function arguments anyway).

So this could become

  }
  return networkstatus_get_param(NULL, "refuseunknownexits", 1);

comment:25 Changed 8 years ago by arma

I'm not sure what I think the default default should be. If there's no consensus or we forget to have enough active votes about that param, and we think the feature works, we want it to default to 1. But if we later decide there's a bug in how Tors do the blocking of exit requests, and we end up moving to a new behavior and a new consensus param name, then we would have to maintain the old consensus param just to declare "no don't do that if you're an old tor" for the next years.

So I would argue that we should make sure to have enough active votes about the param, not worry about the transient edge case where the relay doesn't have any consensus yet, and be defensive against the possibility that our heuristic in connection_exit_begin_conn() is not the final heuristic we want to choose.

On the other hand, it may well be the case that we improve the heuristic in future versions, but want to keep using the one we have now in present versions since it's good enough and better than nothing.

I'm not sure how to read the future here. So I guess either of them is ok, and we'll learn a lesson either way. It really depends whether we think this particular heuristic could go downright wrong down the road, as opposed to "not as right as it could be".

comment:26 Changed 8 years ago by arma

Ok. If you don't think any of this needs more discussion, green light to merge things whenever you like. Thanks!

comment:27 Changed 8 years ago by nickm

Merged a (revised) bug1751_enabling branch.

The point of the _ afterwards is that the C standard (if you want to be pedantic about it) says that identifiers starting with _ are reserved. I didn't care when we started. Now I care a little. I don't want to add new _foo identitiers. We should perhaps move to foo_ identifiers, or use different identifiers at all. Not sure when is a good time for that, though. Probably not now.

Does the merge close this ticket?

comment:28 Changed 8 years ago by arma

Resolution: implemented
Status: needs_reviewclosed

I set the refuseunknownexits=1 param on moria1.

I'm going to close this task as done. Woo.

comment:29 Changed 6 years ago by nickm

Keywords: tor-relay added

comment:30 Changed 6 years ago by nickm

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