Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#18517 closed defect (fixed)

meek is broken in Tor Browser 6.0a3

Reported by: gk Owned by: teor
Priority: Very High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: regression, must-fix-before-028-rc, meek, 2016-bug-retrospective
Cc: teor, dcf Actual Points:
Parent ID: Points:
Reviewer: Sponsor: None

Description

meek does not work any longer in Tor Browser 6.0a3. It seems this is caused by an underlying bug in tor. After some amount of testing and bisecting commit 23b088907fd23da417f5caf2b7b5f664f317ef4a is the first that introduces the new behavior. Trying to start meek with it results in

Mar 10 13:50:53.000 [notice] Ignoring directory request, since no bridge nodes are available yet.
Mar 10 13:50:54.000 [notice] Delaying directory fetches: No running bridges

and nothing thereafter: the startup is stalled.

Child Tickets

Attachments (1)

0001-Use-RFC-5737-TEST-NET-1-addresses-for-meek-dummy-add.patch (2.4 KB) - added by dcf 11 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 12 months ago by dcf

I suppose a workaround is to change the phony addresses in the bridge lines (0.0.2.0) to something else. 0.0.0.0/8 satisfies tor_addr_is_internal which is what 23b088907fd23da417f5caf2b7b5f664f317ef4a is checking for.

I tried editing Browser/TorBrowser/Data/Browser/profile.default/preferences/extension-overrides.js in an already installed Tor Browser and changed 0.0.2.0 to 1.2.3.4. That made it work.

The phony IP address can be almost anything. It is completely ignored. The reason it is not 0.0.0.0 is because tor uses 0.0.0.0 to mean "not set" internally. The reason it is not 0.0.0.[1-255] is because addresses of that form have a special meaning in SOCKS4a. 0.0.1.0 was used for flash proxy (another transport that ignores the address you provide it).

comment:2 Changed 12 months ago by nickm

  • Keywords must-fix-before-028-rc added

Marking these as must-fix-before-028-rc.

Actually, some of them may not need to get 'fixed' before the rc, but I believe that they should either get fixed, or we should have a good explanation of why they don't need to get fixed.

comment:3 Changed 11 months ago by nickm

  • Keywords regression added
  • Sponsor set to None

comment:4 follow-up: Changed 11 months ago by teor

  • Component changed from Tor to Tor Browser
  • Keywords must-fix-before-028-rc removed
  • Milestone Tor: 0.2.8.x-final deleted
  • Owner set to tbb-team
  • Priority changed from Medium to Very High
  • Version Tor: 0.2.8.1-alpha deleted

I think this is a Tor Browser issue and we should adopt dcf's workaround of changing the dummy IP addresses to publicly routable IP addresses.

Tor is correctly checking for internal addresses and refusing to build circuits to them. This is a bugfix on #17674 and #8976. Tor can't make an exception for Tor Browser's sentinel addresses, without also allowing relays and hidden services to mistakenly connect to those addresses. This would open up the same attack vector we're trying to fix here.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 11 months ago by dcf

Replying to teor:

I think this is a Tor Browser issue and we should adopt dcf's workaround of changing the dummy IP addresses to publicly routable IP addresses.

Could we get some guidance on what false IP addresses to use? It's a flaw in the PT spec that bridge specifications have to have an IP address, even if the transport doesn't use it. I tried to make the IP addresses as fake-looking as possible, to try and make it obvious that they are unused.

I don't think it's a good idea the IP address of whatever relay the PT eventually carries the traffic to, because it's not reflective of what's going on. If the bridge changes its IP address, the bridge line will keep working with the "wrong" address, and that's likely to be confusing.

Could we use something in e.g. the reserved 240.0.0.0/4 range? Or the 192.0.2.0/24 range reserved for examples? Maybe a bogus IPv6 address?

But by all means, let's apply any workaround in Tor Browser, because now 6.0a4 is out and presumably also has this problem.

comment:6 in reply to: ↑ 5 Changed 11 months ago by teor

Replying to dcf:

Replying to teor:

I think this is a Tor Browser issue and we should adopt dcf's workaround of changing the dummy IP addresses to publicly routable IP addresses.

Could we get some guidance on what false IP addresses to use? It's a flaw in the PT spec that bridge specifications have to have an IP address, even if the transport doesn't use it. I tried to make the IP addresses as fake-looking as possible, to try and make it obvious that they are unused.

