From f4d8bd16b7788abd26313ec2be3a630b43c233c9 Mon Sep 17 00:00:00 2001 From: Michael Groover Date: Mon, 27 Sep 2021 19:09:49 -0700 Subject: [PATCH] Ensure pkg uid matches provided uid for device phone number check An app on the device is able to directly interact with any of the services that accepts a package name and can return a protected device resource (phone number or identifier). The app is then able to pass the name of another package targeting pre-R and determine whether the app is installed on the device based on whether the service method throws an Exception or not. While the app is able to pass another package's name to the service method, the service method will still use Binder#getCallingUid for the check. To prevent leaking information about packages installed on the device, this commit adds an additional check to verify the provided uid matches that of the package; if not, a SecurityException is thrown that only contains the provided package name, along with the uid / pid of the calling app. Bug: 193441322 Bug: 193445182 Test: atest LegacyPermissionManagerServiceTest Change-Id: If9353b7cb697bd78ab18775aee7723e984d3c1db --- .../LegacyPermissionManagerService.java | 25 +++++ .../LegacyPermissionManagerServiceTest.java | 103 ++++++++++++++++-- 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/LegacyPermissionManagerService.java b/services/core/java/com/android/server/pm/permission/LegacyPermissionManagerService.java index b1676d0e545ff..ea554d3d79964 100644 --- a/services/core/java/com/android/server/pm/permission/LegacyPermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/LegacyPermissionManagerService.java @@ -30,6 +30,7 @@ import android.os.Process; import android.os.ServiceManager; import android.os.UserHandle; import android.permission.ILegacyPermissionManager; +import android.util.EventLog; import android.util.Log; import com.android.internal.annotations.VisibleForTesting; @@ -187,10 +188,25 @@ public class LegacyPermissionManagerService extends ILegacyPermissionManager.Stu private void verifyCallerCanCheckAccess(String packageName, String message, int pid, int uid) { // If the check is being requested by an app then only allow the app to query its own // access status. + boolean reportError = false; int callingUid = mInjector.getCallingUid(); int callingPid = mInjector.getCallingPid(); if (UserHandle.getAppId(callingUid) >= Process.FIRST_APPLICATION_UID && (callingUid != uid || callingPid != pid)) { + reportError = true; + } + // If the query is against an app on the device, then the check should only be allowed if + // the provided uid matches that of the specified package. + if (packageName != null && UserHandle.getAppId(uid) >= Process.FIRST_APPLICATION_UID) { + int packageUid = mInjector.getPackageUidForUser(packageName, UserHandle.getUserId(uid)); + if (uid != packageUid) { + EventLog.writeEvent(0x534e4554, "193441322", + UserHandle.getAppId(callingUid) >= Process.FIRST_APPLICATION_UID + ? callingUid : uid, "Package uid mismatch"); + reportError = true; + } + } + if (reportError) { String response = String.format( "Calling uid %d, pid %d cannot access for package %s (uid=%d, pid=%d): %s", callingUid, callingPid, packageName, uid, pid, message); @@ -385,12 +401,14 @@ public class LegacyPermissionManagerService extends ILegacyPermissionManager.Stu @VisibleForTesting public static class Injector { private final Context mContext; + private final PackageManagerInternal mPackageManagerInternal; /** * Public constructor that accepts a {@code context} within which to operate. */ public Injector(@NonNull Context context) { mContext = context; + mPackageManagerInternal = LocalServices.getService(PackageManagerInternal.class); } /** @@ -453,5 +471,12 @@ public class LegacyPermissionManagerService extends ILegacyPermissionManager.Stu return mContext.getPackageManager().getApplicationInfoAsUser(packageName, 0, UserHandle.getUserHandleForUid(uid)); } + + /** + * Returns the uid for the specified {@code packageName} under the provided {@code userId}. + */ + public int getPackageUidForUser(String packageName, int userId) { + return mPackageManagerInternal.getPackageUid(packageName, 0, userId); + } } } diff --git a/services/tests/servicestests/src/com/android/server/pm/permission/LegacyPermissionManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/pm/permission/LegacyPermissionManagerServiceTest.java index acd3fcab5e52a..3261dfaa95c93 100644 --- a/services/tests/servicestests/src/com/android/server/pm/permission/LegacyPermissionManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/permission/LegacyPermissionManagerServiceTest.java @@ -125,7 +125,7 @@ public class LegacyPermissionManagerServiceTest { public void checkDeviceIdentifierAccess_hasPrivilegedPermission_returnsGranted() { // Apps with the READ_PRIVILEGED_PHONE_STATE permission should have access to device // identifiers. - setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID); + setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID); when(mInjector.checkPermission(android.Manifest.permission.READ_PRIVILEGED_PHONE_STATE, APP_PID, APP_UID)).thenReturn(PackageManager.PERMISSION_GRANTED); @@ -140,7 +140,7 @@ public class LegacyPermissionManagerServiceTest { public void checkDeviceIdentifierAccess_hasAppOp_returnsGranted() { // Apps that have been granted the READ_DEVICE_IDENTIFIERS appop should have access to // device identifiers. - setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID); + setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID); when(mAppOpsManager.noteOpNoThrow(eq(AppOpsManager.OPSTR_READ_DEVICE_IDENTIFIERS), eq(APP_UID), eq(mPackageName), any(), any())).thenReturn( AppOpsManager.MODE_ALLOWED); @@ -156,7 +156,7 @@ public class LegacyPermissionManagerServiceTest { public void checkDeviceIdentifierAccess_hasDpmAccess_returnsGranted() { // Apps that pass a DevicePolicyManager device / profile owner check should have access to // device identifiers. - setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID); + setupCheckDeviceIdentifierAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID); when(mDevicePolicyManager.hasDeviceIdentifierAccess(mPackageName, APP_PID, APP_UID)).thenReturn(true); @@ -236,7 +236,7 @@ public class LegacyPermissionManagerServiceTest { // both the permission and the appop must be granted. If the permission is granted but the // appop is not then AppOpsManager#MODE_IGNORED should be returned to indicate that this // should be a silent failure. - setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID); + setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID); setPackageTargetSdk(Build.VERSION_CODES.Q); grantPermissionAndAppop(android.Manifest.permission.READ_PHONE_STATE, null); @@ -256,7 +256,7 @@ public class LegacyPermissionManagerServiceTest { // Apps targeting R+ with just the READ_PHONE_STATE permission granted should not have // access to the phone number; PERMISSION_DENIED should be returned both with and without // the appop granted since this check should be skipped for target SDK R+. - setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID); + setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID); grantPermissionAndAppop(android.Manifest.permission.READ_PHONE_STATE, null); int resultWithoutAppop = mLegacyPermissionManagerService.checkPhoneNumberAccess( @@ -319,12 +319,79 @@ public class LegacyPermissionManagerServiceTest { assertEquals(PackageManager.PERMISSION_GRANTED, resultWithAppop); } + @Test + public void checkPhoneNumberAccess_providedUidDoesNotMatchPackageUid_throwsException() + throws Exception { + // An app can directly interact with one of the services that accepts a package name and + // returns a protected resource via a direct binder transact. This app could then provide + // the name of another app that targets pre-R, then determine if the app is installed based + // on whether the service throws an exception or not. While the app can provide the package + // name of another app, it cannot specify the package uid which is passed to the + // LegacyPermissionManager using Binder#getCallingUid. Ultimately this uid should then be + // compared against the actual uid of the package to ensure information about packages + // installed on the device is not leaked. + setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID, APP_UID + 1); + + assertThrows(SecurityException.class, + () -> mLegacyPermissionManagerService.checkPhoneNumberAccess(mPackageName, + CHECK_PHONE_NUMBER_MESSAGE, null, APP_PID, APP_UID)); + } + + @Test + public void checkPhoneNumberAccess_nullPackageNameSystemUid_returnsGranted() throws Exception { + // The platform can pass a null package name when checking if the platform itself has + // access to the device phone number(s) / identifier(s). This test ensures if a null package + // is provided, then the package uid check is skipped and the test is based on whether the + // the provided uid / pid has been granted the privileged permission. + setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID, -1); + when(mInjector.checkPermission(android.Manifest.permission.READ_PRIVILEGED_PHONE_STATE, + SYSTEM_PID, SYSTEM_UID)).thenReturn(PackageManager.PERMISSION_GRANTED); + + int result = mLegacyPermissionManagerService.checkPhoneNumberAccess(null, + CHECK_PHONE_NUMBER_MESSAGE, null, SYSTEM_PID, SYSTEM_UID); + + assertEquals(PackageManager.PERMISSION_GRANTED, result); + } + + @Test + public void checkPhoneNumberAccess_systemUidMismatchPackageUid_returnsGranted() + throws Exception { + // When the platform is checking device phone number / identifier access checks for other + // components on the platform, a uid less than the first application UID is provided; this + // test verifies the package uid check is skipped and access is still granted with the + // privileged permission. + int telephonyUid = SYSTEM_UID + 1; + int telephonyPid = SYSTEM_PID + 1; + setupCheckPhoneNumberAccessTest(SYSTEM_PID, SYSTEM_UID, -1); + when(mInjector.checkPermission(android.Manifest.permission.READ_PRIVILEGED_PHONE_STATE, + telephonyPid, telephonyUid)).thenReturn(PackageManager.PERMISSION_GRANTED); + + int result = mLegacyPermissionManagerService.checkPhoneNumberAccess(mPackageName, + CHECK_PHONE_NUMBER_MESSAGE, null, telephonyPid, telephonyUid); + + assertEquals(PackageManager.PERMISSION_GRANTED, result); + } + /** * Configures device identifier access tests to fail; tests verifying access should individually * set an access check to succeed to verify access when that condition is met. */ private void setupCheckDeviceIdentifierAccessTest(int callingPid, int callingUid) { - setupAccessTest(callingPid, callingUid); + setupCheckDeviceIdentifierAccessTest(callingPid, callingUid, callingUid); + } + + /** + * Configures device identifier access tests to fail; tests verifying access should individually + * set an access check to succeed to verify access when that condition is met. + * + *

