Opened 20 months ago

Closed 5 weeks ago

#21863 closed defect (fixed)

Ensure proxy safety on Android

Reported by: gk Owned by: sysrqb
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-7.0-must, tbb-proxy-bypass, TorBrowserTeam201808
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description (last modified by sysrqb)

Mike mentions in #21625:

Android stuff that definitely leaks that we should fix (missing proxy params to HttpUrlConnection - these need to use the buildHttpConnection helper to get a proxy):
 * mobile/android/base/java/org/mozilla/gecko/feeds/FeedFetcher.java
 * mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java
 * mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
 * mobile/android/thirdparty/com/keepsafe/switchboard/SwitchBoard.java

Blocker of releasing TBA.

Child Tickets

TicketStatusOwnerSummaryComponent
#18864closedMake sure Orfox is not affected by Android code using SOCKS_DGRAMApplications/Tor Browser
#19076closedtbb-teamMake sure that networking code in AndroidMediaResourceServer is no problem for OrFoxApplications/Tor Browser
#19077closedtbb-teamMake sure that networking code in sutagent is no problem for OrFoxApplications/Tor Browser
#22059closedtbb-teamRemove screencasting code on mobile as wellApplications/Tor Browser
#22170closedsysrqbCheck uses of ch.boye.httpclientandroidlib.impl.client.* for proxy safety on AndroidApplications/Tor Browser
#25851closedtbb-teamTBA - Make sure third-party code is proxy safeApplications/Tor Browser
#26028closedtbb-teamHLS Player doesn't use the centralized Proxy Selector.Applications/Tor Browser

Change History (37)

comment:1 Changed 19 months ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201704 removed

Moving our tickets to May 2017.

comment:2 Changed 18 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201705 removed

Moving our tickets to June.

comment:3 Changed 17 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:4 Changed 16 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:5 Changed 15 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:6 Changed 14 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:7 Changed 13 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:8 Changed 12 months ago by gk

Moving tickets to December 2017

comment:9 Changed 12 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:10 Changed 11 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:11 Changed 10 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:12 Changed 10 months ago by sysrqb

Parent ID: 19675

comment:13 Changed 10 months ago by sysrqb

Parent ID: 19675#19675

This may be a dup of #22170 and maybe #19077, #19076. It may be smart making this a parent of those.

comment:14 Changed 9 months ago by sysrqb

Parent ID: #19675#5709

Moving these to #5709, these aren't blockers anymore. We'll merge Orfox patches first, then audit everything as we move to m-c and work on TBA. (We should create more tickets as we find more items that need investigating/fixing)

comment:15 Changed 9 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:16 Changed 8 months ago by sysrqb

Description: modified (diff)
Keywords: tbb-proxy-bypass added; ff52-esr removed
Summary: Ensure proxy safety on Android when switching to ESR 52Ensure proxy safety on Android

We're past ESR 52, this is for whatever release we choose (probably 60).

comment:17 Changed 8 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:18 Changed 8 months ago by gk

Priority: MediumHigh

comment:19 Changed 7 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Move our roadmap tickets to May.

comment:20 Changed 6 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:21 Changed 6 months ago by gk

Priority: HighVery High

Upping prio.

comment:22 Changed 5 months ago by sysrqb

Owner: changed from tbb-team to sysrqb
Status: newaccepted

Auditing the code for network connections. On my first pass I see Mozilla already plugged many proxy-bypass calls. Most of the remaining instances are within the Firefox Accounts code. The telemetry code and mozstumbler have bypass bugs, too. We don't use telemetry, so that should not be a problem, but we will plug this hole when we patch the FxA bug. We should exclude mozstumbler at compile-time.

The main problem is in mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java. After many layers, I believe the bypass happens in mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java. In addition, both mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ssl/SSLConnectionSocketFactory.java and mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ssl/SSLSocketFactory.java leak. These should be solved in #22170.

