From 97c29c74766cd6bee5d36792e52ba769b77b1406 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
Merged-In: 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 b3eb53116d495..ffa518e266d9b 100644
--- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
+++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
@@ -586,7 +586,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();
 
@@ -972,7 +972,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);
                 }
             }
@@ -4194,16 +4194,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) {
         }