From 4fd13eefcf99d9b9b0d5f5ea99fdc7c799c83d23 Mon Sep 17 00:00:00 2001 From: Eran Messeri Date: Mon, 25 Mar 2019 14:31:04 +0000 Subject: [PATCH 1/8] Permission Check For DPM.getPermittedAccessibilityServices Bug: 128599660 Test: com.android.server.devicepolicy.DevicePolicyManagerTest Test: com.google.android.gts.devicepolicy.DeviceOwnerTest Change-Id: I8be915bd6a4ff99884d23005a4c6f0100806dbe8 Merged-In: I8ee3f876fcaffa63636645f0f59709cd147254ef --- .../android/server/devicepolicy/DevicePolicyManagerService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index c09799e598357..64e9cea70501d 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -6868,6 +6868,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { if (!mHasFeature) { return null; } + enforceManageUsers(); synchronized (this) { List result = null; // If we have multiple profiles we return the intersection of the From 18e7dedf6c35f07daf8b7239d501737745ac7f43 Mon Sep 17 00:00:00 2001 From: Varun Shah Date: Wed, 20 Mar 2019 11:10:33 -0700 Subject: [PATCH 2/8] Added missing permission check to isPackageDeviceAdminOnAnyUser. Added a check for the MANAGE_USERS permission to PackageManagerService#isPackageDeviceAdminOnAnyUser. To test that the method is still usable: 1) Enable virtual storage via: adb shell sm set-virtual-disk true 2) Follow instructions by clicking on notification to set up virtual storage 3) Go to Settings -> Apps & notifications -> See all X apps 4) Click on any non-system app (example Instagram) 5) Tap Storage and you should see a "Change" button (if not, choose another app) 6) Tap Change and you should see Internal and Virtual storage options listed 7) The above step confirms the method is still usable by Settings Bug: 128599183 Test: SafetyNet logging (steps listed above) Change-Id: I989f1daf52a71f6c778ebd81baa6f1bf83e9a718 Merged-In: I36521fa43daab399e08869647326a7ac32d1e512 --- .../java/com/android/server/pm/PackageManagerService.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 2f4e94823642f..fe7c8738af6cb 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -18839,6 +18839,12 @@ public class PackageManagerService extends IPackageManager.Stub @Override public boolean isPackageDeviceAdminOnAnyUser(String packageName) { final int callingUid = Binder.getCallingUid(); + if (checkUidPermission(android.Manifest.permission.MANAGE_USERS, callingUid) + != PERMISSION_GRANTED) { + EventLog.writeEvent(0x534e4554, "128599183", -1, ""); + throw new SecurityException(android.Manifest.permission.MANAGE_USERS + + " permission is required to call this API"); + } if (getInstantAppPackageName(callingUid) != null && !isCallerSameApp(packageName, callingUid)) { return false; From 7b5a576965696747041c93306a41ed656404ed20 Mon Sep 17 00:00:00 2001 From: Bryan Ferris Date: Thu, 4 Apr 2019 15:18:52 -0700 Subject: [PATCH 3/8] [RESTRICT AUTOMERGE] Added missing permission check to isPackageDeviceAdminOnAnyUser. Added a check for the MANAGE_USERS permission to PackageManagerService#isPackageDeviceAdminOnAnyUser. Test: Modify the settings app to log the call attempt and follow the steps below In order to work around the limitations of N builds we needed to modify the settings app to log the call attempt. This is described in detail at b/128599183#comment15 Bug: 128599183 Change-Id: Ie96c8e174983f61574f12d5d4b210d06377054e5 --- .../java/com/android/server/pm/PackageManagerService.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 949307412a0b2..478ee29856eb5 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -15637,6 +15637,13 @@ public class PackageManagerService extends IPackageManager.Stub { @Override public boolean isPackageDeviceAdminOnAnyUser(String packageName) { + final int callingUid = Binder.getCallingUid(); + if (checkUidPermission(android.Manifest.permission.MANAGE_USERS, callingUid) + != PERMISSION_GRANTED) { + EventLog.writeEvent(0x534e4554, "128599183", -1, ""); + throw new SecurityException(android.Manifest.permission.MANAGE_USERS + + " permission is required to call this API"); + } return isPackageDeviceAdmin(packageName, UserHandle.USER_ALL); } From a2b2e377a0a63cbe9c8bfdfcab1167e55f36a59b Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Wed, 27 Mar 2019 12:15:57 -0400 Subject: [PATCH 4/8] [RESTRICT AUTOMERGE]: Add cross user permission check - areNotificationsEnabledForPackage Test: atest Fixes: 128599467 Change-Id: I13a0ca7590f8c4b44379730e0ee2088aba400c2a (cherry picked from commit 657d164136199126ae241848887de0230699cea0) --- .../server/notification/NotificationManagerService.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index ee09e163988e8..d4e5dfae61948 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -1413,6 +1413,12 @@ public class NotificationManagerService extends SystemService { @Override public boolean areNotificationsEnabledForPackage(String pkg, int uid) { checkCallerIsSystemOrSameApp(pkg); + if (UserHandle.getCallingUserId() != UserHandle.getUserId(uid)) { + getContext().enforceCallingPermission( + android.Manifest.permission.INTERACT_ACROSS_USERS, + "canNotifyAsPackage for uid " + uid); + } + return (mAppOps.checkOpNoThrow(AppOpsManager.OP_POST_NOTIFICATION, uid, pkg) == AppOpsManager.MODE_ALLOWED) && !isPackageSuspendedForUser(pkg, uid); } From 1b6301cf2430f192c9842a05fc22984d782bade9 Mon Sep 17 00:00:00 2001 From: Pavel Grafov Date: Wed, 10 Apr 2019 12:47:25 +0100 Subject: [PATCH 5/8] Limit IsSeparateProfileChallengeAllowed to system callers Fixes: 128599668 Test: build, set up separate challenge Change-Id: I2fef9ab13614627c0f1bcca04759d0974fc6181a --- .../server/devicepolicy/DevicePolicyManagerService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index aac341c1e6e4e..68b078f80c669 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -3428,6 +3428,9 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { @Override public boolean isSeparateProfileChallengeAllowed(int userHandle) { + if (!isCallerWithSystemUid()) { + throw new SecurityException("Caller must be system"); + } ComponentName profileOwner = getProfileOwner(userHandle); // Profile challenge is supported on N or newer release. return profileOwner != null && From 9061fcc46bb1ac5ffc16d036b632dd80963b7b52 Mon Sep 17 00:00:00 2001 From: Pavel Grafov Date: Wed, 10 Apr 2019 12:47:25 +0100 Subject: [PATCH 6/8] Limit IsSeparateProfileChallengeAllowed to system callers Fixes: 128599668 Test: build, set up separate challenge Merged-In: I2fef9ab13614627c0f1bcca04759d0974fc6181a Change-Id: I2fef9ab13614627c0f1bcca04759d0974fc6181a --- .../server/devicepolicy/DevicePolicyManagerService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index c09799e598357..9cc82142e37dd 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -2999,6 +2999,9 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { @Override public boolean isSeparateProfileChallengeAllowed(int userHandle) { + if (!isCallerWithSystemUid()) { + throw new SecurityException("Caller must be system"); + } ComponentName profileOwner = getProfileOwner(userHandle); // Profile challenge is supported on N or newer release. return profileOwner != null && From bed6193b5954565f60afb5f5f9868d89991354d7 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Wed, 27 Mar 2019 12:15:57 -0400 Subject: [PATCH 7/8] [RESTRICT_AUTOMERGE]: Add cross user permission check - areNotificationsEnabledForPackage Test: atest Fixes: 128599467 Change-Id: I13a0ca7590f8c4b44379730e0ee2088aba400c2a (cherry picked from commit 657d164136199126ae241848887de0230699cea0) --- .../server/notification/NotificationManagerService.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index c3f9dc96c67b5..8a7d6000dd017 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -1631,6 +1631,11 @@ public class NotificationManagerService extends SystemService { @Override public boolean areNotificationsEnabledForPackage(String pkg, int uid) { checkCallerIsSystemOrSameApp(pkg); + if (UserHandle.getCallingUserId() != UserHandle.getUserId(uid)) { + getContext().enforceCallingPermission( + android.Manifest.permission.INTERACT_ACROSS_USERS, + "canNotifyAsPackage for uid " + uid); + } return mRankingHelper.getImportance(pkg, uid) != IMPORTANCE_NONE; } From 63846a7093ca7c6d89b73fc77bdff267b3ecb4ef Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Wed, 27 Mar 2019 12:15:57 -0400 Subject: [PATCH 8/8] Add cross user permission check - areNotificationsEnabledForPackage Test: atest Fixes: 128599467 Change-Id: I13a0ca7590f8c4b44379730e0ee2088aba400c2a Merged-In: I13a0ca7590f8c4b44379730e0ee2088aba400c2a (cherry picked from commit 657d164136199126ae241848887de0230699cea0) --- .../NotificationManagerService.java | 5 +++++ .../NotificationManagerServiceTest.java | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 82363d102c407..1928cb7d0f0e6 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -2186,6 +2186,11 @@ public class NotificationManagerService extends SystemService { @Override public boolean areNotificationsEnabledForPackage(String pkg, int uid) { checkCallerIsSystemOrSameApp(pkg); + if (UserHandle.getCallingUserId() != UserHandle.getUserId(uid)) { + getContext().enforceCallingPermission( + android.Manifest.permission.INTERACT_ACROSS_USERS, + "canNotifyAsPackage for uid " + uid); + } return mRankingHelper.getImportance(pkg, uid) != IMPORTANCE_NONE; } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index f02c3f062f352..5622622e925ea 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -34,6 +34,7 @@ import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_SCREEN_ON import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_STATUS_BAR; import static android.content.pm.PackageManager.FEATURE_WATCH; import static android.content.pm.PackageManager.PERMISSION_DENIED; +import static android.content.pm.PackageManager.PERMISSION_GRANTED; import static android.os.Build.VERSION_CODES.O_MR1; import static android.os.Build.VERSION_CODES.P; @@ -106,6 +107,7 @@ import android.testing.AndroidTestingRunner; import android.testing.TestableContext; import android.testing.TestableLooper; import android.testing.TestableLooper.RunWithLooper; +import android.testing.TestablePermissions; import android.text.Html; import android.util.ArrayMap; import android.util.AtomicFile; @@ -3145,4 +3147,21 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals(0, captor.getValue().getNotification().flags); } + + @Test + public void testAreNotificationsEnabledForPackage_crossUser() throws Exception { + try { + mBinderService.areNotificationsEnabledForPackage(mContext.getPackageName(), + mUid + UserHandle.PER_USER_RANGE); + fail("Cannot call cross user without permission"); + } catch (SecurityException e) { + // pass + } + + // cross user, with permission, no problem + TestablePermissions perms = mContext.getTestablePermissions(); + perms.setPermission(android.Manifest.permission.INTERACT_ACROSS_USERS, PERMISSION_GRANTED); + mBinderService.areNotificationsEnabledForPackage(mContext.getPackageName(), + mUid + UserHandle.PER_USER_RANGE); + } }