Opened 9 months ago

Closed 4 weeks ago

#24796 closed task (fixed)

Review all requested and required Android permissions

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

Description

The Orfox patches already disable/remove some of the Firefox permissions. Review/audit the remaining permissions and purge all remaining permissions we do not need.

Child Tickets

TicketStatusOwnerSummaryComponent
#26825closedtbb-teamTBA - Does the app need RECEIVE_BOOT_COMPLETED intent?Applications/Tor Browser
#26826closedtbb-teamTBA - Does the app need the SYSTEM_ALERT_WINDOW permission?Applications/Tor Browser

Change History (18)

comment:1 Changed 8 months ago by gk

Keywords: tbb-mobile added

comment:2 Changed 8 months ago by sysrqb

Parent ID: 19675

comment:3 Changed 8 months ago by sysrqb

Parent ID: 19675#19675

comment:4 Changed 7 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:5 Changed 5 months ago by sysrqb

Status: newneeds_information

Fennec currently requests/requires a large set of permissions. We should be able to reduce this. If we include the permissions requested by Fennec (base) and GeckoView, they are:

android.hardware.camera
android.hardware.camera.autofocus
android.hardware.location
android.hardware.location.gps
android.hardware.touchscreen
android.permission.ACCESS_COARSE_LOCATION
android.permission.ACCESS_FINE_LOCATION
android.permission.ACCESS_NETWORK_STATE
android.permission.ACCESS_WIFI_STATE
android.permission.AUTHENTICATE_ACCOUNTS
android.permission.CAMERA
android.permission.CHANGE_WIFI_STATE
android.permission.GET_ACCOUNTS
android.permission.INTERNET
android.permission.MANAGE_ACCOUNTS
android.permission.READ_EXTERNAL_STORAGE
android.permission.READ_SYNC_SETTINGS
android.permission.READ_SYNC_STATS
android.permission.RECEIVE_BOOT_COMPLETED
android.permission.SYSTEM_ALERT_WINDOW
android.permission.USE_CREDENTIALS
android.permission.VIBRATE
android.permission.WAKE_LOCK
android.permission.WRITE_EXTERNAL_STORAGE
android.permission.WRITE_SETTINGS
android.permission.WRITE_SYNC_SETTINGS
com.android.browser.permission.READ_HISTORY_BOOKMARKS
com.android.launcher.permission.INSTALL_SHORTCUT
com.android.launcher.permission.UNINSTALL_SHORTCUT

This includes permissions and features. Orfox already excludes some of the above (via compile-time pre-processor guards):

android.permission.CHANGE_WIFI_STATE
android.permission.ACCESS_WIFI_STATE
android.permission.ACCESS_FINE_LOCATION
android.hardware.location
android.hardware.location.gps
android.permission.CAMERA
android.hardware.camera
android.hardware.camera.autofocus
android.permission.GET_ACCOUNTS
android.permission.ACCESS_NETWORK_STATE
android.permission.MANAGE_ACCOUNTS

I think we can inherit this during the #25741 rebase, and audit the remaining perms after (or in parallel).

comment:6 in reply to:  5 Changed 5 months ago by sysrqb

Replying to sysrqb:

Fennec currently requests/requires a large set of permissions. We should be able to reduce this. If we include the permissions requested by Fennec (base) and GeckoView, they are:

Plus GCM (Google Cloud Messaging), and Orfox already excludes these, too:

com.google.android.c2dm.permission.RECEIVE

There are other GCM permissions requested when MOZ_ANDROID_GCM is defined, but we don't define it, so I'm not including them in this list (see GcmAndroidManifest_services.xml.in).

comment:7 Changed 3 months ago by sysrqb

Parent ID: #5709#26531

Required for first alpha.

comment:8 Changed 2 months ago by sysrqb

The permissions requested by Fennec (base) are reasonably reduced, but GeckoView still requests many permissions we do not want. Currently GeckoView's manifest (mobile/android/geckoview/src/main/AndroidManifest.xml) is not preprocessed, so we'll need to comment-out most of the same permissions as we ifdef-out in Fennec's permissions.

