From e57620446bfc0c2bdca5e07a0c39f647e07ca18d Mon Sep 17 00:00:00 2001
From: Martijn Coenen <maco@google.com>
Date: Wed, 10 Nov 2021 16:21:25 +0100
Subject: [PATCH] Have getVolumeList() take a userId instead of a UID.

getVolumeList() previously accepted a uid and a packagename. This
approach has a few issues:
1) The uid wasn't actually checked in the implementation to match the
calling UID, nor was the package name checked against the UID in all
cases.
2) The implementation assumed that the UID also specificies the userId
that we want to get the volumes for. This was historically true, but
with the new app cloning implementation, MediaProvider running in user 0
might ask for volumes in the clone user.

To fix this, explicitly pass in the userId that we're requesting volumes
for, and verify that the calling UID and the passed in package name
match.

Bug: 206624903
Test: atest packages/providers/MediaProvider
Change-Id: Idffbb4ae7740c1b9725af0974948dc55bf1f1fde
---
 .../android/os/storage/IStorageManager.aidl   |  4 ++--
 .../android/os/storage/StorageManager.java    |  8 +-------
 .../android/server/StorageManagerService.java | 20 +++++++++++++++----
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/core/java/android/os/storage/IStorageManager.aidl b/core/java/android/os/storage/IStorageManager.aidl
index fbac954b0a35c..bff5c62ed3f12 100644
--- a/core/java/android/os/storage/IStorageManager.aidl
+++ b/core/java/android/os/storage/IStorageManager.aidl
@@ -90,9 +90,9 @@ interface IStorageManager {
      */
     int changeEncryptionPassword(int type, in String password) = 28;
     /**
-     * Returns list of all mountable volumes.
+     * Returns list of all mountable volumes for the specified userId
      */
-    StorageVolume[] getVolumeList(int uid, in String packageName, int flags) = 29;
+    StorageVolume[] getVolumeList(int userId, in String callingPackage, int flags) = 29;
     /**
      * Determines the encryption state of the volume.
      * @return a numerical value. See {@code ENCRYPTION_STATE_*} for possible
diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java
index 77c794cd17a89..627e09eadd26c 100644
--- a/core/java/android/os/storage/StorageManager.java
+++ b/core/java/android/os/storage/StorageManager.java
@@ -1391,13 +1391,7 @@ public class StorageManager {
                 }
                 packageName = packageNames[0];
             }
-            final int uid = ActivityThread.getPackageManager().getPackageUid(packageName,
-                    PackageManager.MATCH_DEBUG_TRIAGED_MISSING, userId);
-            if (uid <= 0) {
-                Log.w(TAG, "Missing UID; no storage volumes available");
-                return new StorageVolume[0];
-            }
-            return storageManager.getVolumeList(uid, packageName, flags);
+            return storageManager.getVolumeList(userId, packageName, flags);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java
index 311555e326ddd..6f009b6c2f40d 100644
--- a/services/core/java/com/android/server/StorageManagerService.java
+++ b/services/core/java/com/android/server/StorageManagerService.java
@@ -3713,8 +3713,19 @@ class StorageManagerService extends IStorageManager.Stub
     }
 
     @Override
-    public StorageVolume[] getVolumeList(int uid, String packageName, int flags) {
-        final int userId = UserHandle.getUserId(uid);
+    public StorageVolume[] getVolumeList(int userId, String callingPackage, int flags) {
+        final int callingUid = Binder.getCallingUid();
+        final int callingUserId = UserHandle.getUserId(callingUid);
+
+        if (!isUidOwnerOfPackageOrSystem(callingPackage, callingUid)) {
+            throw new SecurityException("callingPackage does not match UID");
+        }
+        if (callingUserId != userId) {
+            // Callers can ask for volumes of different users, but only with the correct permissions
+            mContext.enforceCallingOrSelfPermission(
+                    android.Manifest.permission.INTERACT_ACROSS_USERS,
+                    "Need INTERACT_ACROSS_USERS to get volumes for another user");
+        }
 
         final boolean forWrite = (flags & StorageManager.FLAG_FOR_WRITE) != 0;
         final boolean realState = (flags & StorageManager.FLAG_REAL_STATE) != 0;
@@ -3730,7 +3741,7 @@ class StorageManagerService extends IStorageManager.Stub
         // should never attempt to augment the actual storage volume state,
         // otherwise we risk confusing it with race conditions as users go
         // through various unlocked states
-        final boolean callerIsMediaStore = UserHandle.isSameApp(Binder.getCallingUid(),
+        final boolean callerIsMediaStore = UserHandle.isSameApp(callingUid,
                 mMediaStoreAuthorityAppId);
 
         final boolean userIsDemo;
@@ -3740,8 +3751,9 @@ class StorageManagerService extends IStorageManager.Stub
         try {
             userIsDemo = LocalServices.getService(UserManagerInternal.class)
                     .getUserInfo(userId).isDemo();
+            storagePermission = mStorageManagerInternal.hasExternalStorage(callingUid,
+                    callingPackage);
             userKeyUnlocked = isUserKeyUnlocked(userId);
-            storagePermission = mStorageManagerInternal.hasExternalStorage(uid, packageName);
         } finally {
             Binder.restoreCallingIdentity(token);
         }