From dca7230c1d28fcf12688dd1a8446ffd17e3afd8c Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Mon, 28 Jun 2021 04:17:53 +0000 Subject: [PATCH] Use appId instead of uid Multiple user's ares of mApps are not initialized in startMonitoring(), so mApps.get(uid) of multi-user's app returns null in onPackageAdded(). As the result, permission of system uid is updated to "Network" and any system application cannot use dedicated apn like IMS. Using appId avoids this problem. Bug: 168932048 Test: atest FrameworksNetTests Signed-off-by: Sangcheol Lee Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1733212 Merged-In: Ib8ea2185d8056bddb2ca5a8006f83afb3cffc9f4 Change-Id: Ib8ea2185d8056bddb2ca5a8006f83afb3cffc9f4 --- .../connectivity/PermissionMonitor.java | 41 ++++++++++--------- .../connectivity/PermissionMonitorTest.java | 21 +++++++++- 2 files changed, 42 insertions(+), 20 deletions(-) mode change 100644 => 100755 service/src/com/android/server/connectivity/PermissionMonitor.java diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java old mode 100644 new mode 100755 index 332884660c..e98a638ce6 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -230,11 +230,11 @@ public class PermissionMonitor { boolean hasRestrictedPermission = hasRestrictedNetworkPermission(app); if (isNetwork || hasRestrictedPermission) { - Boolean permission = mApps.get(uid); + Boolean permission = mApps.get(UserHandle.getAppId(uid)); // If multiple packages share a UID (cf: android:sharedUserId) and ask for different // permissions, don't downgrade (i.e., if it's already SYSTEM, leave it as is). if (permission == null || permission == NETWORK) { - mApps.put(uid, hasRestrictedPermission); + mApps.put(UserHandle.getAppId(uid), hasRestrictedPermission); } } @@ -325,14 +325,14 @@ public class PermissionMonitor { // networks. mApps contains the result of checks for both hasNetworkPermission and // hasRestrictedNetworkPermission. If uid is in the mApps list that means uid has one of // permissions at least. - return mApps.containsKey(uid); + return mApps.containsKey(UserHandle.getAppId(uid)); } /** * Returns whether the given uid has permission to use restricted networks. */ public synchronized boolean hasRestrictedNetworksPermission(int uid) { - return Boolean.TRUE.equals(mApps.get(uid)); + return Boolean.TRUE.equals(mApps.get(UserHandle.getAppId(uid))); } private void update(Set users, Map apps, boolean add) { @@ -452,12 +452,13 @@ public class PermissionMonitor { // If multiple packages share a UID (cf: android:sharedUserId) and ask for different // permissions, don't downgrade (i.e., if it's already SYSTEM, leave it as is). - final Boolean permission = highestPermissionForUid(mApps.get(uid), packageName); - if (permission != mApps.get(uid)) { - mApps.put(uid, permission); + final int appId = UserHandle.getAppId(uid); + final Boolean permission = highestPermissionForUid(mApps.get(appId), packageName); + if (permission != mApps.get(appId)) { + mApps.put(appId, permission); Map apps = new HashMap<>(); - apps.put(uid, permission); + apps.put(appId, permission); update(mUsers, apps, true); } @@ -472,7 +473,7 @@ public class PermissionMonitor { updateVpnUids(vpn.getKey(), changedUids, true); } } - mAllApps.add(UserHandle.getAppId(uid)); + mAllApps.add(appId); } private Boolean highestUidNetworkPermission(int uid) { @@ -529,16 +530,17 @@ public class PermissionMonitor { return; } - if (permission == mApps.get(uid)) { + final int appId = UserHandle.getAppId(uid); + if (permission == mApps.get(appId)) { // The permissions of this UID have not changed. Nothing to do. return; } else if (permission != null) { - mApps.put(uid, permission); - apps.put(uid, permission); + mApps.put(appId, permission); + apps.put(appId, permission); update(mUsers, apps, true); } else { - mApps.remove(uid); - apps.put(uid, NETWORK); // doesn't matter which permission we pick here + mApps.remove(appId); + apps.put(appId, NETWORK); // doesn't matter which permission we pick here update(mUsers, apps, false); } } @@ -653,7 +655,7 @@ public class PermissionMonitor { */ private void removeBypassingUids(Set uids, int vpnAppUid) { uids.remove(vpnAppUid); - uids.removeIf(uid -> mApps.getOrDefault(uid, NETWORK) == SYSTEM); + uids.removeIf(uid -> mApps.getOrDefault(UserHandle.getAppId(uid), NETWORK) == SYSTEM); } /** @@ -795,12 +797,13 @@ public class PermissionMonitor { for (Integer uid : uidsToUpdate) { final Boolean permission = highestUidNetworkPermission(uid); + final int appId = UserHandle.getAppId(uid); if (null == permission) { - removedUids.put(uid, NETWORK); // Doesn't matter which permission is set here. - mApps.remove(uid); + removedUids.put(appId, NETWORK); // Doesn't matter which permission is set here. + mApps.remove(appId); } else { - updatedUids.put(uid, permission); - mApps.put(uid, permission); + updatedUids.put(appId, permission); + mApps.put(appId, permission); } } diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index e98f5db564..db63495338 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -521,13 +521,13 @@ public class PermissionMonitorTest { // MOCK_UID1: MOCK_PACKAGE1 only has network permission. // SYSTEM_UID: SYSTEM_PACKAGE1 has system permission. // SYSTEM_UID: SYSTEM_PACKAGE2 only has network permission. - doReturn(SYSTEM).when(mPermissionMonitor).highestPermissionForUid(eq(SYSTEM), anyString()); doReturn(SYSTEM).when(mPermissionMonitor).highestPermissionForUid(any(), eq(SYSTEM_PACKAGE1)); doReturn(NETWORK).when(mPermissionMonitor).highestPermissionForUid(any(), eq(SYSTEM_PACKAGE2)); doReturn(NETWORK).when(mPermissionMonitor).highestPermissionForUid(any(), eq(MOCK_PACKAGE1)); + doReturn(SYSTEM).when(mPermissionMonitor).highestPermissionForUid(eq(SYSTEM), anyString()); // Add SYSTEM_PACKAGE2, expect only have network permission. mPermissionMonitor.onUserAdded(MOCK_USER1); @@ -542,6 +542,21 @@ public class PermissionMonitorTest { netdMonitor.expectPermission(SYSTEM, new UserHandle[]{MOCK_USER1, MOCK_USER2}, new int[]{SYSTEM_UID}); + // Remove SYSTEM_PACKAGE2, expect keep system permission. + when(mPackageManager.getPackagesForUid(MOCK_USER1.getUid(SYSTEM_UID))) + .thenReturn(new String[]{SYSTEM_PACKAGE1}); + when(mPackageManager.getPackagesForUid(MOCK_USER2.getUid(SYSTEM_UID))) + .thenReturn(new String[]{SYSTEM_PACKAGE1}); + removePackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, + SYSTEM_PACKAGE2, SYSTEM_UID); + netdMonitor.expectPermission(SYSTEM, new UserHandle[]{MOCK_USER1, MOCK_USER2}, + new int[]{SYSTEM_UID}); + + // Add SYSTEM_PACKAGE2, expect keep system permission. + addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, SYSTEM_PACKAGE2, SYSTEM_UID); + netdMonitor.expectPermission(SYSTEM, new UserHandle[]{MOCK_USER1, MOCK_USER2}, + new int[]{SYSTEM_UID}); + addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_UID1); netdMonitor.expectPermission(SYSTEM, new UserHandle[]{MOCK_USER1, MOCK_USER2}, new int[]{SYSTEM_UID}); @@ -549,6 +564,10 @@ public class PermissionMonitorTest { new int[]{MOCK_UID1}); // Remove MOCK_UID1, expect no permission left for all user. + when(mPackageManager.getPackagesForUid(MOCK_USER1.getUid(MOCK_UID1))) + .thenReturn(new String[]{}); + when(mPackageManager.getPackagesForUid(MOCK_USER2.getUid(MOCK_UID1))) + .thenReturn(new String[]{}); mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID1); removePackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_UID1); netdMonitor.expectNoPermission(new UserHandle[]{MOCK_USER1, MOCK_USER2},