Custom obfs4 bridge does not work on Tor Browser for Android

[gk]

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.

[sysrqb]

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.

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

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

[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

[sisbell]

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

[pili]

Moving tickets to November 2019

[gk]

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

[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

[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";

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?

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

[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

[pili]

Moving tickets to December

[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).

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

[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

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

[sysrqb]

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.