File Analysis
mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java Proxy-bypass by BaseResource
mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java Proxy-bypass in makeConnection(), check UAS passed into constructor
mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/UdpDataSource.java Proxy-bypass, creates UDP socket
mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java Check FxAccount UserAgent, Check how now() is used, Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/oauth/FxAccountOAuthClient10.java Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/profile/FxAccountProfileClient10.java Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/browserid/verifier/BrowserIDRemoteVerifierClient10.java Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/browserid/verifier/BrowserIDRemoteVerifierClient20.java Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java Proxy-bypass using BaseResource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/MetaGlobal.java Likely Proxy-bypass, Check SyncStorageRecordRequest
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRecordRequest.java Proxy-bypass via Resource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java Proxy-bypass via Resource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/TLSSocketFactory.java Possible Proxy-bypass via ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory.createSocket()
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/downloaders/BatchingDownloader.java Proxy-bypass via Resource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/RecordUploadRunnable.java Proxy-bypass via Resource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/EnsureCrypto5KeysStage.java Proxy-bypass via Resource
mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchInfoCollectionsStage.java Likely Proxy-bypass
mobile/android/services/src/main/java/org/mozilla/gecko/tokenserver/TokenServerClient.java Proxy-bypass via Resource
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/AbstractCommunicator.java Proxy-bypass by URL.openConnection()

comment:23 Changed 5 months ago by sysrqb

(Oh, for completeness, mobile/android/thirdparty/com/leanplum/internal/WebSocketClient.java is problematic, too - but we should not include leanplum).

comment:24 Changed 5 months ago by sysrqb

In addition, I went down the rabbit hole: "What does Android *do* when you ask it to establish a connection using a proxy". The result of this long and windy path is "it seems safe", but only when the Java/Dalvik/ART VM uses the default AOSP configuration.

I'll attempt succinctly explaining proxy safety on Android here, but we should seriously consider using Necko for all networking calls in the future (which means exposing necko via jni). I believe GeckoView is already considering this - but how hard could it be? :)

