Opened 3 months ago

Closed 2 months ago

#30166 closed defect (fixed)

If custom bridges are specified, only use those bridges for connecting

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-8.5-must, TorBrowserTeam201905
Cc: igt0, sisbell, sysrqb, hans@…, n8fr8 Actual Points:
Parent ID: #27609 Points:
Reviewer: Sponsor:

Description (last modified by gk)

Currently it seems custom bridges are added to the preconfigured bridges list and then 3 are chosen randomly. What should happen, though, is that only those custom bridges should get used that are specified.

(That's based on a discussion by sisbell and sysrqb on #tor-dev. I am just the messenger in this case. :) )

Child Tickets

Change History (26)

comment:1 Changed 3 months ago by gk

Description: modified (diff)

comment:2 Changed 3 months ago by gk

Keywords: tbb-8.5-must added; bb-8.5-must removed

comment:4 Changed 3 months ago by gk

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: newneeds_review

comment:5 Changed 3 months ago by gk

Status: needs_reviewneeds_information

I created bug_30166 (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_30166&id=5ec4223148f43b2782bf88dc5b1b738eac75a5dd) in my tor-browser-build repo but still have the issue that I can add a random IP address:port combination and Tor Browser is happily ignoring the custom bridge settings and connecting directly. So it's hard for me to test that changes actually. sisbell: are you seeing the same with your build or do you get any error when doing as I did?

comment:6 Changed 3 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201904R removed
Status: needs_informationneeds_revision

Okay, I double-checked that I did not make a mistake here. The tor-android-service repo I used is indeed on commit de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and should thus have your fix. However, testing the resulting nightly as follows still shows the behavior in comment:5

1) Install the resulting nightly
2) Before you start bootstrapping click on the gear icon and enter a random custom bridge. I just used 1.2.3.4:12345
3) Go back to the bootstrap screen
4) Start bootstrapping
5) Expected: error about not working bridge or something
6) Actual: Happy bootstrapping making a direct connection

comment:7 in reply to:  6 Changed 3 months ago by sisbell

Replying to gk:

Okay, I double-checked that I did not make a mistake here. The tor-android-service repo I used is indeed on commit de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and should thus have your fix. However, testing the resulting nightly as follows still shows the behavior in comment:5

1) Install the resulting nightly
2) Before you start bootstrapping click on the gear icon and enter a random custom bridge. I just used 1.2.3.4:12345
3) Go back to the bootstrap screen
4) Start bootstrapping
5) Expected: error about not working bridge or something
6) Actual: Happy bootstrapping making a direct connection

Yes, there is a bug in the code. The app is only processing bridges if the custom entry has obfs4 or meek at a first field. So the custom fields are being ignored and then the app proceeds to just make a direct connection. I'll put in a fix.

comment:8 Changed 3 months ago by sisbell

I updated the TOPL and android-tor-service code to fix this issue:

https://github.com/sisbell/tor-android-service/commit/30a7a83c5e715eb7ef739870bc0ff615ad52daeb
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/061b7e52310279cb70a125819d7abed8da66bfbe

I verified with Orbot app that it is picking up user-defined bridges (using one I pulled from torproject bridges site). When the user does not define a bridge but enables bridges, I verified that it is using one of the random bridges during startup. The PT entries are also in the torrc file.

The one case I'm not certain of (and the commits above don't handle), is if a user specifies a bridge type (like obfs4) as the first field in the 'Bridge Addresses' TextField. Is this allowed?

I also still need to test this on within the tor-browser-build.

comment:9 in reply to:  8 Changed 3 months ago by gk

Replying to sisbell:

I updated the TOPL and android-tor-service code to fix this issue:

https://github.com/sisbell/tor-android-service/commit/30a7a83c5e715eb7ef739870bc0ff615ad52daeb
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/061b7e52310279cb70a125819d7abed8da66bfbe

