Opened 2 years ago

Closed 13 months ago

Last modified 10 months ago

#22170 closed defect (fixed)

Check uses of ch.boye.httpclientandroidlib.impl.client.* for proxy safety on Android

Reported by: gk Owned by: sysrqb
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, tbb-mobile, TorBrowserTeam201808R
Cc: tbb-team Actual Points:
Parent ID: #21863 Points:
Reviewer: Sponsor: Sponsor8

Description

We should check ch.boye.httpclientandroidlib.impl.client.* for proxy compliance. If we don't get to it earlier then it is a blocker for our Tor Browser on mobile.

Child Tickets

Change History (32)

comment:1 Changed 2 years ago by tom

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1357997#c9 and hopefully subsequent comments.

comment:2 Changed 22 months ago by gk

Keywords: TorBrowserTeam201712 added

comment:4 Changed 21 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:5 Changed 20 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:6 Changed 20 months ago by sysrqb

Parent ID: 19675

comment:7 Changed 20 months ago by sysrqb

Parent ID: 19675#19675

comment:8 Changed 19 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:9 Changed 19 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:10 Changed 18 months ago by gk

Parent ID: #5709#21863

comment:11 Changed 18 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:12 Changed 18 months ago by gk

Priority: MediumHigh

comment:13 Changed 17 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Move our roadmap tickets to May.

comment:14 Changed 16 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:15 Changed 15 months ago by sysrqb

As mentioned in the parent ticket, we should look at these are plug the holes:

mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java
mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ssl/SSLConnectionSocketFactory.java 
mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ssl/SSLSocketFactory.java

This is a third-party library, so we may need to hardcode the proxy settings if there isn't existing logic for configuring a proxy.

comment:16 Changed 15 months ago by sysrqb

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

comment:17 Changed 15 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

Moving first batch of tickets to July 2018

comment:18 Changed 14 months ago by sysrqb

This code is kinda scary. It's highly configurable, so we must be very careful that we don't miss something.

In mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java, when the connection is instantiated we configure the default proxy on the client connection.

HttpClientAndroidLib is a crazy web of abstractions over HTTP connections. It uses connection pools for reusing existing connections, it uses routes for retrying connection requests that failed on different interfaces and/or using other proxy servers.

As long as the PlainSocketFactory and SSLSocketFactory are instantiated without setting nameResolver, we should not leak the DNS lookup.

The client is created as a DefaultHttpClient [0]. This is where we hard-code the proxy config:

    HttpHost defaultProxy = new HttpHost(ProxySelector.getProxyHostAddress(),
                                         ProxySelector.getHttpProxyPort());
    client.getParams().setParameter(ConnRoutePNames.DEFAULT_PROXY, defaultProxy);

getParams() returns a HttpParams [1] which is an instance of SyncBasicHttpParams [2]. Then, when client.execute() is called [3], after some levels of abstraction a RequestDirector is created as a DefaultRequestDirector[4]. Here a BasicClientConnectionManager is created as the ClientConnectionManager. In the RequestDirector.execute() method, the request's HttpRoute is found via the DefaultRoutePlanner[6] (created when the Director was created[7]). This is where the default proxy is checked (as it is set above) [8] and this information is passed into the HttpRoute constructor[9]. This configures the proxyChain array of proxies used by this route.

Inside RequestDirector.execute(), at the first connection a new connection is created by calling connManager.requestConnection() in BasicClientConnectionManager. This then creates a new ClientConnectionOperator as a DefaultClientConnectionOperator[10]. Then an OperatedClientConnection is created by DefaultClientConnectionOperator.createConnection(). last a ManagedClientConnectionImpl [11] is created and returned.

Later in execute(), tryConnect() is called, where ManagedClientConnectionImpl.open() is then called. Here, DefaultClientConnectionOperator.open() connection is called where the target is the previously configured proxy [12]. In this method, a Socket is created by the respective Scheme factory for the proxy.

NOTE: this resolved the proxy address using the system DNS resolver [13]. This shouldn't leak anything, but we don't need this.

