From ecd34d4f3243195f4736fb98b62472dbebebd815 Mon Sep 17 00:00:00 2001 From: Saumya Pathak Date: Fri, 10 Dec 2021 15:09:00 +0000 Subject: [PATCH 1/2] Correct permission check for M_E_S and order of getApplicationInfo in the API Bug: 207389517 Bug: 210945734 Test: atest StorageManagerTest#testLaunchManageSpaceActivityIntent Change-Id: I23d884fc5a7c6383b19e2e267b8870d3102d5b7b --- .../android/server/StorageManagerService.java | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 98764e02d803e..67e461f526385 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3698,16 +3698,29 @@ class StorageManagerService extends IStorageManager.Stub @Nullable public PendingIntent getManageSpaceActivityIntent( @NonNull String packageName, int requestCode) { - // Only Apps with MANAGE_EXTERNAL_STORAGE permission should be able to call this API. - enforcePermission(android.Manifest.permission.MANAGE_EXTERNAL_STORAGE); - - // We want to call the manageSpaceActivity as a SystemService and clear identity - // of the calling App + // Only Apps with MANAGE_EXTERNAL_STORAGE permission which have package visibility for + // packageName should be able to call this API. int originalUid = Binder.getCallingUidOrThrow(); - final long token = Binder.clearCallingIdentity(); - try { - ApplicationInfo appInfo = mIPackageManager.getApplicationInfo(packageName, 0, + // Get package name for calling app and verify it has MANAGE_EXTERNAL_STORAGE permission + final String[] packagesFromUid = mIPackageManager.getPackagesForUid(originalUid); + if (packagesFromUid == null) { + throw new SecurityException("Unknown uid " + originalUid); + } + // 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(originalUid, + packagesFromUid[0])) { + throw new SecurityException("Only File Manager Apps permitted"); + } + } catch (RemoteException re) { + throw new SecurityException("Unknown uid " + originalUid, re); + } + + ApplicationInfo appInfo; + try { + appInfo = mIPackageManager.getApplicationInfo(packageName, 0, UserHandle.getUserId(originalUid)); if (appInfo == null) { throw new IllegalArgumentException( @@ -3717,8 +3730,15 @@ class StorageManagerService extends IStorageManager.Stub Log.i(TAG, packageName + " doesn't have a manageSpaceActivity"); return null; } - Context targetAppContext = mContext.createPackageContext(packageName, 0); + } catch (RemoteException e) { + throw new SecurityException("Only File Manager Apps permitted"); + } + // We want to call the manageSpaceActivity as a SystemService and clear identity + // of the calling App + final long token = Binder.clearCallingIdentity(); + try { + Context targetAppContext = mContext.createPackageContext(packageName, 0); Intent intent = new Intent(Intent.ACTION_DEFAULT); intent.setClassName(packageName, appInfo.manageSpaceActivityName); @@ -3728,8 +3748,6 @@ class StorageManagerService extends IStorageManager.Stub intent, FLAG_ONE_SHOT | FLAG_CANCEL_CURRENT | FLAG_IMMUTABLE); return activity; - } catch (RemoteException e) { - throw e.rethrowAsRuntimeException(); } catch (PackageManager.NameNotFoundException e) { throw new IllegalArgumentException( "packageName not found"); From 6bbf094023545e2133b995b1ef54b923c33b0431 Mon Sep 17 00:00:00 2001 From: Saumya Pathak Date: Wed, 12 Jan 2022 19:11:53 +0000 Subject: [PATCH 2/2] Check AppOp mode first in hasExternalStorageAccess MANAGE_EXTERNAL_STORAGE is an AppOp permission. Order of check should be AppOp first, and if its MODE_DEFAULT then check permission with PackageManager. Bug: 213876995 Test: manual Change-Id: I0464db33fc3ec24eb103d1abcaf5c722a5757604 --- .../android/server/StorageManagerService.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 67e461f526385..f71f02a6ec4ee 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -160,8 +160,6 @@ import com.android.server.storage.StorageSessionController.ExternalStorageServic import com.android.server.wm.ActivityTaskManagerInternal; import com.android.server.wm.ActivityTaskManagerInternal.ScreenObserver; -import java.io.ObjectInputStream; -import java.io.ObjectOutputStream; import libcore.io.IoUtils; import libcore.util.EmptyArray; @@ -173,6 +171,8 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.io.PrintWriter; import java.math.BigInteger; import java.security.GeneralSecurityException; @@ -4973,19 +4973,17 @@ class StorageManagerService extends IStorageManager.Stub @Override public boolean hasExternalStorageAccess(int uid, String packageName) { try { - if (mIPackageManager.checkUidPermission( - MANAGE_EXTERNAL_STORAGE, uid) == PERMISSION_GRANTED) { - return true; + final int opMode = mIAppOpsService.checkOperation( + OP_MANAGE_EXTERNAL_STORAGE, uid, packageName); + if (opMode == AppOpsManager.MODE_DEFAULT) { + return mIPackageManager.checkUidPermission( + MANAGE_EXTERNAL_STORAGE, uid) == PERMISSION_GRANTED; } - if (mIAppOpsService.checkOperation( - OP_MANAGE_EXTERNAL_STORAGE, uid, packageName) == MODE_ALLOWED) { - return true; - } + return opMode == AppOpsManager.MODE_ALLOWED; } catch (RemoteException e) { Slog.w("Failed to check MANAGE_EXTERNAL_STORAGE access for " + packageName, e); } - return false; }