Apps using storage must have runtime permission.

A major goal of the Q release is to promote user transparency around
permission usage, and to also give user controls over those
permissions.  To further this goal, all apps requesting the
internal WRITE_MEDIA_STORAGE permission must also request (and be
granted) the "Storage" runtime permission in order to gain the
associated access.

If the user revokes the "Storage" runtime permission, then the app
must lose all access granted to it via WRITE_MEDIA_STORAGE.

Bug: 129144016
Test: atest --test-mapping packages/providers/MediaProvider
Change-Id: I0e685136d2b823e618bbc85cc79e656c9d4aad38

2 files changed, 86 insertions, 38 deletions

diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java
index f6fcdb0fcc76..56c2f4ce9425 100644
--- a/core/java/android/os/storage/StorageManager.java
+++ b/core/java/android/os/storage/StorageManager.java
@@ -135,6 +135,7 @@ import java.util.concurrent.atomic.AtomicInteger;
 @SystemService(Context.STORAGE_SERVICE)
 public class StorageManager {
     private static final String TAG = "StorageManager";
+    private static final boolean LOCAL_LOGV = Log.isLoggable(TAG, Log.VERBOSE);
 
     /** {@hide} */
     public static final String PROP_PRIMARY_PHYSICAL = "ro.vold.primary_physical";
@@ -1652,13 +1653,11 @@ public class StorageManager {
 
     /**
      * Check that given app holds both permission and appop.
-     *
-     * @return {@code null} if the permission and appop are held, otherwise
-     *         returns a string indicating why access was denied.
+     * @hide
      */
-    private boolean checkPermissionAndAppOp(boolean enforce, int pid, int uid, String packageName,
-            String permission, int op) {
-        if (mContext.checkPermission(permission, pid, uid) != PERMISSION_GRANTED) {
+    public static boolean checkPermissionAndAppOp(Context context, boolean enforce,
+            int pid, int uid, String packageName, String permission, int op) {
+        if (context.checkPermission(permission, pid, uid) != PERMISSION_GRANTED) {
             if (enforce) {
                 throw new SecurityException(
                         "Permission " + permission + " denied for package " + packageName);
@@ -1667,7 +1666,7 @@ public class StorageManager {
             }
         }
 
-        final AppOpsManager appOps = mContext.getSystemService(AppOpsManager.class);
+        final AppOpsManager appOps = context.getSystemService(AppOpsManager.class);
         final int mode = appOps.noteOpNoThrow(op, uid, packageName);
         switch (mode) {
             case AppOpsManager.MODE_ALLOWED:
@@ -1688,6 +1687,11 @@ public class StorageManager {
         }
     }
 
+    private boolean checkPermissionAndAppOp(boolean enforce,
+            int pid, int uid, String packageName, String permission, int op) {
+        return checkPermissionAndAppOp(mContext, enforce, pid, uid, packageName, permission, op);
+    }
+
     // Callers must hold both the old and new permissions, so that we can
     // handle obscure cases like when an app targets Q but was installed on
     // a device that was originally running on P before being upgraded to Q.
diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java
index 5b9c1f8d6f24..1a842f72fcb4 100644
--- a/services/core/java/com/android/server/StorageManagerService.java
+++ b/services/core/java/com/android/server/StorageManagerService.java
@@ -17,10 +17,14 @@
 package com.android.server;
 
 import static android.Manifest.permission.INSTALL_PACKAGES;
+import static android.Manifest.permission.READ_EXTERNAL_STORAGE;
+import static android.Manifest.permission.WRITE_EXTERNAL_STORAGE;
 import static android.Manifest.permission.WRITE_MEDIA_STORAGE;
 import static android.app.AppOpsManager.MODE_ALLOWED;
 import static android.app.AppOpsManager.OP_LEGACY_STORAGE;
+import static android.app.AppOpsManager.OP_READ_EXTERNAL_STORAGE;
 import static android.app.AppOpsManager.OP_REQUEST_INSTALL_PACKAGES;
+import static android.app.AppOpsManager.OP_WRITE_EXTERNAL_STORAGE;
 import static android.content.pm.PackageManager.FLAG_PERMISSION_HIDDEN;
 import static android.content.pm.PackageManager.FLAG_PERMISSION_SYSTEM_FIXED;
 import static android.content.pm.PackageManager.GET_PERMISSIONS;
@@ -280,6 +284,7 @@ class StorageManagerService extends IStorageManager.Stub
     private static final boolean EMULATE_FBE_SUPPORTED = true;
 
     private static final String TAG = "StorageManagerService";
+    private static final boolean LOCAL_LOGV = Log.isLoggable(TAG, Log.VERBOSE);
 
     private static final String TAG_STORAGE_BENCHMARK = "storage_benchmark";
     private static final String TAG_STORAGE_TRIM = "storage_trim";
@@ -3863,44 +3868,57 @@ class StorageManagerService extends IStorageManager.Stub
     }
 
     private int getMountMode(int uid, String packageName) {
+        final int mode = getMountModeInternal(uid, packageName);
+        if (LOCAL_LOGV) {
+            Slog.v(TAG, "Resolved mode " + mode + " for " + packageName + "/"
+                    + UserHandle.formatUid(uid));
+        }
+        return mode;
+    }
+
+    private int getMountModeInternal(int uid, String packageName) {
         try {
+            // Get some easy cases out of the way first
             if (Process.isIsolated(uid)) {
                 return Zygote.MOUNT_EXTERNAL_NONE;
             }
-            if (mIPackageManager.checkUidPermission(WRITE_MEDIA_STORAGE, uid)
-                    == PERMISSION_GRANTED) {
+            if (mPmInternal.isInstantApp(packageName, UserHandle.getUserId(uid))) {
+                return Zygote.MOUNT_EXTERNAL_NONE;
+            }
+
+            // Determine if caller is holding runtime permission
+            final boolean hasRead = StorageManager.checkPermissionAndAppOp(mContext, false, 0,
+                    uid, packageName, READ_EXTERNAL_STORAGE, OP_READ_EXTERNAL_STORAGE);
+            final boolean hasWrite = StorageManager.checkPermissionAndAppOp(mContext, false, 0,
+                    uid, packageName, WRITE_EXTERNAL_STORAGE, OP_WRITE_EXTERNAL_STORAGE);
+            final boolean hasStorage = hasRead || hasWrite;
+
+            // We're only willing to give out broad access if they also hold
+            // runtime permission; this is a firm CDD requirement
+            final boolean hasFull = mIPackageManager.checkUidPermission(WRITE_MEDIA_STORAGE,
+                    uid) == PERMISSION_GRANTED;
+            if (hasFull && hasStorage) {
                 return Zygote.MOUNT_EXTERNAL_FULL;
-            } else if (mIAppOpsService.checkOperation(OP_LEGACY_STORAGE, uid,
-                    packageName) == MODE_ALLOWED) {
-                return Zygote.MOUNT_EXTERNAL_LEGACY;
-            } else if (mIPackageManager.checkUidPermission(INSTALL_PACKAGES, uid)
-                    == PERMISSION_GRANTED || mIAppOpsService.checkOperation(
-                            OP_REQUEST_INSTALL_PACKAGES, uid, packageName) == MODE_ALLOWED) {
+            }
+
+            // We're only willing to give out installer access if they also hold
+            // runtime permission; this is a firm CDD requirement
+            final boolean hasInstall = mIPackageManager.checkUidPermission(INSTALL_PACKAGES,
+                    uid) == PERMISSION_GRANTED;
+            final boolean hasInstallOp = mIAppOpsService.checkOperation(OP_REQUEST_INSTALL_PACKAGES,
+                    uid, packageName) == MODE_ALLOWED;
+            if ((hasInstall || hasInstallOp) && hasStorage) {
                 return Zygote.MOUNT_EXTERNAL_INSTALLER;
-            } else if (mPmInternal.isInstantApp(packageName, UserHandle.getUserId(uid))) {
-                return Zygote.MOUNT_EXTERNAL_NONE;
+            }
+
+            // Otherwise we're willing to give out sandboxed or non-sandboxed if
+            // they hold the runtime permission
+            final boolean hasLegacy = mIAppOpsService.checkOperation(OP_LEGACY_STORAGE,
+                    uid, packageName) == MODE_ALLOWED;
+            final boolean hasGreylist = isLegacyGreylisted(packageName);
+            if ((hasLegacy || hasGreylist) && hasStorage) {
+                return Zygote.MOUNT_EXTERNAL_LEGACY;
             } else {
-                if (ENABLE_LEGACY_GREYLIST) {
-                    // STOPSHIP: remove this temporary workaround once developers
-                    // fix bugs where they're opening _data paths in native code
-                    switch (packageName) {
-                        case "com.facebook.katana": // b/123996076
-                        case "jp.naver.line.android": // b/124767356
-                        case "com.mxtech.videoplayer.ad": // b/124531483
-                        case "com.whatsapp": // b/124766614
-                        case "com.maxmpz.audioplayer": // b/127886230
-                        case "com.estrongs.android.pop": // b/127926473
-                        case "com.roidapp.photogrid": // b/128269119
-                        case "com.cleanmaster.mguard": // b/128384413
-                        case "com.skype.raider": // b/128487044
-                        case "org.telegram.messenger": // b/128652960
-                        case "com.jrtstudio.AnotherMusicPlayer": // b/129084562
-                        case "ak.alizandro.smartaudiobookplayer": // b/129084042
-                        case "com.campmobile.snow": // b/128803870
-                        case "com.qnap.qfile": // b/126374406
-                            return Zygote.MOUNT_EXTERNAL_LEGACY;
-                    }
-                }
                 return Zygote.MOUNT_EXTERNAL_WRITE;
             }
         } catch (RemoteException e) {
@@ -3909,6 +3927,32 @@ class StorageManagerService extends IStorageManager.Stub
         return Zygote.MOUNT_EXTERNAL_NONE;
     }
 
+    private boolean isLegacyGreylisted(String packageName) {
+        // TODO: decide legacy defaults at install time based on signals
+        if (ENABLE_LEGACY_GREYLIST) {
+            // STOPSHIP: remove this temporary workaround once developers
+            // fix bugs where they're opening _data paths in native code
+            switch (packageName) {
+                case "com.facebook.katana": // b/123996076
+                case "jp.naver.line.android": // b/124767356
+                case "com.mxtech.videoplayer.ad": // b/124531483
+                case "com.whatsapp": // b/124766614
+                case "com.maxmpz.audioplayer": // b/127886230
+                case "com.estrongs.android.pop": // b/127926473
+                case "com.roidapp.photogrid": // b/128269119
+                case "com.cleanmaster.mguard": // b/128384413
+                case "com.skype.raider": // b/128487044
+                case "org.telegram.messenger": // b/128652960
+                case "com.jrtstudio.AnotherMusicPlayer": // b/129084562
+                case "ak.alizandro.smartaudiobookplayer": // b/129084042
+                case "com.campmobile.snow": // b/128803870
+                case "com.qnap.qfile": // b/126374406
+                    return true;
+            }
+        }
+        return false;
+    }
+
     private static class Callbacks extends Handler {
         private static final int MSG_STORAGE_STATE_CHANGED = 1;
         private static final int MSG_VOLUME_STATE_CHANGED = 2;