$ grep -n -e feature -e permission obj-arm-linux-androideabi/gradle/build/mobile/android/app/intermediates/manifests/full/officialWithoutGeckoBinariesNoMinApiPhoton/debug/AndroidManifest.xml
3:  <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
4:  <uses-permission android:name="android.permission.INTERNET"/>
5:  <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
6:  <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/>
7:  <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
8:  <uses-permission android:name="com.android.launcher.permission.INSTALL_SHORTCUT"/>
9:  <uses-permission android:name="com.android.launcher.permission.UNINSTALL_SHORTCUT"/>
10:  <uses-permission android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/>
11:  <uses-permission android:name="android.permission.WAKE_LOCK"/>
12:  <uses-permission android:name="android.permission.VIBRATE"/>
13:  <uses-feature android:name="android.hardware.touchscreen"/>
14:  <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>
15:  <uses-feature android:required="true" android:glEsVersion="0x00020000"/>
16:  <uses-permission android:name="android.permission.CHANGE_WIFI_STATE"/>
17:  <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
18:  <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>
19:  <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
20:  <uses-feature android:required="false" android:name="android.hardware.location"/>
21:  <uses-feature android:required="false" android:name="android.hardware.location.gps"/>
22:  <uses-permission android:name="android.permission.CAMERA"/>
23:  <uses-feature android:required="false" android:name="android.hardware.camera"/>
24:  <uses-feature android:required="false" android:name="android.hardware.camera.autofocus"/>
25:  <uses-permission android:name="android.permission.RECORD_AUDIO"/>
26:  <uses-feature android:required="false" android:name="android.hardware.audio.low_latency"/>
27:  <uses-feature android:required="false" android:name="android.hardware.microphone"/>
28:  <uses-feature android:required="false" android:name="android.hardware.camera.any"/>

All Permissions for Tor Browser for Android - as described by Android:

Camera:
take pictures and videos

Location:
access approximate location (network-basesd)
access precise location (GPS and network-based)

Microphone:
record audio

Storage:
read the contents of your USB storage
modify or delete the contents of your USB storage


Other:
view network connections
have full network access
run at startup
install shortcuts
uninstall shortcuts
prevent phone from sleeping
control vibration
connect and disconnect from Wi-Fi
view Wi-Fi connections

comment:9 Changed 2 months ago by sysrqb

I commented-out some of the permissions.

Branch 26401_1+24796 (based on the last branch for #26401)

$ grep -n -e feature -e permission obj-arm-linux-androideabi/gradle/build/mobile/android/app/intermediates/manifests/full/officialWithoutGeckoBinariesNoMinApiPhoton/debug/AndroidManifest.xml
3:  <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
4:  <uses-permission android:name="android.permission.INTERNET"/>
5:  <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
6:  <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/>
7:  <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
8:  <uses-permission android:name="com.android.launcher.permission.INSTALL_SHORTCUT"/>
9:  <uses-permission android:name="com.android.launcher.permission.UNINSTALL_SHORTCUT"/>
10:  <uses-permission android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/>
11:  <uses-permission android:name="android.permission.WAKE_LOCK"/>
12:  <uses-permission android:name="android.permission.VIBRATE"/>
13:  <uses-feature android:name="android.hardware.touchscreen"/>
14:  <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>
15:  <uses-feature android:required="true" android:glEsVersion="0x00020000"/>

Permissions shown by Android:

Storage:
read the contents of your USB storage
modify or delete the contents of your USB storage

Other:
view network connections
have full network access
run at startup
install shortcuts
uninstall shortcuts
prevent phone from sleeping
control vibration

Remaining permissions we should consider excluding:

android.permission.ACCESS_NETWORK_STATE
android.permission.SYSTEM_ALERT_WINDOW

And, I think, if we do not include the updater then we can likely exclude:

android.permission.READ_EXTERNAL_STORAGE
android.permission.READ_EXTERNAL_STORAGE

I'm not sure what Fennec does when it receives the BOOT_COMPLETED intent.
I'm also not sure how it uses SYSTEM_ALERT_WINDOW.

I'll move READ_HISTORY_BOOKMARKS under the MOZ_ANDROID_LOCATION ifdef guard for Fennec - including this permission likely breaks state separation.

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

Replying to sysrqb:

I'm not sure what Fennec does when it receives the BOOT_COMPLETED intent.

#26825

I'm also not sure how it uses SYSTEM_ALERT_WINDOW.

#26826

I'll move READ_HISTORY_BOOKMARKS under the MOZ_ANDROID_LOCATION ifdef guard for Fennec - including this permission likely breaks state separation.

We should include this as a FIXUP into the commit:

commit 85e08083165e95299b9ea4e96dd6d47bcd7dd1ee
Author: Matthew Finkel <Matthew.Finkel@gmail.com>
Date:   Mon Apr 16 04:22:51 2018 +0000

    Bug 25741 - TBA: Only include Firefox Account permissions if we want them

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

Replying to sysrqb:

Replying to sysrqb:

I'll move READ_HISTORY_BOOKMARKS under the MOZ_ANDROID_LOCATION ifdef guard for Fennec - including this permission likely breaks state separation.

We should include this as a FIXUP into the commit:

commit 85e08083165e95299b9ea4e96dd6d47bcd7dd1ee
Author: Matthew Finkel <Matthew.Finkel@gmail.com>
Date:   Mon Apr 16 04:22:51 2018 +0000

    Bug 25741 - TBA: Only include Firefox Account permissions if we want them

Okay done. https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-60.1.0esr-8.0-1&id=481f6b374dbf9a22348c24429fcd05eb12082449

comment:12 Changed 7 weeks ago by sysrqb

Keywords: TorBrowserTeam201808R added
Status: needs_informationneeds_review

I have branch 24796 ready for review. Basically, it comments-out permissions from GeckoView's AndroidManifest.xml, most of the permissions we already exclude from Fennec's main AndroidManifest.xml, but they are duplicated for GeckoView.

comment:13 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewneeds_revision

Could you please to the following things

1) If the #ifdefs don't work let's not include them as this is confusing. Rather we could fall back to the <!--#ifdef MOZ_WEBRTC--> style indicating where #ifdefs would be available if we had them

