Opened 2 months ago

Closed 8 weeks ago

#33693 closed defect (fixed)

snowflake's 0.0.3.0 dummy address means rate limits are skipped means BW controller events show no bandwidth used

Reported by: arma Owned by: cohosh
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-9.5a10
Cc: cohosh, phw, arlolra, dcf Actual Points:
Parent ID: #19001 Points:
Reviewer: Sponsor:

Description

Snowflake's default bridge pretends to be on 0.0.3.0. It's a dummy address since snowflake-client knows how to connect to the right bridge and ignores the address that Tor tells it.

But my Tor client still uses that bridge address to make decisions. For example, connection_is_rate_limited() decides "no, it isn't rate limited", because tor_addr_is_internal() says 0.0.3.0 is essentially part of localhost. And that choice has a cascading effect where when I attach my nyx to Tor Browser to graph bandwidth use (nyx -i 9151), the BW events all say "0 0" because my Tor hasn't sent or received any non-internal bytes.

The quick fix is to keep using a dummy address, but to pick one that isn't an internal address. I confirmed that if I change snowflake's dummy address to 11.0.3.0, then connection_is_rate_limited() decides it's external, my BW events work again, and nyx gives me graphs. That is, Tor is smart enough to know that even though the connection is from the Tor client to the localhost snowflake client, the connection is "really" to the (non-localhost) destination bridge address.

I confess that I don't know which "apparently routable but don't worry we won't actually connect to it, probably" address is the best choice here. :/

The longer term answer is to have some other way to signal that it's a dummy address, or to change the PT interface so we don't need the fake address. But I don't think we need to wait for the longer term answer here.

The reason I noticed this issue is because I am pondering lobbying for the Tor Browser folks to give me a tiny bandwidth graph (or activity spinner) somewhere in the browser, because I got a super slow snowflake, but I was still getting 5-10KBytes/s, and my page did load after like 90 seconds, but if I hadn't been staring at the

2020/03/23 09:33:05 Traffic Bytes (in|out): 9018 | 10981 -- (27 OnMessages, 24 Sends)

lines I would have assumed that it was wedged.

Child Tickets

Attachments (1)

0001-Bug-33693-Change-dummy-addresses-for-snowflake-and-m.patch (2.0 KB) - added by cohosh 8 weeks ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 months ago by arma

Once we resolve this one to our satisfaction, meek has a similar issue with its 0.0.2.0 dummy address.

comment:2 Changed 2 months ago by cohosh

Look at this helpful comment for the connection_is_rate_limited() function:

/** Return 1 if we should apply rate limiting to <b>conn</b>, and 0
 * otherwise.
 * Right now this just checks if it's an internal IP address or an
 * internal connection. We also should, but don't, check if the connection
 * uses pluggable transports, since we should then limit it even if it
 * comes from an internal IP address. */
static int
connection_is_rate_limited(connection_t *conn)
{
  const or_options_t *options = get_options();
  if (conn->linked)
    return 0; /* Internal connection */
  else if (! options->CountPrivateBandwidth &&
           (tor_addr_family(&conn->addr) == AF_UNSPEC || /* no address */
            tor_addr_family(&conn->addr) == AF_UNIX ||   /* no address */
            tor_addr_is_internal(&conn->addr, 0)))
    return 0; /* Internal address */
  else
    return 1;
}

comment:3 Changed 2 months ago by cohosh

Out of curiosity, are there any cases in which a non-PT address would be in AF_USPEC here? Could we just fix this by removing that condition?

comment:4 Changed 2 months ago by dcf

Great find, arma. Here is some background on this and related issues.

  • #18517 meek is broken in Tor Browser 6.0a3
    Commit 23b08890 in tor temporarily broke meek's 0.0.2.0 address in the course of trying to fix unrelated bugs. It added a test for tor_addr_is_internal to circuit_handle_first_hop. There's discussion there of what address to use that tor won't consider internal--it's not guaranteed that tor will ever permanently reserve a safe set of addresses for us--but the best candidate seems to be 192.0.2.0/24, a range reserved for documentation. In the end, tor's code was changed to allow bridges on internal addresses, which resolved the problem even though meek doesn't actually connect to a bridge on an internal address.
  • #18611 Improve semantics for pluggable transports with dummy addresses
    Was an attempt to formalize the situation around dummy addresses, and provide guidance as to what transport authors should do if they don't use the address field in the bridge line. phw suggested an implicit token in the bridge line that would tell tor not to consider that the bridge line address is actually being connected to.
  • #19487 Meek and ReachableAddresses
    A related address-handling bug: if you configure ReachableAddresses such that a dummy address is considered not reachable, tor will not even try connecting to the transport. (Not knowing that the transport has no intention of trying to connect to that address.) teor has a patch there to ignore ReachableAddresses for bridges with transports, but it fell off the review pipeline.
  • #25528 When ClientTransportPlugin is missing, tor connects directly to bridge addresses, even if they have a transport name
    Shows the danger of putting a routable address in a bridge line, even if ignored by the transport. If there are errors elsewhere in torrc, tor may make a direct TCP connection to whatever address is there, instead of passing it to a pluggable transport proxy. This is what I was trying to prevent by using an obviously invalid address like 0.0.3.0: in the event of error, tor still tries to connect directly to the address, but it fails at the OS level with an "Invalid argument" error and no traffic leaves the machine.