[0] mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/client/DefaultHttpClient.java
[1] mobile/android/thirdparty/ch/boye/httpclientandroidlib/params/HttpParams.java
[2] mobile/android/thirdparty/ch/boye/httpclientandroidlib/params/SyncBasicHttpParams.java
[3] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java?h=tor-browser-60.1.0esr-8.0-1#n315
[4] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/client/AbstractHttpClient.java?h=tor-browser-60.1.0esr-8.0-1#n805
[5] mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/BasicClientConnectionManager.java
[6] mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultHttpRoutePlanner.java
[7] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/client/AbstractHttpClient.java?h=tor-browser-60.1.0esr-8.0-1#n811
[8] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/params/ConnRouteParams.java?h=tor-browser-60.1.0esr-8.0-1#n68
[9] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultHttpRoutePlanner.java?h=tor-browser-60.1.0esr-8.0-1#n118
[10] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/BasicClientConnectionManager.java?h=tor-browser-60.1.0esr-8.0-1#n167
[11] mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/ManagedClientConnectionImpl.java
[12] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/ManagedClientConnectionImpl.java?h=tor-browser-60.1.0esr-8.0-1#n304
[13] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java?h=tor-browser-60.1.0esr-8.0-1#n159

comment:19 Changed 14 months ago by gk

Cc: tbb-team added

comment:20 Changed 14 months ago by sysrqb

All files where Fennec uses impl.client

$ git grep -n ch.boye.httpclientandroidlib.impl.client mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:15:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:50:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/oauth/FxAccountAbstractClient.java:30:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:35:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AbstractBearerTokenAuthHeaderProvider.java:9:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AuthHeaderProvider.java:11:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:51:import ch.boye.httpclientandroidlib.impl.client.BasicAuthCache;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:52:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResourceDelegate.java:8:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BasicAuthHeaderProvider.java:12:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/HMACAuthHeaderProvider.java:23:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/HawkAuthHeaderProvider.java:29:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/ResourceDelegate.java:13:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageCollectionRequest.java:20:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java:20:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/main/java/org/mozilla/gecko/tokenserver/TokenServerClient.java:37:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/test/java/org/mozilla/android/sync/test/helpers/MockResourceDelegate.java:9:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/test/java/org/mozilla/gecko/sync/net/test/TestHawkAuthHeaderProvider.java:12:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/test/java/org/mozilla/gecko/sync/net/test/TestLiveHawkAuth.java:11:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

All files where Fennec uses conn

$ git grep -n ch.boye.httpclientandroidlib.conn mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:14:import ch.boye.httpclientandroidlib.conn.util.InetAddressUtils;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:44:import ch.boye.httpclientandroidlib.conn.ClientConnectionManager;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:45:import ch.boye.httpclientandroidlib.conn.params.ConnRoutePNames;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:46:import ch.boye.httpclientandroidlib.conn.scheme.PlainSocketFactory;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:47:import ch.boye.httpclientandroidlib.conn.scheme.Scheme;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:48:import ch.boye.httpclientandroidlib.conn.scheme.SchemeRegistry;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:49:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/TLSSocketFactory.java:16:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/TLSSocketFactory.java is now dead code since Bug 1061273 (originally imported in Bug 709391 with only one caller)

$ git grep -n ch.boye.httpclientandroidlib.impl.conn mobile/android/[bs]*
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:53:import ch.boye.httpclientandroidlib.impl.conn.tsccm.ThreadSafeClientConnManager;

I don't see any problematic usage in ch.boye.httpclientandroidlib.client.*.

comment:21 in reply to:  20 ; Changed 14 months ago by sysrqb

Replying to sysrqb:

All files where Fennec uses impl.client

$ git grep -n ch.boye.httpclientandroidlib.impl.client mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:15:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

We should never get here because its telemetry, but it's worth checking. The DefaultHttpClient is passed in, but not created. The DATE headers is set. A BaseResource is created and BaseResource.postBlocking() is called. The proxy will be set within BaseResource.execute().

mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:50:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

All connections are created via BaseResource. DefaultHttpClient is passed into an addHeader() where an ACCEPT_LANGAUGE and ACCEPT header is added.

Note: FxA uses a unique user agent string in its request.
https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountConstants.java?h=tor-browser-60.1.0esr-8.0-1#n40

mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/oauth/FxAccountAbstractClient.java:30:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

DefaultHttpClient is passed into an addHeader() where an ACCEPT_LANGAUGE and ACCEPT header is added.

mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:35:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
/**
 * Interact with the autopush endpoint HTTP API.
 * <p/>
 * The API is a Mozilla-proprietary interface, and not even specified to Mozilla's usual ad-hoc standards.
 * This client is written against a work-in-progress, un-deployed upstream commit.
 */

That's reassuring.

All connections are created via BaseResource. DefaultHttpClient is passed into an addHeader() where an ACCEPT_LANGAUGE and ACCEPT header is added.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AbstractBearerTokenAuthHeaderProvider.java:9:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

