Opened 5 weeks ago

Closed 4 weeks ago

Last modified 3 weeks ago

#33931 closed defect (fixed)

obfs4 bridges are used instead of meek if meek is selected in Tor Browser for Android alpha

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-parity, tbb-regression, TorBrowserTeam202004, tbb-9.5a12
Cc: sisbell, sysrqb Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (12)

comment:1 Changed 5 weeks ago by gk

STR: Take a new 9.5a11 and select meek during start-up. Resume connecting to the Tor network and watch the log. obfs4 bridge details show up instead of meek ones.

comment:2 Changed 5 weeks ago by sysrqb

I think I found the bug:
service/src/main/java/org/torproject/android/service/CustomTorInstaller.java

    public 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.java

    TorConfigBuilder 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, 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) an obfs4 bridge.

comment:3 Changed 4 weeks ago by sysrqb

Status: newneeds_review

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_00 in my tor-android-service repo for one piece of this.

https://gitweb.torproject.org/user/sysrqb/tor-android-service.git/commit/?h=bug33931_00&id=769b3c85de468bb23fbb891266ab6cbb9c662e13

And branch bug33931_00 in my tor-browser-build repo:

https://gitweb.torproject.org/user/sysrqb/tor-browser-build.git/commit/?h=bug33931_00&id=91e6aec4f60783fc0008d4d3c60c29ddecafac0d

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.

comment:4 Changed 4 weeks ago by cypherpunks

TorBrowserTeam202004R
s/first-byte/first byte

// Terrible hack.

Indeed.

comment:5 Changed 4 weeks ago by sysrqb

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202004 removed

comment:6 Changed 4 weeks ago by 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.

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?

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

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

comment:7 in reply to:  6 ; Changed 4 weeks ago by 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.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!

comment:8 in reply to:  7 ; Changed 4 weeks ago by gk

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

I am not exactly sure if that's what you meant but I think the case where bridgeType for whatever reason is still 0 at the end of openBridgeSream() should be treated in addBridgesFromResources() in the throw block. That is, there should not be a case 0 check 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!

comment:9 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202004R removed
Status: needs_reviewneeds_revision

comment:10 in reply to:  8 ; Changed 4 weeks ago by sysrqb

Status: needs_revisionneeds_review

Replying to gk:

Replying to sysrqb:

Replying to acat:

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!

I am not exactly sure if that's what you meant but I think the case where bridgeType for whatever reason is still 0 at the end of openBridgeSream() should be treated in addBridgesFromResources() in the throw block. That is, there should not be a case 0 check 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 throw at the end of openBridgeStream(), 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_00 branch, and I pushed a new tor-browser-build branch bug33931_01 containing an updated TOPL patch.

https://gitweb.torproject.org/user/sysrqb/tor-android-service.git/commit/?h=bug33931_00&id=42d2cdb46542ff488ce0ed4bf9e5b41b3a8356a7

https://gitweb.torproject.org/user/sysrqb/tor-browser-build.git/commit/?h=bug33931_01&id=04da65342a76f74b3d4a58601f326ded457dc97a

comment:11 in reply to:  10 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

Replying to gk:

Replying to sysrqb:

Replying to acat:

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!

I am not exactly sure if that's what you meant but I think the case where bridgeType for whatever reason is still 0 at the end of openBridgeSream() should be treated in addBridgesFromResources() in the throw block. That is, there should not be a case 0 check 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 throw at the end of openBridgeStream(), 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_00 branch, and I pushed a new tor-browser-build branch bug33931_01 containing an updated TOPL patch.

https://gitweb.torproject.org/user/sysrqb/tor-android-service.git/commit/?h=bug33931_00&id=42d2cdb46542ff488ce0ed4bf9e5b41b3a8356a7

https://gitweb.torproject.org/user/sysrqb/tor-browser-build.git/commit/?h=bug33931_01&id=04da65342a76f74b3d4a58601f326ded457dc97a

Looks good to me. Squashed and merged to tor-android-service's master (commit ed2e1479eeddede01b1d510ef79dc5ec798b39c0) and merged to tor-browser-build's master + a commit to pick the tor-android-service changes up (commits 04da65342a76f74b3d4a58601f326ded457dc97a and b858dca8745b583afd2660389ccd5a96630154a0).

comment:12 Changed 3 weeks ago by sysrqb

Keywords: tbb-9.5a12 added
Note: See TracTickets for help on using tickets.