To prevent leaking package information, access checks for package UIDs >= {@link + * android.os.Process#FIRST_APPLICATION_UID} must ensure the provided uid matches the uid of + * the package being checked; to ensure this check is successful, this method accepts the + * {@code packageUid} to be used for the package being checked. + */ + public void setupCheckDeviceIdentifierAccessTest(int callingPid, int callingUid, + int packageUid) { + setupAccessTest(callingPid, callingUid, packageUid); when(mDevicePolicyManager.hasDeviceIdentifierAccess(anyString(), anyInt(), anyInt())).thenReturn(false); @@ -333,11 +400,26 @@ public class LegacyPermissionManagerServiceTest { } /** - * Configures phone number access tests to fail; tests verifying access should individually set - * an access check to succeed to verify access when that condition is met. + * Configures phone number access tests to fail; tests verifying access should individually + * set an access check to succeed to verify access when that condition is set. + * */ private void setupCheckPhoneNumberAccessTest(int callingPid, int callingUid) throws Exception { - setupAccessTest(callingPid, callingUid); + setupCheckPhoneNumberAccessTest(callingPid, callingUid, callingUid); + } + + /** + * Configures phone number access tests to fail; tests verifying access should individually set + * an access check to succeed to verify access when that condition is met. + * + *

To prevent leaking package information, access checks for package UIDs >= {@link + * android.os.Process#FIRST_APPLICATION_UID} must ensure the provided uid matches the uid of + * the package being checked; to ensure this check is successful, this method accepts the + * {@code packageUid} to be used for the package being checked. + */ + private void setupCheckPhoneNumberAccessTest(int callingPid, int callingUid, int packageUid) + throws Exception { + setupAccessTest(callingPid, callingUid, packageUid); setPackageTargetSdk(Build.VERSION_CODES.R); } @@ -345,9 +427,10 @@ public class LegacyPermissionManagerServiceTest { * Configures the common mocks for any access tests using the provided {@code callingPid} * and {@code callingUid}. */ - private void setupAccessTest(int callingPid, int callingUid) { + private void setupAccessTest(int callingPid, int callingUid, int packageUid) { when(mInjector.getCallingPid()).thenReturn(callingPid); when(mInjector.getCallingUid()).thenReturn(callingUid); + when(mInjector.getPackageUidForUser(anyString(), anyInt())).thenReturn(packageUid); when(mInjector.checkPermission(anyString(), anyInt(), anyInt())).thenReturn( PackageManager.PERMISSION_DENIED);