2) Could you put

+    <!--
     <uses-permission android:name="android.permission.CAMERA" />
     <uses-feature android:name="android.hardware.camera" android:required="false"/>
     <uses-feature android:name="android.hardware.camera.autofocus" android:required="false"/>
+    -->

into the WebRTC block so that it is easier to see that they belong together (assuming FennecManifest_permissions.xml.in has that part right).

3) The first part of the patch is

#ifdef MOZ_ANDROID_NETWORK_STATE
+    <!--
     <uses-permission android:name="android.permission.CHANGE_WIFI_STATE"/>
     <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
     <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>
     <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
     <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
+    -->
 #endif

but if you compare that to the respective part in FennecManifest_permissions.xml.in

#ifdef MOZ_ANDROID_NETWORK_STATE
    <!-- Android WIFI state -->
    <uses-permission android:name="android.permission.CHANGE_WIFI_STATE"/>
    <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
#endif

#ifdef MOZ_ANDROID_LOCATION
    <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
#endif
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>

that's not aligned (ACCESS_NETWORK_STATE is treated differently).

comment:14 Changed 4 weeks ago by sysrqb

An aside, external storage permissions:

modify or delete the contents of your SD card
read the content of your SD card

These permissions are used for updating (#26574), and for saving files: "Set Image As" saves the image on the external storage, "Save Image" (currently broken, #26317), video and audio capture. We can come back to this.

Remaining permissions (in addition to above external storage permissions):

full network access

automatically launch at start up

draw over other apps

control vibrations
prevent device from sleeping

install shortcuts
uninstall shortcuts

comment:15 in reply to:  13 Changed 4 weeks ago by sysrqb

Replying to gk:

3) The first part of the patch is
that's not aligned (ACCESS_NETWORK_STATE is treated differently).

Note, after excluding ACCESS_NETWORK_STATE, the app now crashes because it assumes it has that permission unconditionally. I think we can keep this for the first alpha, and consider refactoring the affected code and exclude the permission in a later alpha. #27217

--------- beginning of crash
08-20 12:01:14.662 26467 26467 E AndroidRuntime: FATAL EXCEPTION: main
08-20 12:01:14.662 26467 26467 E AndroidRuntime: Process: org.torproject.torbrowser_24796, PID: 26467
08-20 12:01:14.662 26467 26467 E AndroidRuntime: java.lang.SecurityException: ConnectivityService: Neither user 10152 nor current process has android.permission.ACCESS_NETWORK_STATE.
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at android.os.Parcel.readException(Parcel.java:2005)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at android.os.Parcel.readException(Parcel.java:1951)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at android.net.IConnectivityManager$Stub$Proxy.getActiveNetworkInfo(IConnectivityManager.java:1195)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at android.net.ConnectivityManager.getActiveNetworkInfo(ConnectivityManager.java:786)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.util.NetworkUtils.getConnectionType(NetworkUtils.java:123)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.util.NetworkUtils.isWifi(NetworkUtils.java:115)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.activitystream.homepanel.stream.StreamOverridablePageIconLayout.updateIcon(StreamOverridablePageIconLayout.java:92)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.activitystream.homepanel.stream.WebpageItemRow.bind(WebpageItemRow.java:85)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.onBindViewHolder(StreamRecyclerAdapter.java:202)
08-20 12:01:14.662 26467 26467 E AndroidRuntime: 	at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.onBindViewHolder(StreamRecyclerAdapter.java:51)

comment:16 Changed 4 weeks ago by sysrqb

Status: needs_revisionneeds_review

Okay. 24796_2 is ready for review.

comment:17 Changed 4 weeks ago by sysrqb

Also, just as a note: #27219

comment:18 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed
Resolution: fixed
Status: needs_reviewclosed

Looks good, cherry-picked to tor-browser-60.1.0esr-8.0-1 (commit d0ccd485e2bb22fea4d9d758906dcee7468b4a6a).

Note: See TracTickets for help on using tickets.