I verified with Orbot app that it is picking up user-defined bridges (using one I pulled from torproject bridges site). When the user does not define a bridge but enables bridges, I verified that it is using one of the random bridges during startup. The PT entries are also in the torrc file.

The one case I'm not certain of (and the commits above don't handle), is if a user specifies a bridge type (like obfs4) as the first field in the 'Bridge Addresses' TextField. Is this allowed?

Yes. If you e.g. test on https://bridges.torproject.org and obtain obfs4 bridges that way the idea is to copy those lines into your Tor Browser and the correct bridges are picked up. (see the howto for instructions, which is for now only for desktop, though: https://bridges.torproject.org/howto and the bridge options at https://bridges.torproject.org/options).

comment:10 Changed 2 months ago by sisbell

Status: needs_revisionneeds_review

Fixed in branch 505

  • I tested with a bad bridge. The console says that tor unable to start. This is not descriptive to the user but failing to start is what we expect
  • Tested with working custom bridge and saw the bridge used during startup
  • Tested with any obfs4 bridge (one from pre-configured list in apk) and saw one of the bridges in the list used during startup

This fix is in TOPL and tor-android-service

https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/115

https://github.com/sisbell/tor-android-service/issues/26

The commit in tor-browser-build that picks these up is here. 

https://github.com/sisbell/tor-browser-build/commit/c1cf5b6d77637b5095b3063d4f5d5fedfab6e9e7

comment:11 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201904 removed

comment:12 Changed 2 months ago by gk

Okay, we have now an own tor-android-service repo and we are using that one for our builds. Thus, just bumping the git version in our tor-browser-build project is not working anymore. We should start going the usual review process: changes to tor-android-service need to get reviewed and if there are no issues then the commit will be pushed to master on TPO infra. Then we need to bump the commit in tor-browser-build. So, let me start by reviewing the proposed tor-android-service commits.

comment:13 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed
Status: needs_reviewneeds_revision

Alright here come some questions:

1) It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem?

2) What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message)

3) What's the deal with the TOPL libs in the repo given that we build those ourselves during the build?

comment:14 in reply to:  13 ; Changed 2 months ago by sisbell

Replying to gk:

Alright here come some questions:

1) It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem?

Sure I'll get that fixed. I have been working off master, so I didn't want to rebase in case anyone else was using the library but now that we have brought in the code, I'll work off the branches with rebases.

2) What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message)

This may be an issue for further discussion. My idea has been to create a library for general use in the community so I wanted to keep the Android versions updated. Otherwise, we lock developers into older versions of Android Studio and dependencies, which makes adoption more difficult.

The versions are isolated to the properties files so changes are easy. As long our patches for the versions still work, this seems a good compromise.

3) What's the deal with the TOPL libs in the repo given that we build those ourselves during the build?

This is also related to general community use. I want this to work outside of tor-browser-build. Ideally, when things are stable (which looks very close), we will get the TOPL libraries pushed to a maven repo, rather than including them in the libs directory.

comment:15 in reply to:  14 Changed 2 months ago by gk

Replying to sisbell:

Replying to gk:

Alright here come some questions:

1) It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem?

Sure I'll get that fixed. I have been working off master, so I didn't want to rebase in case anyone else was using the library but now that we have brought in the code, I'll work off the branches with rebases.

Thanks.

2) What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message)

This may be an issue for further discussion. My idea has been to create a library for general use in the community so I wanted to keep the Android versions updated. Otherwise, we lock developers into older versions of Android Studio and dependencies, which makes adoption more difficult.

The versions are isolated to the properties files so changes are easy. As long our patches for the versions still work, this seems a good compromise.

Works for me. I'd like to have in the commit message a hint, though, what makes the update necessary or why we are doing it (maybe it's a new SDK we want to support, maybe a new Android version, maybe...), just so that one has something to reason about whether the actual change is good/necessary.

3) What's the deal with the TOPL libs in the repo given that we build those ourselves during the build?