DefaultHttpClient isn't used. No network calls in this class.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AuthHeaderProvider.java:11:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This is an interface, no logic here.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:51:import ch.boye.httpclientandroidlib.impl.client.BasicAuthCache;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:52:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This class is probably proxy-safe. I'll need to look at this again (and a second pair of eyes would be welcome).

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResourceDelegate.java:8:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This class only provides accessors and mutators, no network calls.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BasicAuthHeaderProvider.java:12:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

No network calls.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/HMACAuthHeaderProvider.java:23:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

DefaultHttpClient isn't used. No network calls.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/HawkAuthHeaderProvider.java:29:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

DefaultHttpClient isn't used. No network calls.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/ResourceDelegate.java:13:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This is an interface, no logic here.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageCollectionRequest.java:20:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
// TODO: this is awful.

Sets ACCEPT header. This class mostly handles HTTP responses.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java:20:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

Adds a x-if-unmodified-since header. Uses BaseResource for creating network connections.

Note: uses another different user agent string.
https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/services/src/main/java/org/mozilla/gecko/sync/SyncConstants.java?h=tor-browser-60.1.0esr-8.0-1#n40

mobile/android/services/src/main/java/org/mozilla/gecko/tokenserver/TokenServerClient.java:37:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

Sets X-Conditions-Accepted and X-Client-State headers. Uses BaseResource for networking.

mobile/android/services/src/test/java/org/mozilla/android/sync/test/helpers/MockResourceDelegate.java:9:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/test/java/org/mozilla/gecko/sync/net/test/TestHawkAuthHeaderProvider.java:12:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
mobile/android/services/src/test/java/org/mozilla/gecko/sync/net/test/TestLiveHawkAuth.java:11:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

Testing.

comment:22 in reply to:  20 ; Changed 14 months ago by sysrqb

Replying to sysrqb:

All files where Fennec uses conn

$ git grep -n ch.boye.httpclientandroidlib.conn mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:14:import ch.boye.httpclientandroidlib.conn.util.InetAddressUtils;

Only used for parsing a string. This class is only a utility, it doesn't create any connections.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:44:import ch.boye.httpclientandroidlib.conn.ClientConnectionManager;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:45:import ch.boye.httpclientandroidlib.conn.params.ConnRoutePNames;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:46:import ch.boye.httpclientandroidlib.conn.scheme.PlainSocketFactory;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:47:import ch.boye.httpclientandroidlib.conn.scheme.Scheme;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:48:import ch.boye.httpclientandroidlib.conn.scheme.SchemeRegistry;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:49:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;

This is proxy-safe but only because we hard-code the default HTTP proxy. scheme.PlainSocketFactory and ssl.SSLSocketFactory are used for establishing a connection to the proxy, instead of the destination. params.ConnRoutePNames is used for specifying the default proxy. scheme.Scheme, scheme.SchemeRegistry, and ClientConnectionManager are used during instantiation of the connection manager (ThreadSafeClientConnManager).

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/TLSSocketFactory.java:16:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;

Dead class.

comment:23 Changed 14 months ago by sysrqb

The only remaining code that can bypass the proxy is in Adjust. We don't include Adjust at build-time, so this is not a problem.

For those wondering "What is Adjust?":

Fennec (Firefox for Android) tracks certain types of installs using a third party install tracking
framework called Adjust.  The intention is to determine the origin of Fennec installs by answering
the question, "Did this user on this device install Fennec in response to a specific advertising
campaign performed by Mozilla?"

mobile/android/docs/adjust.rst

All other usage of ch.boye.httpclientandroidlib.impl.client.* is via BaseResource which is proxy-safe.

comment:24 Changed 14 months ago by sysrqb

Status: acceptedneeds_review

Any other thoughts or concerns here?

comment:25 Changed 14 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201807 removed

comment:26 Changed 14 months ago by gk

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201807R removed

comment:27 in reply to:  18 Changed 14 months ago by sysrqb

Replying to sysrqb:

NOTE: this resolved the proxy address using the system DNS resolver [13]. This shouldn't leak anything, but we don't need this.
[13] https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java?h=tor-browser-60.1.0esr-8.0-1#n159

This actually uses java.net.InetAddress.getAllByName() which resolves a host name or parses the a literal IP address:

The host name can either be a machine name, such as "java.sun.com", or a
textual representation of its IP address. If a literal IP address is
supplied, only the validity of the address format is checked. 

https://developer.android.com/reference/java/net/InetAddress.html#getAllByName(java.lang.String)