And some history on how we arrived at the 0.0.X.0:1 pattern for dummy addresses. With flash proxy back in 2011, there was originally was no such thing as ClientTransportPlugin or transport-aware bridge lines, so you had to fake it with Socks4Proxy and a SOCKS-speaking local transport program. It happened that the address of the local SOCKS proxy was 127.0.0.1:9001, and I put that address in both Socks4Proxy and Bridge lines:

https://gitweb.torproject.org/flashproxy.git/tree/README?id=d40953b0b2164548d06964f2e98087e2dd528f31#n53

UseBridges 1
Bridge 127.0.0.1:9001
Socks4Proxy 127.0.0.1:9001

Later I decided it was confusing to have the same address in both places when one of them is ignored and one is not, so I changed the ignored one to 0.0.0.0:1. I didn't document why I chose the port number 1, but I'm pretty sure it's because tor internally uses port 0 as a sentinel in some places.

https://gitweb.torproject.org/flashproxy.git/commit/?id=bf51cf0b761214e39f635d14451582d27a8915ab

But 0.0.0.0 didn't work well because tor uses an IPv4 address of 0 as a sentinel in various places. I think I first tried 0.0.0.1 after that, but that one is no good because 0.0.0.nonzero is a SOCKS4a magic value indicating a hostname. So I switched it to 0.0.X.0:1.

https://gitweb.torproject.org/flashproxy.git/commit/?id=fdcb2d1382b430565afbcd5dcfad829479c0bb7b

Since then I've been incrementing X with each new transport that uses dummy addresses. This is because if we use the same dummy address for two transports that actually connect to different bridges, tor will complain about seeing two different fingerprints for the "same" bridge, because internally it keys its tables by IP:port.

Last edited 2 months ago by dcf (previous) (diff)

comment:5 in reply to:  3 Changed 2 months ago by arma

Replying to cohosh:

Out of curiosity, are there any cases in which a non-PT address would be in AF_USPEC here? Could we just fix this by removing that condition?

To be clear, I think the 0.0.3.0 bridge is triggering that middle condition: the tor_addr_is_internal() one. That's because 0.0.3.0 is an internal ipv4 address.

comment:6 Changed 2 months ago by arma

[...nothing to see here...]

Last edited 2 months ago by arma (previous) (diff)

comment:7 Changed 2 months ago by cohosh

Owner: set to cohosh
Parent ID: #19001
Status: newassigned

Adding this to the list of things we should address before Snowflake is out of alpha.

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

Replying to arma:

The reason I noticed this issue is because I am pondering lobbying for the Tor Browser folks to give me a tiny bandwidth graph (or activity spinner) somewhere in the browser, because I got a super slow snowflake, but I was still getting 5-10KBytes/s, and my page did load after like 90 seconds

I have filed this Tor Browser feature request: #33746.

comment:9 in reply to:  2 Changed 2 months ago by arma

Replying to cohosh:

Look at this helpful comment for the connection_is_rate_limited() function:

/** Return 1 if we should apply rate limiting to <b>conn</b>, and 0
 * otherwise.
 * Right now this just checks if it's an internal IP address or an
 * internal connection. We also should, but don't, check if the connection
 * uses pluggable transports, since we should then limit it even if it
 * comes from an internal IP address. */

Huh. I don't think that's true. Who wrote that garbage?! git blame says it was...that arma guy. What does he know. :) (git commit f6639d56 for those following along at home)

In particular, I think that comment was aiming to talk about the *bridge* side, and it was true before the ExtORPort existed. Now connections from the ExtORPort report their external address, and Tor treats the connection as having come to the ORPort from that external address, so it gets proper bandwidth accounting and proper rate limiting. (See connection_ext_or_handle_cmd_useraddr() for where this happens.)

That said, there is an edge case where you can use ExtORPort wrong, and #33157 hit it in Snowflake, so I have opened #33747 to handle it better on the Tor side.

comment:10 Changed 8 weeks ago by cohosh

Status: assignedneeds_review

Thanks dcf for all that background, that was extremely useful.

I wrote a patch to change the dummy addresses to the 192.0.2.0/24 range. And kept up a pattern of mapping the X in the original 0.0.X.0 address to 192.0.2.X.

I checked to make sure it built in the latest version of Tor Browser, and that nyx -i 9151 now works.

This works as a temporary fix but we should push forward with updating the way tor deals with new style PTs that don't have traditional bridge lines. #18611 seems like a good ticket to track all that in.

comment:11 Changed 8 weeks ago by dcf

Status: needs_reviewmerge_ready

Looks great, thanks.

If you would, please also change the dummy addresses in client/torrc and client/torrc-localhost in the snowflake repo.

BTW the reason why the port number is 2 instead of 1 in meek-azure is that we were using a different port number for each meek backend. meek-google at 0.0.2.0:1 (never deployed), meek-amazon at 0.0.2.0:2, meek-azure at 0.0.2.0:3. Then here meek-azure took over port 2 when meek-amazon was removed.

comment:12 in reply to:  11 Changed 8 weeks ago by cohosh

Replying to dcf:

If you would, please also change the dummy addresses in client/torrc and client/torrc-localhost in the snowflake repo.

Done: https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=ea01bf41c3011590938b079ed96c7b35cb40588b

comment:13 Changed 8 weeks ago by cohosh

Component: Circumvention/SnowflakeApplications/Tor Browser

This is now a tor browser merge request

comment:14 Changed 8 weeks ago by sysrqb

Keywords: tbb-9.5a10 added

comment:15 Changed 8 weeks ago by sysrqb

Resolution: fixed
Status: merge_readyclosed

This is commit dfdf0e22bf9989952cae8c076e512bd898dac90a on tor-browser-build master.

Note: See TracTickets for help on using tickets.