From 8f7e68c0f4b00f88aa0b8305aaa3ddc9ddfc1c29 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Thu, 26 May 2022 15:00:46 +0800 Subject: [PATCH] Fix cross user package visibility leakage for getAppOpPermissionPackages To fix cross user package visibility leakage, this CL filters out packages that aren't installed in the calling user before the API returns results to the caller. Also adding a user id parameter to the API for the system modules to specify the correct user id when querying the appop permission packages. NoNonSdkCheck: Keep @UnsupportedAppUsage for new signature api Bug: 229684723 Test: atest CrossUserPackageVisibilityTests Change-Id: I9d3de91b0195d3396d2737673cb23ef899e23467 --- boot/hiddenapi/hiddenapi-unsupported.txt | 1 - .../android/content/pm/IPackageManager.aidl | 2 +- .../java/com/android/server/pm/Computer.java | 2 +- .../com/android/server/pm/ComputerEngine.java | 15 +++++++------ .../pm/CrossProfileAppsServiceImpl.java | 14 +++++++------ .../server/pm/IPackageManagerBase.java | 4 ++-- .../CrossProfileAppsServiceImplRoboTest.java | 4 ++-- .../AppEnumerationInternalTests.java | 5 +++-- .../CrossUserPackageVisibilityTests.java | 21 +++++++++++++++++++ 9 files changed, 45 insertions(+), 23 deletions(-) diff --git a/boot/hiddenapi/hiddenapi-unsupported.txt b/boot/hiddenapi/hiddenapi-unsupported.txt index f6199137012fa..6b71e24a1d357 100644 --- a/boot/hiddenapi/hiddenapi-unsupported.txt +++ b/boot/hiddenapi/hiddenapi-unsupported.txt @@ -110,7 +110,6 @@ Landroid/content/pm/IPackageInstallObserver2$Stub;->()V Landroid/content/pm/IPackageInstallObserver2$Stub;->asInterface(Landroid/os/IBinder;)Landroid/content/pm/IPackageInstallObserver2; Landroid/content/pm/IPackageManager$Stub$Proxy;->(Landroid/os/IBinder;)V Landroid/content/pm/IPackageManager$Stub$Proxy;->checkUidPermission(Ljava/lang/String;I)I -Landroid/content/pm/IPackageManager$Stub$Proxy;->getAppOpPermissionPackages(Ljava/lang/String;)[Ljava/lang/String; Landroid/content/pm/IPackageManager$Stub$Proxy;->getInstallLocation()I Landroid/content/pm/IPackageManager$Stub$Proxy;->getLastChosenActivity(Landroid/content/Intent;Ljava/lang/String;I)Landroid/content/pm/ResolveInfo; Landroid/content/pm/IPackageManager$Stub$Proxy;->getPackagesForUid(I)[Ljava/lang/String; diff --git a/core/java/android/content/pm/IPackageManager.aidl b/core/java/android/content/pm/IPackageManager.aidl index 1b1143a992c98..2ef1c78474f23 100644 --- a/core/java/android/content/pm/IPackageManager.aidl +++ b/core/java/android/content/pm/IPackageManager.aidl @@ -746,7 +746,7 @@ interface IPackageManager { // We need to keep these in IPackageManager for app compatibility //------------------------------------------------------------------------ @UnsupportedAppUsage(maxTargetSdk = 30, trackingBug = 170729553) - String[] getAppOpPermissionPackages(String permissionName); + String[] getAppOpPermissionPackages(String permissionName, int userId); @UnsupportedAppUsage PermissionGroupInfo getPermissionGroupInfo(String name, int flags); diff --git a/services/core/java/com/android/server/pm/Computer.java b/services/core/java/com/android/server/pm/Computer.java index 33e9b2f27eb2a..5a2f6feb4d831 100644 --- a/services/core/java/com/android/server/pm/Computer.java +++ b/services/core/java/com/android/server/pm/Computer.java @@ -425,7 +425,7 @@ public interface Computer extends PackageDataSnapshot { boolean isUidPrivileged(int uid); @NonNull - String[] getAppOpPermissionPackages(@NonNull String permissionName); + String[] getAppOpPermissionPackages(@NonNull String permissionName, int userId); @NonNull ParceledListSlice getPackagesHoldingPermissions(@NonNull String[] permissions, diff --git a/services/core/java/com/android/server/pm/ComputerEngine.java b/services/core/java/com/android/server/pm/ComputerEngine.java index 7a01f1ed5ba82..f4c750fbcff96 100644 --- a/services/core/java/com/android/server/pm/ComputerEngine.java +++ b/services/core/java/com/android/server/pm/ComputerEngine.java @@ -4699,22 +4699,21 @@ public class ComputerEngine implements Computer { // NOTE: Can't remove due to unsupported app usage @NonNull @Override - public String[] getAppOpPermissionPackages(@NonNull String permissionName) { - if (permissionName == null) { - return EmptyArray.STRING; - } + public String[] getAppOpPermissionPackages(@NonNull String permissionName, int userId) { final int callingUid = Binder.getCallingUid(); - if (getInstantAppPackageName(callingUid) != null) { + enforceCrossUserPermission(callingUid, userId, false /* requireFullPermission */, + false /* checkShell */, "getAppOpPermissionPackages"); + if (permissionName == null || getInstantAppPackageName(callingUid) != null + || !mUserManager.exists(userId)) { return EmptyArray.STRING; } - final int callingUserId = UserHandle.getUserId(callingUid); final ArraySet packageNames = new ArraySet( mPermissionManager.getAppOpPermissionPackages(permissionName)); for (int i = packageNames.size() - 1; i >= 0; i--) { final String packageName = packageNames.valueAt(i); - if (!shouldFilterApplication(mSettings.getPackage(packageName), callingUid, - callingUserId)) { + if (!shouldFilterApplicationIncludingUninstalled( + mSettings.getPackage(packageName), callingUid, userId)) { continue; } packageNames.removeAt(i); diff --git a/services/core/java/com/android/server/pm/CrossProfileAppsServiceImpl.java b/services/core/java/com/android/server/pm/CrossProfileAppsServiceImpl.java index daac7c04098ae..ff6420c45c392 100644 --- a/services/core/java/com/android/server/pm/CrossProfileAppsServiceImpl.java +++ b/services/core/java/com/android/server/pm/CrossProfileAppsServiceImpl.java @@ -265,8 +265,9 @@ public class CrossProfileAppsServiceImpl extends ICrossProfileApps.Stub { } private boolean canRequestInteractAcrossProfilesUnchecked(String packageName) { + final int callingUserId = mInjector.getCallingUserId(); final int[] enabledProfileIds = - mInjector.getUserManager().getEnabledProfileIds(mInjector.getCallingUserId()); + mInjector.getUserManager().getEnabledProfileIds(callingUserId); if (enabledProfileIds.length < 2) { return false; } @@ -274,13 +275,14 @@ public class CrossProfileAppsServiceImpl extends ICrossProfileApps.Stub { return false; } return hasRequestedAppOpPermission( - AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName); + AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName, + callingUserId); } - private boolean hasRequestedAppOpPermission(String permission, String packageName) { + private boolean hasRequestedAppOpPermission(String permission, String packageName, int userId) { try { String[] packages = - mInjector.getIPackageManager().getAppOpPermissionPackages(permission); + mInjector.getIPackageManager().getAppOpPermissionPackages(permission, userId); return ArrayUtils.contains(packages, packageName); } catch (RemoteException exc) { Slog.e(TAG, "PackageManager dead. Cannot get permission info"); @@ -604,7 +606,7 @@ public class CrossProfileAppsServiceImpl extends ICrossProfileApps.Stub { return false; } if (!hasRequestedAppOpPermission( - AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName)) { + AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName, userId)) { return false; } return isCrossProfilePackageAllowlisted(packageName); @@ -627,7 +629,7 @@ public class CrossProfileAppsServiceImpl extends ICrossProfileApps.Stub { return false; } if (!hasRequestedAppOpPermission( - AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName)) { + AppOpsManager.opToPermission(OP_INTERACT_ACROSS_PROFILES), packageName, userId)) { return false; } return !isPlatformSignedAppWithNonUserConfigurablePermission(packageName, profileIds); diff --git a/services/core/java/com/android/server/pm/IPackageManagerBase.java b/services/core/java/com/android/server/pm/IPackageManagerBase.java index c16071dbbb11a..f4285eb0d9d90 100644 --- a/services/core/java/com/android/server/pm/IPackageManagerBase.java +++ b/services/core/java/com/android/server/pm/IPackageManagerBase.java @@ -324,8 +324,8 @@ public abstract class IPackageManagerBase extends IPackageManager.Stub { @NonNull @Override @Deprecated - public final String[] getAppOpPermissionPackages(@NonNull String permissionName) { - return snapshot().getAppOpPermissionPackages(permissionName); + public final String[] getAppOpPermissionPackages(@NonNull String permissionName, int userId) { + return snapshot().getAppOpPermissionPackages(permissionName, userId); } @Override diff --git a/services/robotests/src/com/android/server/pm/CrossProfileAppsServiceImplRoboTest.java b/services/robotests/src/com/android/server/pm/CrossProfileAppsServiceImplRoboTest.java index 5dc048b9d7c6f..df1d244ab879f 100644 --- a/services/robotests/src/com/android/server/pm/CrossProfileAppsServiceImplRoboTest.java +++ b/services/robotests/src/com/android/server/pm/CrossProfileAppsServiceImplRoboTest.java @@ -171,7 +171,7 @@ public class CrossProfileAppsServiceImplRoboTest { private void mockCrossProfileAppRequestsInteractAcrossProfiles() throws Exception { final String permissionName = Manifest.permission.INTERACT_ACROSS_PROFILES; - when(mIPackageManager.getAppOpPermissionPackages(permissionName)) + when(mIPackageManager.getAppOpPermissionPackages(eq(permissionName), anyInt())) .thenReturn(new String[] {CROSS_PROFILE_APP_PACKAGE_NAME}); } @@ -488,7 +488,7 @@ public class CrossProfileAppsServiceImplRoboTest { private void mockCrossProfileAppDoesNotRequestInteractAcrossProfiles() throws Exception { final String permissionName = Manifest.permission.INTERACT_ACROSS_PROFILES; - when(mIPackageManager.getAppOpPermissionPackages(permissionName)) + when(mIPackageManager.getAppOpPermissionPackages(eq(permissionName), anyInt())) .thenReturn(new String[] {}); } diff --git a/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/AppEnumerationInternalTests.java b/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/AppEnumerationInternalTests.java index ab004bea553e4..08cea06fd3143 100644 --- a/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/AppEnumerationInternalTests.java +++ b/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/AppEnumerationInternalTests.java @@ -24,6 +24,7 @@ import android.app.AppGlobals; import android.content.pm.IPackageManager; import android.content.pm.ProviderInfo; import android.os.Process; +import android.os.UserHandle; import androidx.test.runner.AndroidJUnit4; @@ -102,7 +103,7 @@ public class AppEnumerationInternalTests { installPackage(HAS_APPOP_PERMISSION_APK_PATH, true /* forceQueryable */); final String[] packageNames = mIPackageManager.getAppOpPermissionPackages( - PERMISSION_REQUEST_INSTALL_PACKAGES); + PERMISSION_REQUEST_INSTALL_PACKAGES, UserHandle.myUserId()); assertThat(packageNames).asList().contains(TARGET_HAS_APPOP_PERMISSION); } @@ -112,7 +113,7 @@ public class AppEnumerationInternalTests { installPackage(HAS_APPOP_PERMISSION_APK_PATH, false /* forceQueryable */); final String[] packageNames = mIPackageManager.getAppOpPermissionPackages( - PERMISSION_REQUEST_INSTALL_PACKAGES); + PERMISSION_REQUEST_INSTALL_PACKAGES, UserHandle.myUserId()); assertThat(packageNames).asList().doesNotContain(TARGET_HAS_APPOP_PERMISSION); } diff --git a/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/CrossUserPackageVisibilityTests.java b/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/CrossUserPackageVisibilityTests.java index 0f742a39c6796..e33ca7775e220 100644 --- a/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/CrossUserPackageVisibilityTests.java +++ b/services/tests/PackageManagerServiceTests/appenumeration/src/com/android/server/pm/test/appenumeration/CrossUserPackageVisibilityTests.java @@ -73,13 +73,20 @@ public class CrossUserPackageVisibilityTests { "com.android.appenumeration.crossuserpackagevisibility"; private static final String SHARED_USER_TEST_PACKAGE_NAME = "com.android.appenumeration.shareduid"; + private static final String HAS_APPOP_PERMISSION_PACKAGE_NAME = + "com.android.appenumeration.hasappoppermission"; + private static final File CROSS_USER_TEST_APK_FILE = new File(TEST_DATA_DIR, "AppEnumerationCrossUserPackageVisibilityTestApp.apk"); private static final File SHARED_USER_TEST_APK_FILE = new File(TEST_DATA_DIR, "AppEnumerationSharedUserTestApp.apk"); + private static final File HAS_APPOP_PERMISSION_APK_FILE = + new File(TEST_DATA_DIR, "AppEnumerationHasAppOpPermissionTestApp.apk"); private static final String ACTION_CROSS_USER_TEST = "com.android.appenumeration.action.CROSS_USER_TEST"; + private static final String PERMISSION_REQUEST_INSTALL_PACKAGES = + "android.permission.REQUEST_INSTALL_PACKAGES"; private static final ComponentName TEST_ACTIVITY_COMPONENT_NAME = new ComponentName( CROSS_USER_TEST_PACKAGE_NAME, "com.android.appenumeration.testapp.DummyActivity"); @@ -264,6 +271,20 @@ public class CrossUserPackageVisibilityTests { mCurrentUser.id())).isFalse(); } + @Test + public void testGetAppOpPermissionPackages_cannotDetectPkg() throws Exception { + final int userId = mCurrentUser.id(); + assertThat(mIPackageManager + .getAppOpPermissionPackages(PERMISSION_REQUEST_INSTALL_PACKAGES, userId)) + .asList().doesNotContain(HAS_APPOP_PERMISSION_PACKAGE_NAME); + + installPackageForUser(HAS_APPOP_PERMISSION_APK_FILE, mOtherUser, true /* forceQueryable */); + + assertThat(mIPackageManager + .getAppOpPermissionPackages(PERMISSION_REQUEST_INSTALL_PACKAGES, userId)) + .asList().doesNotContain(HAS_APPOP_PERMISSION_PACKAGE_NAME); + } + private boolean clearApplicationUserData(String packageName) throws Exception { final AtomicInteger result = new AtomicInteger(-1); final IPackageDataObserver localObserver = new IPackageDataObserver.Stub() {