Opened 12 months ago

Closed 4 months ago

#30767 closed defect (fixed)

Custom obfs4 bridge does not work on Tor Browser for Android

Reported by: gk Owned by: sisbell
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-parity, tbb-mobile, TorBrowserTeam202001R, 9.5a5, tbb-backport
Cc: phw, cohosh, arma, tbb-team Actual Points:
Parent ID: #31284 Points: 0.5
Reviewer: sysrqb Sponsor: Sponsor30-can

Description

We got a report on our blog post (https://blog.torproject.org/comment/282217#comment-282217) mentioning that custom obfs4 bridges don't work on Android while they do on desktop. I tested with a bridge provided and can reproduce the issue.

Child Tickets

Change History (29)

comment:1 Changed 12 months ago by gk

Keywords: tbb-paritiy added; tbb-partiy removed

comment:2 Changed 12 months ago by gk

Keywords: tbb-parity added; tbb-paritiy removed

comment:3 Changed 10 months ago by pili

Parent ID: #31284
Sponsor: Sponsor30-can

comment:4 Changed 8 months ago by arma

Cc: phw cohosh arma added

comment:5 Changed 8 months ago by sysrqb

Cc: sisbell added
Keywords: TorBrowserTeam201909 added; TorBrowserTeam201906 removed

Notice the missing ClientTransportPlugin line for obfs4proxy.

generic_x86:/ # cat /data/data/org.torproject.torbrowser_alpha/app_torservice/torrc
AutomapHostsOnResolve 1
ControlPortWriteToFile /data/user/0/org.torproject.torbrowser_alpha/app_torservice/lib/tor/control.txt
ControlPort auto
CookieAuthentication 1 
CookieAuthFile /data/user/0/org.torproject.torbrowser_alpha/app_torservice/lib/tor/control_auth_cookie
DisableNetwork 1
DNSPort 5400
HTTPTunnelPort 8218
ReducedConnectionPadding 1
RunAsDaemon 1
SafeSocks 0
SOCKSPort 9150 KeepAliveIsolateSOCKSAuth IPv6Traffic PreferIPv6
StrictNodes 0
TestSocks 0
TransPort 9140
VirtualAddrNetwork 10.192.0.0/10
Bridge obfs4 123.234.345.456:3322 AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA cert=asdfasdf
UseBridges 1

It's because we only add the ClientTransportPlugin line for the default bridges.

comment:6 Changed 8 months ago by pili

Keywords: TorBrowserTeam201910 added

comment:7 Changed 8 months ago by pili

Keywords: TorBrowserTeam201909 removed

comment:8 Changed 8 months ago by sysrqb

Points: 0.5

comment:9 Changed 8 months ago by sisbell

This should pick up. It will take the list of bridges from preferences in the UI (settings). I'm thinking maybe the preferences are not getting set anymore after removing the old orbot code. I'll confirm if this is the case.

comment:10 in reply to:  5 ; Changed 8 months ago by arma

Replying to sysrqb:

It's because we only add the ClientTransportPlugin line for the default bridges.

In theory you should be able to put all the ClientTransportPlugin lines in there by default, even when there are no bridges set, and Tor won't launch any of them until it has a bridge that needs them.

comment:11 in reply to:  10 Changed 8 months ago by mcs

Replying to arma:

In theory you should be able to put all the ClientTransportPlugin lines in there by default, even when there are no bridges set, and Tor won't launch any of them until it has a bridge that needs them.

FWIW, Tor Browser on desktop has used this approach ever since we switched to Tor Launcher (i.e., for a long time now). The following gets appended to the torrc-defaults file which is always loaded by TB desktop's tor:

https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/tor-browser/Bundle-Data/PTConfigs/linux/torrc-defaults-appendix

Last edited 8 months ago by mcs (previous) (diff)

comment:12 Changed 7 months ago by sisbell

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

I fixed this as part of fixing the bridge list. I took the approach suggested by arma, the plugin transport is added regardless of whether user specifies a bridge. This is a much cleaner approach.I added some unit tests for verify behavior.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/ac9ad2295b3dcb312a9aff4f3f7b27343bc4b73d

comment:13 Changed 7 months ago by pili

Keywords: TorBrowserTeam201911 added

Moving tickets to November 2019

comment:14 Changed 7 months ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201910R removed

There is no way to do reviews in October 2019 anymore.

comment:15 Changed 7 months ago by gk

Keywords: TorBrowserTeam201911 removed

comment:16 Changed 6 months ago by sisbell

This is a very tight fix, only addressing the obfs4 bug and a couple of bridge bugs. No change required for tor-android-service since the API hasn't changed. This commit does not address all issues we have with settings.

Changes

  • Break apart the setting of transports and bridges. Transports will be set independent of whether user has configured bridges.
  • Removed shuffle of bridges (addresses previous review comment)
  • Removed max limit on number of bridges (addresses previous review comment)

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/39f6aad9257947d7c47181e04963857284a67a44

comment:17 Changed 6 months ago by sysrqb

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

Looks pretty good.

+    @SettingsConfig
+    public TorConfigBuilder bridgesFromSettings() {
+        List<String> supportedBridges = settings.getListOfSupportedBridges();
+        if (supportedBridges == null || supportedBridges.isEmpty()) {
+            return this;
+        }
+        String type = supportedBridges.contains("meek") ? "meek_lite" : "obfs4";

Do we still use both? obfs4 now has support for meek, so I assume we simply can do the same. As an example, I have this in my Tor Browser torrc:

ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec ./TorBrowser/Tor/PluggableTransports/obfs4proxy

-        if(hasAddedBridge) useBridges();

I don't see any other useBridges() calls, do we need an unconditional call now?

comment:18 in reply to:  17 Changed 6 months ago by sisbell

Replying to sysrqb:

Looks pretty good.

+    @SettingsConfig
+    public TorConfigBuilder bridgesFromSettings() {
+        List<String> supportedBridges = settings.getListOfSupportedBridges();
+        if (supportedBridges == null || supportedBridges.isEmpty()) {
+            return this;
+        }
+        String type = supportedBridges.contains("meek") ? "meek_lite" : "obfs4";

This is done to filter out obfs4 bridges is meek is asked for but based on your information, I don't think we need a filter.

Do we still use both? obfs4 now has support for meek, so I assume we simply can do the same. As an example, I have this in my Tor Browser torrc:

ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec ./TorBrowser/Tor/PluggableTransports/obfs4proxy

-        if(hasAddedBridge) useBridges();

I don't see any other useBridges() calls, do we need an unconditional call now?

I looked through the bridge settings code. Since UseBridges is 0 by default, the following methods needs to be changed to write bridges if enabled.

    @SettingsConfig
    public TorConfigBuilder useBridgesFromSettings() {
        return !settings.hasBridges() ? dontUseBridges() : this;
    }

to

    @SettingsConfig
    public TorConfigBuilder useBridgesFromSettings() {
        return settings.hasBridges() ? useBridges() : this;
    }

There is one other use of useBridges, but I believe I can remove that as well. This MAY cause a problem with Orbot configuration but that can be resolved later as we clean up the code.

I'll make a couple of changes and make a new commit.

Last edited 6 months ago by sisbell (previous) (diff)

comment:19 Changed 6 months ago by sisbell

I made the following changes:

  • We no longer filter bridges based on type
  • Write out UseBridges, only if explicitly set

The filter part was supporting a filter feature from orbot that I don't think was fully baked since it only supported an either/or meek/obfs option. Filtering is a useful option but we should open an issue for that and come up with a more general solution. This is a breaking change with Orbot.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/d05797385127876889d6d7a8e8fc8cc45d8cccdd

comment:20 Changed 6 months ago by pili

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201911 removed

Moving tickets to December

comment:21 Changed 6 months ago by sysrqb

Cc: tbb-team added; sisbell removed
Keywords: TorBrowserTeam201912R added; TorBrowserTeam201912 removed
Owner: changed from tbb-team to sisbell
Reviewer: sysrqb
Status: needs_revisionassigned

comment:22 Changed 6 months ago by sysrqb

Status: assignedneeds_review

comment:23 Changed 5 months ago by sysrqb

Keywords: TorBrowserTeam202001R added; TorBrowserTeam201912R removed

comment:24 in reply to:  19 ; Changed 4 months ago by sysrqb

Status: needs_reviewneeds_revision

Replying to sisbell:

I made the following changes:

  • We no longer filter bridges based on type
  • Write out UseBridges, only if explicitly set

The filter part was supporting a filter feature from orbot that I don't think was fully baked since it only supported an either/or meek/obfs option. Filtering is a useful option but we should open an issue for that and come up with a more general solution. This is a breaking change with Orbot.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/d05797385127876889d6d7a8e8fc8cc45d8cccdd

This patch should unconditionally call transportPluginObfs() and transportPluginMeek() in configurePluggableTransportsFromSettings() after it checks that the file exists and is executable. We don't care if the transport is part of the supportedBridges list because they aren't related.

I also mentioned earlier that we can simply combine the transportPlugin*() methods, and that can output a single line containing all of the transports (ClientTransportPlugin meek_lite,obfs3,obfs4 exec).

comment:25 in reply to:  24 ; Changed 4 months ago by sisbell

Replying to sysrqb:

Replying to sisbell:

I made the following changes:

  • We no longer filter bridges based on type
  • Write out UseBridges, only if explicitly set

The filter part was supporting a filter feature from orbot that I don't think was fully baked since it only supported an either/or meek/obfs option. Filtering is a useful option but we should open an issue for that and come up with a more general solution. This is a breaking change with Orbot.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/d05797385127876889d6d7a8e8fc8cc45d8cccdd

This patch should unconditionally call transportPluginObfs() and transportPluginMeek() in configurePluggableTransportsFromSettings() after it checks that the file exists and is executable. We don't care if the transport is part of the supportedBridges list because they aren't related.

I also mentioned earlier that we can simply combine the transportPlugin*() methods, and that can output a single line containing all of the transports (ClientTransportPlugin meek_lite,obfs3,obfs4 exec).

If we don't need to check the supported bridges, then collapsing the transport line is super easy. I'll make these changes.

comment:26 in reply to:  25 Changed 4 months ago by sysrqb

Replying to sisbell:

Replying to sysrqb:

Replying to sisbell:

I made the following changes:

  • We no longer filter bridges based on type
  • Write out UseBridges, only if explicitly set

The filter part was supporting a filter feature from orbot that I don't think was fully baked since it only supported an either/or meek/obfs option. Filtering is a useful option but we should open an issue for that and come up with a more general solution. This is a breaking change with Orbot.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/d05797385127876889d6d7a8e8fc8cc45d8cccdd

This patch should unconditionally call transportPluginObfs() and transportPluginMeek() in configurePluggableTransportsFromSettings() after it checks that the file exists and is executable. We don't care if the transport is part of the supportedBridges list because they aren't related.

I also mentioned earlier that we can simply combine the transportPlugin*() methods, and that can output a single line containing all of the transports (ClientTransportPlugin meek_lite,obfs3,obfs4 exec).

If we don't need to check the supported bridges, then collapsing the transport line is super easy. I'll make these changes.

Great, thanks. I pushed a branch with a possible fixup commit. Feel free to use it or not.
https://gitweb.torproject.org/user/sysrqb/tor-onion-proxy-library.git/commit/?h=bug30767&id=63fad679212838268935b45f12ec3f9ab61a4e5f

comment:27 Changed 4 months ago by sisbell

So here is the commit with sysrqb's patch

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/0a482a749fd770827d3d6c71debb112a07e7fae3

I still need to test in tbb build.

comment:28 Changed 4 months ago by sysrqb

Keywords: 9.5a5 tbb-backport added

Thanks. I landed this as a patch on tor-onion-proxy-library as commit 33aed285867063ed8dd107568a7e3cde1fc23f07. We can work on upstreaming this after the release.

We can think about backporting this after it bakes a bit.

comment:29 Changed 4 months ago by sysrqb

Resolution: fixed
Status: needs_revisionclosed
Note: See TracTickets for help on using tickets.