[tbb-bugs] #33931 [Applications/Tor Browser]: obfs4 bridges are used instead of meek if meek is selected in Tor Browser for Android alpha

[Tor Bug Tracker & Wiki]

#33931: obfs4 bridges are used instead of meek if meek is selected in Tor Browser
for Android alpha
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, tbb-parity, tbb-         |  Actual Points:
  regression, TorBrowserTeam202004R              |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by sysrqb):

 Replying to [comment:6 acat]:
 > I did not have time to fully understand the code, but let's see. My
 understanding is that the code in `CustomTorInstaller.java` cannot do the
 filtering, since it's not responsible of parsing the bridges, and the
 changes are needed so that the code that parses the bridges can filter
 them. I hope that's correct.

 Correct.

 >
 > Ok, so in https://gitweb.torproject.org/user/sysrqb/tor-android-
 service.git/commit/?h=bug33931_00&id=769b3c85de468bb23fbb891266ab6cbb9c662e13
 >
 > would it make sense to update the comment:
 > `For (1), we just pass back all bridges, the filter will occur elsewhere
 in the library.`
 > to include the fact that we are also encoding the bridge type that has
 to be filtered?

 Yes, I'll add that comment.

 >
 > Besides, given the `userDefinedBridgeList.length() > 5` check, I think
 `userDefinedBridgeList` cannot be `"meek_lite"`, so I assume this can be
 removed from the switch.

 Yes. I'll remove that.

 >
 > I guess there are no other values that could make `bridgeType=0` other
 than the empty string? If we know all the possible values of
 `userDefinedBridgeList` (when `bridgeType == 0`), would it make sense to
 have cases for all of them, and then have a default that throws an error
 (similar to the switch in https://gitweb.torproject.org/user/sysrqb/tor-
 browser-
 build.git/commit/?h=bug33931_00&id=91e6aec4f60783fc0008d4d3c60c29ddecafac0d)?

 The defined cases in this patch are the only pluggable transports we
 currently use (`obfs4` and `meek(_lite)`). In the past, we had `obfs3`. I
 don't think we should expand this list, but (after thinking about this a
 little more) we should fail safe if the type is not recognized instead of
 "all bridges" being the default. I'll add that in a fixup commit. Thanks!

 >
 > The rest looks ok to me,. I did not have time to test the patch, but
 I'll start a build for that in case it's needed (it will take a while).

 Great, thanks for the review!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/33931#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online