Opened 12 months ago

Closed 8 months ago

#25851 closed defect (fixed)

TBA - Make sure third-party code is proxy safe

Reported by: sysrqb Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-proxy-bypass
Cc: Actual Points:
Parent ID: #21863 Points:
Reviewer: Sponsor: Sponsor4

Description

It looks like Picasso (for image download and rendering) create connections that aren't proxy safe. There is other third party code that does this, as well, but we should never use leanplum (telemetry). We should audit httpclientandroidlib and confirm the connections are correctly proxying.

$ git grep -n openConnection\( mobile/android/thirdparty/
mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ClientConnectionOperator.java:78:    void openConnection(OperatedClientConnection conn,
mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java:144:    public void openConnection(
mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/ManagedClientConnectionImpl.java:304:        this.operator.openConnection(
mobile/android/thirdparty/com/leanplum/internal/SocketIOClient.java:82:    HttpURLConnection connection = (HttpURLConnection) url.openConnection();
mobile/android/thirdparty/com/leanplum/internal/Util.java:540:    HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection();
mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:46:  protected HttpURLConnection openConnection(Uri path) throws IOException {
mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:47:    HttpURLConnection connection = (HttpURLConnection) new URL(path.toString()).openConnection();
mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:58:    HttpURLConnection connection = openConnection(uri);

This isn't the only offending method, we should audit these thoroughly.

Child Tickets

TicketStatusOwnerSummaryComponent
#27012closedtbb-teamTBA: Audit thirdparty bookingApplications/Tor Browser
#27013closedtbb-teamTBA: Audit thirdparty selfbrailleApplications/Tor Browser
#27014closedtbb-teamTBA: Audit thirdparty disklrucacheApplications/Tor Browser
#27015closedtbb-teamTBA: Audit thirdparty leakcanaryApplications/Tor Browser
#27016closedtbb-teamTBA: Audit thirdparty picassoApplications/Tor Browser
#27017closedtbb-teamTBA: Audit thirdparty jsonApplications/Tor Browser
#27018closedtbb-teamTBA: Audit thirdparty dspecApplications/Tor Browser
#27019closedtbb-teamTBA: Audit thirdparty apache/commons/codecApplications/Tor Browser

Change History (4)

comment:1 in reply to:  description ; Changed 9 months ago by sysrqb

Replying to sysrqb:

$ git grep -n openConnection\( mobile/android/thirdparty/
mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/ClientConnectionOperator.java:78:    void openConnection(OperatedClientConnection conn,
mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/DefaultClientConnectionOperator.java:144:    public void openConnection(
mobile/android/thirdparty/ch/boye/httpclientandroidlib/impl/conn/ManagedClientConnectionImpl.java:304:        this.operator.openConnection(

#22170

mobile/android/thirdparty/com/leanplum/internal/SocketIOClient.java:82:    HttpURLConnection connection = (HttpURLConnection) url.openConnection();
mobile/android/thirdparty/com/leanplum/internal/Util.java:540:    HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection();

LeanPlum is not included by default. It is only included if MOZ_ANDROID_MMA is true (false by default) and MOZ_ANDROID_GCM must be true (which we set false at configure time):
https://gitweb.torproject.org/tor-browser.git/tree/.mozconfig-android?h=tor-browser-60.1.0esr-8.0-1&id=ce3ad196040db4886e953cf13fc8d24fdf712d4b#n34

mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:46:  protected HttpURLConnection openConnection(Uri path) throws IOException {
mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:47:    HttpURLConnection connection = (HttpURLConnection) new URL(path.toString()).openConnection();
mobile/android/thirdparty/com/squareup/picasso/UrlConnectionDownloader.java:58:    HttpURLConnection connection = openConnection(uri);

This isn't the only offending method, we should audit these thoroughly.

Code we should audit:

$ ls mobile/android/thirdparty/com/
adjust  booking  googlecode  jakewharton  leanplum  squareup
$ ls mobile/android/thirdparty/com/googlecode/eyesfree/braille/selfbraille/
ISelfBrailleService.java  SelfBrailleClient.java  WriteData.java
$ ls mobile/android/thirdparty/org/
json  lucasr  mozilla

comment:2 in reply to:  1 Changed 9 months ago by sysrqb

Replying to sysrqb:

Code we should audit:

$ ls mobile/android/thirdparty/com/
adjust  booking  googlecode  jakewharton  leanplum  squareup

Adjust is excluded at build-time, so we can ignore that. It is excluded if MOZ_INSTALL_TRACKING is not set. This is similar to LeanPlum - it requires MOZ_ANDROID_GCM, too. We could change the MOZ_INSTALL_TRACKING default value, too (being extra safe) - but currently it will not be included.

comment:3 in reply to:  1 Changed 9 months ago by sysrqb

Replying to sysrqb:

Code we should audit:

$ ls mobile/android/thirdparty/com/
adjust  booking  googlecode  jakewharton  leanplum  squareup

adjust - skip
booking - #27012
googlecode - #27013
jakewharton - #27014
leanplum - skip
squareup/leakcanary - #27015
squareup/picasso - #27016

$ ls mobile/android/thirdparty/org/
json  lucasr  mozilla

json - #27017
lucasr/dspec - #27018
mozilla/apache/commons/codec - #27019

comment:4 Changed 8 months ago by gk

Resolution: fixed
Status: newclosed

I think we are done here.

Note: See TracTickets for help on using tickets.