From fa8340654b3f24b4ac1f95e7a9d52a1b105b81a3 Mon Sep 17 00:00:00 2001
From: Sudheer Shanka <sudheersai@google.com>
Date: Thu, 1 Oct 2020 03:19:00 -0700
Subject: [PATCH] Update INTERNET perm state cache to use uids instead of
 appIds.

It really shouldn't have mattered whether we use uids or appIds for
this cache but given the way NetworkPolicy iterates over all apps
on the device (all_apps * all_users), it is possible that we end
up checking the permission state of apps which are not installed
on a user which will always be DENIED and we end up caching this.
So, we could end up treating an app as not having INTERNET permission
on a user even though it has.

Also, update the cache stragety to always check with PackageManager
when the permission state is denied in the cache just to be safe, until
NetworkPolicy iteration of apps is fixed.

Bug: 168299219
Test: atest cts/hostsidetests/net/src/com/android/cts/net/HostsideRestrictBackgroundNetworkTests.java
Test: manual

Change-Id: I6f2a60695a519a972c96ec8e053d3be5dc732461
---
 .../net/NetworkPolicyManagerService.java      | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
index 2acc60db52e3b..f215ec5910ae9 100644
--- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
+++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
@@ -579,7 +579,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
 
     private final NetworkPolicyLogger mLogger = new NetworkPolicyLogger();
 
-    /** List of apps indexed by appId and whether they have the internet permission */
+    /** List of apps indexed by uid and whether they have the internet permission */
     @GuardedBy("mUidRulesFirstLock")
     private final SparseBooleanArray mInternetPermissionMap = new SparseBooleanArray();
 
@@ -965,7 +965,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
                 if (LOGV) Slog.v(TAG, "ACTION_PACKAGE_ADDED for uid=" + uid);
                 // Clear the cache for the app
                 synchronized (mUidRulesFirstLock) {
-                    mInternetPermissionMap.delete(UserHandle.getAppId(uid));
+                    mInternetPermissionMap.delete(uid);
                     updateRestrictionRulesForUidUL(uid);
                 }
             }
@@ -4179,16 +4179,14 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
     @GuardedBy("mUidRulesFirstLock")
     private boolean hasInternetPermissionUL(int uid) {
         try {
-            final int appId = UserHandle.getAppId(uid);
-            final boolean hasPermission;
-            if (mInternetPermissionMap.indexOfKey(appId) < 0) {
-                hasPermission =
-                        mIPm.checkUidPermission(Manifest.permission.INTERNET, uid)
-                                == PackageManager.PERMISSION_GRANTED;
-                mInternetPermissionMap.put(appId, hasPermission);
-            } else {
-                hasPermission = mInternetPermissionMap.get(appId);
+            if (mInternetPermissionMap.get(uid)) {
+                return true;
             }
+            // If the cache shows that uid doesn't have internet permission,
+            // then always re-check with PackageManager just to be safe.
+            final boolean hasPermission = mIPm.checkUidPermission(Manifest.permission.INTERNET,
+                    uid) == PackageManager.PERMISSION_GRANTED;
+            mInternetPermissionMap.put(uid, hasPermission);
             return hasPermission;
         } catch (RemoteException e) {
         }