The relevant lists of internal addresses in tor_addr_is_internal_ are:
IPv6:

    if (for_listening && !iph6[0] && !iph6[1] && !iph6[2] && !iph6[3]) /* :: */
      return 0;

    if (((iph6[0] & 0xfe000000) == 0xfc000000) || /* fc00/7  - RFC4193 */
        ((iph6[0] & 0xffc00000) == 0xfe800000) || /* fe80/10 - RFC4291 */
        ((iph6[0] & 0xffc00000) == 0xfec00000))   /* fec0/10 D- RFC3879 */
      return 1;

    if (!iph6[0] && !iph6[1] && !iph6[2] &&
        ((iph6[3] & 0xfffffffe) == 0x00000000))  /* ::/127 */

IPv4:

    if (((iph4 & 0xff000000) == 0x0a000000) || /*       10/8 */
        ((iph4 & 0xff000000) == 0x00000000) || /*        0/8 */
        ((iph4 & 0xff000000) == 0x7f000000) || /*      127/8 */
        ((iph4 & 0xffff0000) == 0xa9fe0000) || /* 169.254/16 */
        ((iph4 & 0xfff00000) == 0xac100000) || /*  172.16/12 */
        ((iph4 & 0xffff0000) == 0xc0a80000))   /* 192.168/16 */

I don't think it's a good idea the IP address of whatever relay the PT eventually carries the traffic to, because it's not reflective of what's going on. If the bridge changes its IP address, the bridge line will keep working with the "wrong" address, and that's likely to be confusing.

I agree. And you run the risk that the actual address is unavailable to Tor Browser, or that you retrieve the remote address and it's an internal address, and therefore blocked.

Could we use something in e.g. the reserved 240.0.0.0/4 range?

I'd advise against this, in case it's released for public use, or repurposed.

Or the 192.0.2.0/24 range reserved for examples? Maybe a bogus IPv6 address?

Yes, either of these options will currently be accepted by tor (return 0 from tor_addr_is_internal).

Over the long term, the list of internal addresses in tor may be expanded. It's worth selecting an address that we'll never include in that list, so we don't have to revisit this issue in a later release. (The most likely to be included in future is multicast (224.0.0.0/4), which we already block in some places in the code.)

Ideally, Tor Browser would use an address blackholed at the OS level, so we never have to or want to block it in tor. But there's nothing like this in IPv4, and IPv6 potentially routes its blackhole range to an IDS, which we never want to happen, even by accident.

It's worth noting that RFC 7600 reserves 192.0.0.8/32 as a dummy address, but only for the purpose of the IPv6 transition. So let's not use that, because it could be confusing. (See https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml )

Let's use the first documentation range: 192.0.2.0/24 (the others are 198.51.100.0/24 and 203.0.113.0/24). They're not exactly what we want, but they should be usable as a generic dummy address. Let's document the range we choose in the pluggable transport spec (this ticket?), and the tor source code (#18582), so we handle those addresses appropriately in future.

comment:7 follow-ups: Changed 11 months ago by teor

  • Component changed from Tor Browser to Tor
  • Keywords must-fix-before-028-rc added
  • Milestone set to Tor: 0.2.8.x-final
  • Version set to Tor: 0.2.8.1-alpha

Hmm, after thinking about this for a few days, I actually think picking another dummy IP address and hoping it keeps working in future is a brittle approach. I'd rather work out why tor is trying to connect to that address in the first place, when the pluggable transport should be the thing connecting to it, not tor.

(Am I understanding how pluggable transports work?)

I think what tor should do is:

  • if a transport is specified in the Bridge line, and that transport corresponds to a ClientTransportPlugin line, tor should pass the address in the bridge line to the pluggable transport, and not try to connect to that address itself. (The pluggable transport is free to use [obfsproxy] or ignore [meek] that address.)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 11 months ago by yawning

Replying to teor:

Hmm, after thinking about this for a few days, I actually think picking another dummy IP address and hoping it keeps working in future is a brittle approach. I'd rather work out why tor is trying to connect to that address in the first place, when the pluggable transport should be the thing connecting to it, not tor.

