From f9c27b354abf14043f8638dfa14a3b0bf97937fa Mon Sep 17 00:00:00 2001 From: Himanshu Gupta Date: Fri, 9 Sep 2022 14:02:15 +0100 Subject: [PATCH] Fixing Storage Volume listing for Cloned User. Current implementation only returns the Volumes for the current user. In case of Cloned user this causes issues as Cloned user uses Media Provider of User 0, thereby returning only Volume 0, causing problems in assigning projectIds, and affecting storage calculation. The projectId assignement happens via MediaProvider.updateExternalStorageFileQuota. Bug: 235321217 Test: atest android.appsecurity.cts.StorageHostTest Change-Id: I617bae9b201fd66d7b62b6cbd1c48b9f276f1924 Fixing Storage Volume(s) Retrieval. With ag/19901205 shared_profile's volumes were also listed in StorageManager#updateExternalStorageFileQuotaType. However, the above API can be called from MediaProvider process, without MANAGE_EXTERNAL_STORAGE permissions, resulting in SecurityException("Only File Manager Apps permitted") to be thrown from StorageManagerService#getVolumeList This fix allows the exception to be bypassed in case the caller is Media Store process. Bug: 235321217 Test: atest android.appsecurity.cts.StorageHostTest Change-Id: I6835cc4d29f3e9c85731979aaf9ab12a30f6419b Downgrading API permissions for updateExternalStorage With ag/19901205 the permissions for this API were increased, and calling users required 'MANAGE_EXTERNAL_STORAGE' permissions. There is now a way to add the same functionality without adding any additional permissions. Bug: 235321217 Test: atest android.appsecurity.cts.StorageHostTest Change-Id: I6b166ac191a80c600b527266f53107b8d9c78177 Merged-In: I6b166ac191a80c600b527266f53107b8d9c78177 Change-Id: I475d0782cab9fa1033e16d2b4dc297b00296783b --- .../android/os/storage/StorageManager.java | 11 ++++++++- .../android/server/StorageManagerService.java | 23 +++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/core/java/android/os/storage/StorageManager.java b/core/java/android/os/storage/StorageManager.java index 497bfa6380bc1..d9604b3f0145e 100644 --- a/core/java/android/os/storage/StorageManager.java +++ b/core/java/android/os/storage/StorageManager.java @@ -2555,6 +2555,8 @@ public class StorageManager { * This API doesn't require any special permissions, though typical implementations * will require being called from an SELinux domain that allows setting file attributes * related to quota (eg the GID or project ID). + * If the calling user has MANAGE_EXTERNAL_STORAGE permissions, quota for shared profile's + * volumes is also updated. * * The default platform user of this API is the MediaProvider process, which is * responsible for managing all of external storage. @@ -2575,7 +2577,14 @@ public class StorageManager { @QuotaType int quotaType) throws IOException { long projectId; final String filePath = path.getCanonicalPath(); - final StorageVolume volume = getStorageVolume(path); + int volFlags = FLAG_REAL_STATE | FLAG_INCLUDE_INVISIBLE; + // If caller has MANAGE_EXTERNAL_STORAGE permission, results from User Profile(s) are also + // returned by enabling FLAG_INCLUDE_SHARED_PROFILE. + if (mContext.checkSelfPermission(MANAGE_EXTERNAL_STORAGE) == PERMISSION_GRANTED) { + volFlags |= FLAG_INCLUDE_SHARED_PROFILE; + } + final StorageVolume[] availableVolumes = getVolumeList(mContext.getUserId(), volFlags); + final StorageVolume volume = getStorageVolume(availableVolumes, path); if (volume == null) { Log.w(TAG, "Failed to update quota type for " + filePath); return; diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index c4333d96f92ea..8a429331f766a 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3813,6 +3813,13 @@ class StorageManagerService extends IStorageManager.Stub final boolean includeSharedProfile = (flags & StorageManager.FLAG_INCLUDE_SHARED_PROFILE) != 0; + // When the caller is the app actually hosting external storage, we + // 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(callingUid, + mMediaStoreAuthorityAppId); + // Only Apps with MANAGE_EXTERNAL_STORAGE should call the API with includeSharedProfile if (includeSharedProfile) { try { @@ -3825,8 +3832,13 @@ class StorageManagerService extends IStorageManager.Stub // Checking first entry in packagesFromUid is enough as using "sharedUserId" // mechanism is rare and discouraged. Also, Apps that share same UID share the same // permissions. - if (!mStorageManagerInternal.hasExternalStorageAccess(callingUid, - packagesFromUid[0])) { + // Allowing Media Provider is an exception, Media Provider process should be allowed + // to query users across profiles, even without MANAGE_EXTERNAL_STORAGE access. + // Note that ordinarily Media provider process has the above permission, but if they + // are revoked, Storage Volume(s) should still be returned. + if (!callerIsMediaStore + && !mStorageManagerInternal.hasExternalStorageAccess(callingUid, + packagesFromUid[0])) { throw new SecurityException("Only File Manager Apps permitted"); } } catch (RemoteException re) { @@ -3839,13 +3851,6 @@ class StorageManagerService extends IStorageManager.Stub // point final boolean systemUserUnlocked = isSystemUnlocked(UserHandle.USER_SYSTEM); - // When the caller is the app actually hosting external storage, we - // 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(callingUid, - mMediaStoreAuthorityAppId); - final boolean userIsDemo; final boolean userKeyUnlocked; final boolean storagePermission;