As an example, let's look at Fennec's Updater download logic. Fennec uses the wrapper org.mozilla.gecko.util.ProxySelector.openConnectionWithProxy(java.net.URI) most places. This provides a central location where Fennec decides if a connection should be establishing via a proxy or if the connection should be direct. When openConnectionWithProxy() is called, Fennec asks the Dalvik/Art system for the currently configured proxies. From the list of returned proxies, Fennec chooses the first one. (Unfortunately, it seems like Fennec doesn't configure a proxy, it must be configured somewhere else).

openConnectionWithProxy() calls java.net.URL::openConnection(java.net.Proxy). This is the entrance into the Android SDK. openConnection() establishes the initial connection by passing the request onto another library. The protocol handler is dynamically found when the URL object is instantiated (and statically cached after that). It retrieves a list of protocol handlers from the system properties. This list may be empty. If the list is empty, or the system doesn't have a handler for the specific protocol, then it checks a set of default handlers. For the sake of simplicity, let's assume we're requesting a connection using the http protocol. That protocol defaults to using com.android.okhttp.HttpHandler. At this point, we go into the okhttp library with a call into its URLStreamHandler:openConnection() method. This brings us here.

The HttpHandler instantiates a helper OkHttpClient client which is responsible for the connection. The proxy is explicitly set, as well. From here, the newly instantiated connection is returned down the stack. The requested connect is established when the caller requests the InputStream or the OutputStream. At this time, a HttpEngine object is instantiated where a new StreamAllocation is created. The StreamAllocation is important because this is the next location where the Proxy information is handed-off from one layer to the next, and it is bundled into an Address. After creating the HttpHandler, execute() is called, next calling sendRequest() where connect() is called. Then the previously-created StreamAllocation finds the "route" it will use for its new "stream" (socket connection). The route is another abstraction where the Address and Proxy are bundled together. Next, connect() is called on the connection where the first socket is created via Socket. Socket then hands-off to SocksSocketImpl. After the SocksSocket s created, OkHttp calls a platform-specific connectSocket() method. On Android, this effectively calls connect() directly on the socket. Socket calls SocksSocketImpl where the connection is actually established with the proxy server. At this point, the SOCKS handshake continues synchronously. If connect() does not throw an exception and it returns, then the proxied connection was successfully established.

My main concern with this is the lack of control we have over this logic. I didn't audit the code from 5 years ago, and this could change at any time. Currently, it seems like Android respects a request by an app to use a proxy for a connection, but that could break at any time.

comment:25 Changed 5 months ago by sysrqb

Parent ID: #5709#26531

Required for first alpha.

comment:26 in reply to:  24 ; Changed 5 months ago by gk

Replying to sysrqb:

In addition, I went down the rabbit hole: "What does Android *do* when you ask it to establish a connection using a proxy". The result of this long and windy path is "it seems safe", but only when the Java/Dalvik/ART VM uses the default AOSP configuration.

I'll attempt succinctly explaining proxy safety on Android here, but we should seriously consider using Necko for all networking calls in the future (which means exposing necko via jni). I believe GeckoView is already considering this - but how hard could it be? :)

Yes, please. Do you have a ticket for GeckoView tracking this work? I looked a bit around but did not find anything.

comment:27 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

Moving first batch of tickets to July 2018

comment:28 Changed 4 months ago by n8fr8

I thought @amoghbl had found and patched all of these cases. We definitely focused on checking for DNS leaks in the past, using a variety of web based DNS leak test tools.

The goal has been to directly proxy everything using HTTP and SOCKs, and not rely on the Android OS proxy subsystems at all, since they are not reliable, especially on WAN networks.

This is also all related to the fact that there are 3 different HTTP packages in use within Fennec, making standardized proxying hard.

comment:29 Changed 4 months ago by toproxy

Tor browser for Android compiled from esr60.1 branch in sysrqb's Tor browser repository is leaking domains of visited websites. Orfox has the same proxy config values as TBA but Orfox does not leak DNS. So I've decided to test Firefox 52 and found that Firefox 52 leaks DNS just like TBA based on esr60. Were not all Orfox patches added to TBA?

It appears that this proxy-bypass happens after websites are completely loaded. And another thing to note is DNS requests for resources within websites are not leaked but that's not always the case.

comment:30 in reply to:  26 Changed 4 months ago by sysrqb

Replying to gk:

Replying to sysrqb:

In addition, I went down the rabbit hole: "What does Android *do* when you ask it to establish a connection using a proxy". The result of this long and windy path is "it seems safe", but only when the Java/Dalvik/ART VM uses the default AOSP configuration.

I'll attempt succinctly explaining proxy safety on Android here, but we should seriously consider using Necko for all networking calls in the future (which means exposing necko via jni). I believe GeckoView is already considering this - but how hard could it be? :)

Yes, please. Do you have a ticket for GeckoView tracking this work? I looked a bit around but did not find anything.

No, I dont, I'll ask the devs if they have an open bugzilla ticket.

comment:31 in reply to:  28 Changed 4 months ago by sysrqb

Replying to n8fr8:

I thought @amoghbl had found and patched all of these cases. We definitely focused on checking for DNS leaks in the past, using a variety of web based DNS leak test tools.

The goal has been to directly proxy everything using HTTP and SOCKs, and not rely on the Android OS proxy subsystems at all, since they are not reliable, especially on WAN networks.

This is also all related to the fact that there are 3 different HTTP packages in use within Fennec, making standardized proxying hard.

I agree. There were a lot of conflicts when I was applying the proxy-safe Orfox patches, so I did a lot of patching by hand. I wanted to be sure we didn't miss any new instances of proxy-bypass, so auditing the current code seemed like a good idea.

comment:32 in reply to:  29 Changed 4 months ago by sysrqb

Replying to toproxy:

Tor browser for Android compiled from esr60.1 branch in sysrqb's Tor browser repository is leaking domains of visited websites. Orfox has the same proxy config values as TBA but Orfox does not leak DNS. So I've decided to test Firefox 52 and found that Firefox 52 leaks DNS just like TBA based on esr60. Were not all Orfox patches added to TBA?

It appears that this proxy-bypass happens after websites are completely loaded. And another thing to note is DNS requests for resources within websites are not leaked but that's not always the case.

Hrm. I think that may be an old build. I'll upload a new one, and I hope you can test that and confirm it does not leak.

comment:33 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:34 Changed 3 months ago by gk

Resolution: fixed
Status: acceptedclosed

We are done here, thanks for the nice work done, sysrqb!

comment:35 Changed 3 months ago by towiw

@sysrqb: I tested it and I can confirm that it does not leak DNS anymore.

comment:36 Changed 2 months ago by traumschule

Parent ID: #26531
Resolution: fixed
Status: closedreopened

a user in #tor-mobile reported #27822 so i reopen this and remove the closed parent

comment:37 in reply to:  36 Changed 5 weeks ago by sysrqb

Resolution: fixed
Status: reopenedclosed

Replying to traumschule:

a user in #tor-mobile reported #27822 so i reopen this and remove the closed parent

Thanks traumschule. I'd prefer opening a new ticket for this.

Note: See TracTickets for help on using tickets.