From 421270ad5fddc3672291d72963d45985fdfe77fb Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 14 Feb 2019 14:25:46 +0100 Subject: [PATCH] handle implied READ_EXTERNAL_STORAGE permissions Having _WRITE_EXTERNAL_STORAGE_ will implied _READ_EXTERNAL_STORAGE_: https://developer.android.com/reference/android/Manifest.permission#READ_EXTERNAL_STORAGE closes #1702 --- app/src/androidTest/assets/extendedPerms.xml | 4 +- .../fdroid/installer/ApkVerifierTest.java | 93 +++++++++++++++++-- .../main/java/org/fdroid/fdroid/data/Apk.java | 14 +++ .../fdroid/fdroid/data/RepoXMLHandler.java | 5 + .../fdroid/updater/IndexV1UpdaterTest.java | 11 +++ 5 files changed, 119 insertions(+), 8 deletions(-) diff --git a/app/src/androidTest/assets/extendedPerms.xml b/app/src/androidTest/assets/extendedPerms.xml index a573dfa28..d3b612c75 100644 --- a/app/src/androidTest/assets/extendedPerms.xml +++ b/app/src/androidTest/assets/extendedPerms.xml @@ -68,7 +68,7 @@ - + @@ -85,7 +85,7 @@ 23 2016-06-26 - org.dmfs.permission.READ_TASKS,READ_EXTERNAL_STORAGE,WRITE_CONTACTS,GET_ACCOUNTS,AUTHENTICATE_ACCOUNTS,WRITE_EXTERNAL_STORAGE,READ_CALENDAR,ACCESS_WIFI_STATE,org.dmfs.permission.WRITE_TASKS,ACCESS_NETWORK_STATE,WRITE_CALENDAR,READ_CONTACTS,READ_SYNC_SETTINGS,INTERNET,MANAGE_ACCOUNTS,WRITE_SYNC_SETTINGS + org.dmfs.permission.READ_TASKS,WRITE_CONTACTS,GET_ACCOUNTS,AUTHENTICATE_ACCOUNTS,WRITE_EXTERNAL_STORAGE,READ_CALENDAR,ACCESS_WIFI_STATE,org.dmfs.permission.WRITE_TASKS,ACCESS_NETWORK_STATE,WRITE_CALENDAR,READ_CONTACTS,READ_SYNC_SETTINGS,INTERNET,MANAGE_ACCOUNTS,WRITE_SYNC_SETTINGS diff --git a/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java b/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java index 9ee53fcdf..cf8f267f7 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java +++ b/app/src/androidTest/java/org/fdroid/fdroid/installer/ApkVerifierTest.java @@ -26,13 +26,12 @@ import android.support.annotation.NonNull; import android.support.test.InstrumentationRegistry; import android.support.test.runner.AndroidJUnit4; import android.util.Log; - import org.fdroid.fdroid.AssetUtils; -import org.fdroid.fdroid.data.RepoXMLHandler; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.compat.FileCompatTest; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.Repo; +import org.fdroid.fdroid.data.RepoXMLHandler; import org.fdroid.fdroid.mock.RepoDetails; import org.junit.Before; import org.junit.Test; @@ -45,6 +44,7 @@ import java.io.InputStream; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; +import java.util.TreeSet; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -103,7 +103,7 @@ public class ApkVerifierTest { public void testNulls() { assertTrue(ApkVerifier.requestedPermissionsEqual(null, null)); - String[] perms = new String[] {"Blah"}; + String[] perms = new String[]{"Blah"}; assertFalse(ApkVerifier.requestedPermissionsEqual(perms, null)); assertFalse(ApkVerifier.requestedPermissionsEqual(null, perms)); } @@ -290,7 +290,7 @@ public class ApkVerifierTest { public void testExtendedPerms() throws IOException, ApkVerifier.ApkPermissionUnequalException, ApkVerifier.ApkVerificationException { RepoDetails actualDetails = getFromFile(extendedPermsXml); - HashSet expectedSet = new HashSet<>(Arrays.asList(new String[]{ + HashSet expectedSet = new HashSet<>(Arrays.asList( "android.permission.ACCESS_NETWORK_STATE", "android.permission.ACCESS_WIFI_STATE", "android.permission.INTERNET", @@ -301,8 +301,8 @@ public class ApkVerifierTest { "android.permission.READ_CONTACTS", "android.permission.WRITE_CONTACTS", "android.permission.READ_CALENDAR", - "android.permission.WRITE_CALENDAR", - })); + "android.permission.WRITE_CALENDAR" + )); if (Build.VERSION.SDK_INT <= 18) { expectedSet.add("android.permission.READ_EXTERNAL_STORAGE"); expectedSet.add("android.permission.WRITE_EXTERNAL_STORAGE"); @@ -345,6 +345,87 @@ public class ApkVerifierTest { apkVerifier.verifyApk(); } + @Test + public void testImpliedPerms() throws IOException { + RepoDetails actualDetails = getFromFile(extendedPermsXml); + TreeSet expectedSet = new TreeSet<>(Arrays.asList( + "android.permission.ACCESS_NETWORK_STATE", + "android.permission.ACCESS_WIFI_STATE", + "android.permission.INTERNET", + "android.permission.READ_CALENDAR", + "android.permission.READ_CONTACTS", + "android.permission.READ_EXTERNAL_STORAGE", + "android.permission.READ_SYNC_SETTINGS", + "android.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS", + "android.permission.WRITE_CALENDAR", + "android.permission.WRITE_CONTACTS", + "android.permission.WRITE_EXTERNAL_STORAGE", + "android.permission.WRITE_SYNC_SETTINGS", + "org.dmfs.permission.READ_TASKS", + "org.dmfs.permission.WRITE_TASKS" + )); + if (Build.VERSION.SDK_INT <= 22) { // maxSdkVersion="22" + expectedSet.addAll(Arrays.asList( + "android.permission.AUTHENTICATE_ACCOUNTS", + "android.permission.GET_ACCOUNTS", + "android.permission.MANAGE_ACCOUNTS" + )); + } + Apk apk = actualDetails.apks.get(1); + Log.i(TAG, "APK: " + apk.apkName); + HashSet actualSet = new HashSet<>(Arrays.asList(apk.requestedPermissions)); + for (String permission : expectedSet) { + if (!actualSet.contains(permission)) { + Log.i(TAG, permission + " in expected but not actual! (android-" + + Build.VERSION.SDK_INT + ")"); + } + } + for (String permission : actualSet) { + if (!expectedSet.contains(permission)) { + Log.i(TAG, permission + " in actual but not expected! (android-" + + Build.VERSION.SDK_INT + ")"); + } + } + String[] expectedPermissions = expectedSet.toArray(new String[expectedSet.size()]); + assertTrue(ApkVerifier.requestedPermissionsEqual(expectedPermissions, apk.requestedPermissions)); + + expectedSet = new TreeSet<>(Arrays.asList( + "android.permission.ACCESS_NETWORK_STATE", + "android.permission.ACCESS_WIFI_STATE", + "android.permission.AUTHENTICATE_ACCOUNTS", + "android.permission.GET_ACCOUNTS", + "android.permission.INTERNET", + "android.permission.MANAGE_ACCOUNTS", + "android.permission.READ_CALENDAR", + "android.permission.READ_CONTACTS", + "android.permission.READ_EXTERNAL_STORAGE", + "android.permission.READ_SYNC_SETTINGS", + "android.permission.WRITE_CALENDAR", + "android.permission.WRITE_CONTACTS", + "android.permission.WRITE_EXTERNAL_STORAGE", + "android.permission.WRITE_SYNC_SETTINGS", + "org.dmfs.permission.READ_TASKS", + "org.dmfs.permission.WRITE_TASKS" + )); + expectedPermissions = expectedSet.toArray(new String[expectedSet.size()]); + apk = actualDetails.apks.get(2); + Log.i(TAG, "APK: " + apk.apkName); + actualSet = new HashSet<>(Arrays.asList(apk.requestedPermissions)); + for (String permission : expectedSet) { + if (!actualSet.contains(permission)) { + Log.i(TAG, permission + " in expected but not actual! (android-" + + Build.VERSION.SDK_INT + ")"); + } + } + for (String permission : actualSet) { + if (!expectedSet.contains(permission)) { + Log.i(TAG, permission + " in actual but not expected! (android-" + + Build.VERSION.SDK_INT + ")"); + } + } + assertTrue(ApkVerifier.requestedPermissionsEqual(expectedPermissions, apk.requestedPermissions)); + } + @NonNull private RepoDetails getFromFile(File indexFile) throws IOException { InputStream inputStream = null; diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 66a815fff..0cdae110c 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -1,5 +1,6 @@ package org.fdroid.fdroid.data; +import android.Manifest; import android.annotation.TargetApi; import android.content.ContentValues; import android.content.Context; @@ -486,6 +487,16 @@ public class Apk extends ValueObject implements Comparable, Parcelable { setRequestedPermissions(permissions, 23); } + /** + * Generate the set of requested permissions for the current Android version. + *