Because, the code assumes that every node has an IP address (reasonable, except for oddball PTs like meek). How else are you going to identify nodes before you bootstrap in the general case anyway? (The relay fingerprint on Bridge lines is optional, which doesn't leave many other options...).

(Am I understanding how pluggable transports work?)

I think what tor should do is:

  • if a transport is specified in the Bridge line, and that transport corresponds to a ClientTransportPlugin line, tor should pass the address in the bridge line to the pluggable transport, and not try to connect to that address itself. (The pluggable transport is free to use [obfsproxy] or ignore [meek] that address.)

This is what it used to do before it changed it, the "decide if need to pass it onto a PT" is done at the last possible moment. What your code change effectively does is add "before passing the address to the PT (if applicable), check that it is not internal" (See: connection_or.c:connection_or_connect()).

This change also breaks normal (non-PT) bridges hosted on an internal network, but I'm not sure if that's a common use case for actual users. It's rather common for me to run a bridge on the same host (communicating over the loopback interface) for testing, but I only need to edit 15 or so torrc files....

The solutions I can think of off the top of my head are:

  • Allow connections to *all* bridges on "internal addresses".
  • Allow connections to PT bridges on "internal addresses".
  • Pick a magic address for meek-like transports that bypasses the sanity check and document it as "the one true synthetic address to use".

Out of those the first two options are more appealing to me, and are trivial to do...

comment:9 in reply to: ↑ 7 Changed 11 months ago by dcf

(Conflicted with yawning's comment:8, posting anyway.)

Replying to teor:

Hmm, after thinking about this for a few days, I actually think picking another dummy IP address and hoping it keeps working in future is a brittle approach. I'd rather work out why tor is trying to connect to that address in the first place, when the pluggable transport should be the thing connecting to it, not tor.

(Am I understanding how pluggable transports work?)

I think what tor should do is:

  • if a transport is specified in the Bridge line, and that transport corresponds to a ClientTransportPlugin line, tor should pass the address in the bridge line to the pluggable transport, and not try to connect to that address itself. (The pluggable transport is free to use [obfsproxy] or ignore [meek] that address.)

Actually that's how it works now. tor does not connect to the address. (That makes sense, because the transport might not even use TCP, for example.) tor passes the address to the pluggable transport executable though a SOCKS interface, roughly the same as if tor were connecting through an upstream proxy.

In the case of obfs*, the bridge IP address also happens to be the address that the transport client should connect to, and it's an ordinary TCP connection. For meek, the transport client ignores the IP address, because the decision of what IP address to connect to is configured globally at the CDN, not at each client.

I think tor is confused because it associates the bridge IP address with the connection, or something, and it rejects the connection even though it's not actually directly connecting to the IP address. Maybe the restriction on what IP addresses may be connected to should be relaxed in the case you cited above (Bridge configured with a matching ClientTransportPlugin).

comment:10 Changed 11 months ago by dcf

  • Keywords meek added

comment:11 Changed 11 months ago by dcf

  • Status changed from new to needs_review

attachment:0001-Use-RFC-5737-TEST-NET-1-addresses-for-meek-dummy-add.patch is a patch that applies the 192.0.2.0/24 workaround, so we can have working bundles until we decide on something more permanent. It applies to master and maint-5.5.

comment:12 in reply to: ↑ 8 ; follow-ups: Changed 11 months ago by teor

Replying to yawning:

Replying to teor:

I think what tor should do is:

  • if a transport is specified in the Bridge line, and that transport corresponds to a ClientTransportPlugin line, tor should pass the address in the bridge line to the pluggable transport, and not try to connect to that address itself. (The pluggable transport is free to use [obfsproxy] or ignore [meek] that address.)

This is what it used to do before it changed it, the "decide if need to pass it onto a PT" is done at the last possible moment. What your code change effectively does is add "before passing the address to the PT (if applicable), check that it is not internal" (See: connection_or.c:connection_or_connect()).

This change also breaks normal (non-PT) bridges hosted on an internal network, but I'm not sure if that's a common use case for actual users. It's rather common for me to run a bridge on the same host (communicating over the loopback interface) for testing, but I only need to edit 15 or so torrc files....

The solutions I can think of off the top of my head are:

  • Allow connections to *all* bridges on "internal addresses".

I think allowing bridges on internal addresses is a good fix for this.

(And since they're configured in the torrc, this doesn't open any security holes. Bridge addresses can be updated from bridgedb, but if bridgedb started returning broken addresses, then that's not an issue clients can defend against.)

Please see my branch bug18517 at https://github.com/teor2345/tor.git
It fixes this issue and clarifies the corresponding entries in the tor man page.
It needs to go in tor-0.2.8-rc, target date end of March.

dcf, sorry that I've flipped this issue from tor to tor browser and back.
Which one do you want to make a separate ticket for?

comment:13 Changed 11 months ago by teor

  • Owner changed from tbb-team to teor
  • Status changed from needs_review to accepted

comment:14 Changed 11 months ago by teor

  • Status changed from accepted to needs_review

comment:15 in reply to: ↑ 12 ; follow-up: Changed 11 months ago by yawning

  • Status changed from needs_review to needs_revision

Replying to teor:

I think allowing bridges on internal addresses is a good fix for this.

(And since they're configured in the torrc, this doesn't open any security holes. Bridge addresses can be updated from bridgedb, but if bridgedb started returning broken addresses, then that's not an issue clients can defend against.)

I agree that this is probably the best way to go.

Please see my branch bug18517 at https://github.com/teor2345/tor.git
It fixes this issue and clarifies the corresponding entries in the tor man page.
It needs to go in tor-0.2.8-rc, target date end of March.

NACK. The check you added only checks if UseBridges is set in the torrc, and bypasses the behavior. This isn't great, and we can/should be more explicit in what we allow.

Instead, if we wish to go with the current fix, we should verify that firsthop->extend_info->addr is actually a bridge (and while we're here it, also validate that the port of our possibly synthetic address matches that on the bridge line). We do not have an easy function to query if a given address/port is a configured bridge, but get_transport_by_bridge_addrpor() is 95% of what such a thing will look like and is trivial to add.

Basically the check should function like:

int is_configured_bridge(const tor_addr_t *addr, uint16_t port) {
  // Traverse `bridge_list` (see entrynodes.c), return 1 if addr/port is present, 0 otherwise.
}

static int is_or_destination_valid(const tor_addr_t *addr, uint16_t port) { // Rename as appropriate...
  if (!get_options()->ExtendAllowPrivateAddresses) {
    if (get_options()->UseBridges && is_configured_bridge(addr, port)) // Write the 2nd function...
      return 1; // All address/port combinations listed on `Bridge` directives allowed.
    if (tor_addr_is_internal(addr, 0))
      return 0; // Filter out local address destinations for all non-Bridge ORs.
  }
  return 1; // Default allow.
}

comment:16 in reply to: ↑ 12 ; follow-up: Changed 11 months ago by dcf

Replying to teor:

dcf, sorry that I've flipped this issue from tor to tor browser and back.
Which one do you want to make a separate ticket for?

Um, maybe make a separate one for the temporary Tor Browser workaround. Attach attachment:0001-Use-RFC-5737-TEST-NET-1-addresses-for-meek-dummy-add.patch​, mark it needs_review, and tag it TorBrowserTeam201603R so it gets quick attention.

comment:17 in reply to: ↑ 16 Changed 11 months ago by teor

Replying to dcf:

Replying to teor:

dcf, sorry that I've flipped this issue from tor to tor browser and back.
Which one do you want to make a separate ticket for?

Um, maybe make a separate one for the temporary Tor Browser workaround. Attach attachment:0001-Use-RFC-5737-TEST-NET-1-addresses-for-meek-dummy-add.patch​, mark it needs_review, and tag it TorBrowserTeam201603R so it gets quick attention.

Split off #18622 for this.

comment:18 in reply to: ↑ 15 Changed 11 months ago by teor

  • Status changed from needs_revision to needs_review

Replying to yawning:

Replying to teor:

I think allowing bridges on internal addresses is a good fix for this.
Please see my branch bug18517 at https://github.com/teor2345/tor.git

NACK. The check you added only checks if UseBridges is set in the torrc, and bypasses the behavior. This isn't great, and we can/should be more explicit in what we allow.

Instead, if we wish to go with the current fix, we should verify that firsthop->extend_info->addr is actually a bridge (and while we're here it, also validate that the port of our possibly synthetic address matches that on the bridge line). We do not have an easy function to query if a given address/port is a configured bridge, but get_transport_by_bridge_addrpor() is 95% of what such a thing will look like and is trivial to add.

Basically the check should function like:

int is_configured_bridge(const tor_addr_t *addr, uint16_t port) {
  // Traverse `bridge_list` (see entrynodes.c), return 1 if addr/port is present, 0 otherwise.
}

static int is_or_destination_valid(const tor_addr_t *addr, uint16_t port) { // Rename as appropriate...
  if (!get_options()->ExtendAllowPrivateAddresses) {
    if (get_options()->UseBridges && is_configured_bridge(addr, port)) // Write the 2nd function...
      return 1; // All address/port combinations listed on `Bridge` directives allowed.
    if (tor_addr_is_internal(addr, 0))
      return 0; // Filter out local address destinations for all non-Bridge ORs.
  }
  return 1; // Default allow.
}

Calling this Y1.

Y1 is addressed by the fixup commit 4b271ad in my branch bug18517.

This commit:

  • adds extend_info_is_a_configured_bridge, which calls addr_is_a_configured_bridge, based on the existing function get_configured_bridge_by_addr_port_digest
  • replaces the original patch's options->UseBridges with extend_info_is_a_configured_bridge

I think addresses Yawning's concerns, and fixes dcf's issue with pluggable transports with dummy private addresses (and restores bridges on private addresses).

comment:19 Changed 11 months ago by nickm

I think this looks okay; merging it.

comment:20 Changed 11 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:21 Changed 11 months ago by nickm

  • Keywords 2016-bug-retrospective added

Mark more tickets for severe bug retrospective, based on Priority and date and hand-inspection.

Note: See TracTickets for help on using tickets.