We are currently hard-coding an IP address, so this should not be a problem - in theory. The main problem here is we configure the proxy using an IP address, but that is stored as a string when it is passed around between the different layers of abstraction as a HTTP parameter. The IP address-as-a-String is then parsed into a InetAddress when the connection is created. I see four options here:

  1. We leave this as it is and assume this the Android API "does the right thing"
  2. We hard-code the InetAddress at this point in the code, too
  3. We modify httpclientandroidlib so the DEFAULT_PROXY parameter is stored as a URI so we never need to reparse the address
  4. We copy another IP address parsing implementation and use that instead of relying on the Android implementation (something like Google Guava's which provides similar functionality but does not perform name resolution - InetAddresses.forString(String))
     * <p><b>Important note:</b> Unlike {@code InetAddress.getByName()}, the methods of this class never
     * cause DNS services to be accessed. For this reason, you should prefer these methods as much as
     * possible over their JDK equivalents whenever you are expecting to handle only IP address string
     * literals -- there is no blocking DNS penalty for a malformed string.
    

https://github.com/google/guava/blob/master/android/guava/src/com/google/common/net/InetAddresses.java#L128

I'm more in favor or (2) or (4).

comment:28 Changed 13 months ago by gk

(2) seems like a good idea to me, I guess we can do this in a separate ticket, though?

comment:29 in reply to:  21 Changed 13 months ago by gk

Replying to sysrqb:

Replying to sysrqb:

All files where Fennec uses impl.client

$ git grep -n ch.boye.httpclientandroidlib.impl.client mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:15:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

We should never get here because its telemetry, but it's worth checking. The DefaultHttpClient is passed in, but not created. The DATE headers is set. A BaseResource is created and BaseResource.postBlocking() is called. The proxy will be set within BaseResource.execute().

You mean BaseResource.prepareClient()?

[snip]

/**
 * Interact with the autopush endpoint HTTP API.
 * <p/>
 * The API is a Mozilla-proprietary interface, and not even specified to Mozilla's usual ad-hoc standards.
 * This client is written against a work-in-progress, un-deployed upstream commit.
 */

That's reassuring.

Indeed.

All connections are created via BaseResource. DefaultHttpClient is passed into an addHeader() where an ACCEPT_LANGAUGE and ACCEPT header is added.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AbstractBearerTokenAuthHeaderProvider.java:9:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

DefaultHttpClient isn't used. No network calls in this class.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/AuthHeaderProvider.java:11:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This is an interface, no logic here.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:51:import ch.boye.httpclientandroidlib.impl.client.BasicAuthCache;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:52:import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;

This class is probably proxy-safe. I'll need to look at this again (and a second pair of eyes would be welcome).

Looks good to me.

[snip]

comment:30 in reply to:  22 Changed 13 months ago by gk

Replying to sysrqb:

Replying to sysrqb:

All files where Fennec uses conn

$ git grep -n ch.boye.httpclientandroidlib.conn mobile/android/[bs]*
mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:14:import ch.boye.httpclientandroidlib.conn.util.InetAddressUtils;

Only used for parsing a string. This class is only a utility, it doesn't create any connections.

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:44:import ch.boye.httpclientandroidlib.conn.ClientConnectionManager;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:45:import ch.boye.httpclientandroidlib.conn.params.ConnRoutePNames;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:46:import ch.boye.httpclientandroidlib.conn.scheme.PlainSocketFactory;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:47:import ch.boye.httpclientandroidlib.conn.scheme.Scheme;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:48:import ch.boye.httpclientandroidlib.conn.scheme.SchemeRegistry;
mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/BaseResource.java:49:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;

This is proxy-safe but only because we hard-code the default HTTP proxy. scheme.PlainSocketFactory and ssl.SSLSocketFactory are used for establishing a connection to the proxy, instead of the destination. params.ConnRoutePNames is used for specifying the default proxy. scheme.Scheme, scheme.SchemeRegistry, and ClientConnectionManager are used during instantiation of the connection manager (ThreadSafeClientConnManager).

mobile/android/services/src/main/java/org/mozilla/gecko/sync/net/TLSSocketFactory.java:16:import ch.boye.httpclientandroidlib.conn.ssl.SSLSocketFactory;

Dead class.

Looks good.

comment:31 Changed 13 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks for the detailed analysis, nice job! I think we are good here, closing.

comment:32 Changed 10 months ago by gk

Sponsor: Sponsor8

Sponsor8 in August 2018.

Note: See TracTickets for help on using tickets.