+ * There are also a bunch of crazy rules where having one permission will imply + * another permission, for example, {@link Manifest.permission#WRITE_EXTERNAL_STORAGE} + * implies {@code Manifest.permission#READ_EXTERNAL_STORAGE}. Many of these rules + * are for quite old Android versions, so they are not included here. + * + * @see Manifest.permission#READ_EXTERNAL_STORAGE + */ private void setRequestedPermissions(Object[][] permissions, int minSdk) { HashSet set = new HashSet<>(); if (requestedPermissions != null) { @@ -500,6 +511,9 @@ public class Apk extends ValueObject implements Comparable, Parcelable { set.add((String) versions[0]); } } + if (Build.VERSION.SDK_INT >= 16 && set.contains(Manifest.permission.WRITE_EXTERNAL_STORAGE)) { + set.add(Manifest.permission.READ_EXTERNAL_STORAGE); + } requestedPermissions = set.toArray(new String[set.size()]); } diff --git a/app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java b/app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java index 1da274f63..58287bdda 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java @@ -19,6 +19,7 @@ package org.fdroid.fdroid.data; +import android.Manifest; import android.os.Build; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -98,6 +99,10 @@ public class RepoXMLHandler extends DefaultHandler { if ("application".equals(localName) && curapp != null) { onApplicationParsed(); } else if ("package".equals(localName) && curapk != null && curapp != null) { + if (Build.VERSION.SDK_INT >= 16 && + requestedPermissionsSet.contains(Manifest.permission.WRITE_EXTERNAL_STORAGE)) { + requestedPermissionsSet.add(Manifest.permission.READ_EXTERNAL_STORAGE); + } int size = requestedPermissionsSet.size(); curapk.requestedPermissions = requestedPermissionsSet.toArray(new String[size]); requestedPermissionsSet.clear(); diff --git a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java index cc9d6394d..3a8d231ef 100644 --- a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java @@ -38,6 +38,8 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Field; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -53,6 +55,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -132,6 +135,14 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { InstalledAppTestUtils.install(context, "com.waze", 1019841, "v3.9.5.4", "362488e7be5ea0689b4e97d989ae1404", "cbbdb8c5dafeccd7dd7b642dde0477d3489e18ac366e3c8473d5c07e5f735a95"); assertEquals(1, AppProvider.Helper.findInstalledAppsWithKnownVulns(context).size()); + + Apk apk = ApkProvider.Helper.findApkFromAnyRepo(context, "io.proto.player", 1110); + assertNotNull("We should find this APK", apk); + assertEquals("io.proto.player-1.apk", apk.apkName); + HashSet requestedPermissions = new HashSet<>(Arrays.asList(apk.requestedPermissions)); + assertTrue(requestedPermissions.contains(android.Manifest.permission.READ_EXTERNAL_STORAGE)); + assertTrue(requestedPermissions.contains(android.Manifest.permission.WRITE_EXTERNAL_STORAGE)); + assertFalse(requestedPermissions.contains(android.Manifest.permission.READ_CALENDAR)); } @Test(expected = IndexUpdater.SigningException.class)