obfs4 bridges are used instead of meek if meek is selected in Tor Browser for Android alpha
In the 9.5 alpha series meek is not used anymore even if meek is selected as pluggable transport. Instead Tor Browser tries to use some obfs4 bridge from the bridge list coming with Tor Browser. That's happening on 9.5a11. 9.0.9 is not affected.
Activity
I think I found the bug:
service/src/main/java/org/torproject/android/service/CustomTorInstaller.javapublic InputStream openBridgesStream() throws IOException { String userDefinedBridgeList = Prefs.getBridgesList(); byte bridgeType = (byte) (userDefinedBridgeList.length() > 5 ? 1 : 0); ByteArrayInputStream bridgeTypeStream = new ByteArrayInputStream(new byte[]{bridgeType}); InputStream bridgeStream = (bridgeType == 1) ? new ByteArrayInputStream((userDefinedBridgeList + "\r\n").getBytes()) : context.getResources().getAssets().open("common/bridges.txt"); return new SequenceInputStream(bridgeTypeStream, bridgeStream);universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.javaTorConfigBuilder addBridgesFromResources() throws IOException { if(settings.hasBridges()) { InputStream bridgesStream = context.getInstaller().openBridgesStream(); int formatType = bridgesStream.read(); if (formatType == 0) { addBridges(bridgesStream); } else { addCustomBridges(bridgesStream); } } return this; } /** * Add bridges from bridges.txt file. */ private void addBridges(InputStream input) { if (input == null) { return; } List<Bridge> bridges = readBridgesFromStream(input); for (Bridge b : bridges) { bridge(b.type, b.config); } }When we combine this with the fix for #30767 (moved), we have a recipe for adding all default bridges and configuring a pluggable transport for
meek_lite,obfs3,obfs4. We never filter the list of bridge for the selected type. As a result, tor chooses one of the configured bridges which is (probabilistically) anobfs4bridge.Okay. I forgot String equality comparison in Java should use
.equal()instead of==, so that was a waste of some time. In any case, for review:Branch
bug33931_00in mytor-android-servicerepo for one piece of this.And branch
bug33931_00in mytor-browser-buildrepo:The summary of these patches is that tor-android-service encodes (within the first byte) if the following datastream is arbitrary custom bridges or if it comes from a list of default bridges (and which specific bridge type, if applicable).
When the first byte is:
- 1, then the stream contains arbitrary bridges
- 0, then the stream is all default bridges
- 2, then the stream is all default bridges and only obfs4 bridges should be used after parsing
- 3, then the stream is all default bridges and only meek_lite bridges should be used after parsing
TOPL receives this and now handles them appropriately.
Trac:
Status: new to needs_reviewI did not have time to fully understand the code, but let's see. My understanding is that the code in
CustomTorInstaller.javacannot 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.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?Besides, given the
userDefinedBridgeList.length() > 5check, I thinkuserDefinedBridgeListcannot be"meek_lite", so I assume this can be removed from the switch.I guess there are no other values that could make
bridgeType=0other than the empty string? If we know all the possible values ofuserDefinedBridgeList(whenbridgeType == 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 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).
Replying to acat:
I did not have time to fully understand the code, but let's see. My understanding is that the code in
CustomTorInstaller.javacannot 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.
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() > 5check, I thinkuserDefinedBridgeListcannot 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=0other than the empty string? If we know all the possible values ofuserDefinedBridgeList(whenbridgeType == 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 (
obfs4andmeek(_lite)). In the past, we hadobfs3. 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!
Replying to sysrqb:
Replying to acat:
I did not have time to fully understand the code, but let's see. My understanding is that the code in
CustomTorInstaller.javacannot 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.
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() > 5check, I thinkuserDefinedBridgeListcannot 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=0other than the empty string? If we know all the possible values ofuserDefinedBridgeList(whenbridgeType == 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 (
obfs4andmeek(_lite)). In the past, we hadobfs3. 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!I am not exactly sure if that's what you meant but I think the case where
bridgeTypefor whatever reason is still0at the end ofopenBridgeSream()should be treated inaddBridgesFromResources()in thethrowblock. That is, there should not be acase 0check anymore. Keeping that smells just like trouble we are currently dealing with. We start to be explicit with your patches in this bug. so let's avoid ambiguity here where we can.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!
Replying to gk:
Replying to sysrqb:
Replying to acat:
I guess there are no other values that could make
bridgeType=0other than the empty string? If we know all the possible values ofuserDefinedBridgeList(whenbridgeType == 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 (
obfs4andmeek(_lite)). In the past, we hadobfs3. 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!I am not exactly sure if that's what you meant but I think the case where
bridgeTypefor whatever reason is still0at the end ofopenBridgeSream()should be treated inaddBridgesFromResources()in thethrowblock. That is, there should not be acase 0check anymore. Keeping that smells just like trouble we are currently dealing with. We start to be explicit with your patches in this bug. so let's avoid ambiguity here where we can.I think I was imagining that we would
throwat the end ofopenBridgeStream(), but lettingaddBridgesFromResources()handle this error case seems okay to me, in this case, as well.I pushed a fixup commit onto my tor-android-service
bug33931_00branch, and I pushed a new tor-browser-build branchbug33931_01containing an updated TOPL patch.Trac:
Status: needs_revision to needs_review-
Replying to sysrqb:
Replying to gk:
Replying to sysrqb:
Replying to acat:
I guess there are no other values that could make
bridgeType=0other than the empty string? If we know all the possible values ofuserDefinedBridgeList(whenbridgeType == 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 (
obfs4andmeek(_lite)). In the past, we hadobfs3. 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!I am not exactly sure if that's what you meant but I think the case where
bridgeTypefor whatever reason is still0at the end ofopenBridgeSream()should be treated inaddBridgesFromResources()in thethrowblock. That is, there should not be acase 0check anymore. Keeping that smells just like trouble we are currently dealing with. We start to be explicit with your patches in this bug. so let's avoid ambiguity here where we can.I think I was imagining that we would
throwat the end ofopenBridgeStream(), but lettingaddBridgesFromResources()handle this error case seems okay to me, in this case, as well.I pushed a fixup commit onto my tor-android-service
bug33931_00branch, and I pushed a new tor-browser-build branchbug33931_01containing an updated TOPL patch.Looks good to me. Squashed and merged to
tor-android-service'smaster(commit ed2e1479eeddede01b1d510ef79dc5ec798b39c0) and merged totor-browser-build'smaster+ a commit to pick thetor-android-servicechanges up (commits 04da65342a76f74b3d4a58601f326ded457dc97a and b858dca8745b583afd2660389ccd5a96630154a0).Trac:
Resolution: N/A to fixed
Status: needs_review to closed