This is also related to general community use. I want this to work outside of tor-browser-build. Ideally, when things are stable (which looks very close), we will get the TOPL libraries pushed to a maven repo, rather than including them in the libs directory.

Okay, yes, please. Meanwhile I am fine having them in the repo, although I'd like to avoid that as a permanent solution.

comment:16 Changed 2 months ago by sisbell

Status: needs_revisionneeds_review

Fixed in branch 0512: https://github.com/sisbell/tor-android-service/commits/0512

comment:17 Changed 2 months ago by gk

Status: needs_reviewneeds_revision

I tested a resulting build using sisbell/0506 + pointing tor-android-service tor your 0512 and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./

It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard.

Setting into needs_revision for the nit and for redoing the tor-browser-build patches taking other review comments into account (http:// -> https://).

comment:18 Changed 2 months ago by gk

Oh, and it just came to my mind that we'll probably have a snowflake transport in Tor Browser for Android, soon (at least in the alpha). Thus,

+        byte bridgeType = (byte) (userDefinedBridgeList.length() > 5 ? 1 : 0);

might not work as expected anymore then. I guess we take the length of "snowflake" into account until we've redone the bridges list part.

comment:19 in reply to:  17 ; Changed 2 months ago by sisbell

Replying to gk:

I tested a resulting build using sisbell/0506 + pointing tor-android-service tor your 0512 and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./

It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard.

Not overloading the preference is preferable. I think this change may have UI impact, since the UI field we are reading from is also overloaded. This will require some looking into to see if this is the case.

Setting into needs_revision for the nit and for redoing the tor-browser-build patches taking other review comments into account (http:// -> https://).

comment:20 in reply to:  19 Changed 2 months ago by gk

Replying to sisbell:

Replying to gk:

I tested a resulting build using sisbell/0506 + pointing tor-android-service tor your 0512 and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./

It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard.

Not overloading the preference is preferable. I think this change may have UI impact, since the UI field we are reading from is also overloaded. This will require some looking into to see if this is the case.

Yes, which we should do it in a new ticket (could you open that one?) and not for 8.5.

comment:21 Changed 2 months ago by sisbell

I updated the nit and added https to the maven repo.

https://github.com/sisbell/tor-android-service/commits/0513

I also updated tor-browser-build to use the latest commits

https://github.com/sisbell/tor-browser-build/commits/0513

Note that the git_hash in tor-android-service/config file doesn't exist yet in the torproject repo. The commit is only in the github branch. So we will need to merge the tor-android-service changes for the ttb build to work.

git_hash: d5c9b1f569e1f04ccadeb75184cec0766c4c2f4d
git_url: https://git.torproject.org/tor-android-service.git

For testing, you can locally modify the git_url to

https://github.com/sisbell/tor-android-service.git

comment:22 Changed 2 months ago by gk

Thanks. I fixed this particular issue tor-android-service'smaster with de62a5af6026a9caa6448eaac4e0bc9709a20817.

comment:23 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: needs_revisionneeds_review

comment:24 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed
Status: needs_reviewneeds_revision

Okay, for the tor-browser-build patch just a nit:

 -
++    

in the servive/build.gradle patch you are adding a bunch of superfluous whitespace, please remove that. Otherwise this looks good.

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

Replying to gk:

Okay, for the tor-browser-build patch just a nit:

 -
++    

in the servive/build.gradle patch you are adding a bunch of superfluous whitespace, please remove that. Otherwise this looks good.

Removed in branch 0514

comment:26 Changed 2 months ago by gk

Resolution: fixed
Status: needs_revisionclosed

Thanks! Merged to master (commit 9276f194da86c48792c8ebee23dd4e7056d01eae) and cherry-picked to maint-8.5 (commit 05c9f970270441c071dc66b51034f4343a83c7de).

Note: See TracTickets for help on using tickets.