Merge "Sanitize owner UID iff owning app does not have location permissions." am: 4d95254cb3 am: aba23b3f43 am: a06f220d24

Change-Id: I9b8fb3ff8d907ecda918a8a926edb133b742c8f1

core/java/android/net/ConnectivityManager.java

@@ -1279,7 +1279,8 @@ public class ConnectivityManager {
     @UnsupportedAppUsage
     public NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(int userId) {
         try {
-            return mService.getDefaultNetworkCapabilitiesForUser(userId);
+            return mService.getDefaultNetworkCapabilitiesForUser(
+                    userId, mContext.getOpPackageName());
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
@@ -1361,7 +1362,7 @@ public class ConnectivityManager {
     @Nullable
     public NetworkCapabilities getNetworkCapabilities(@Nullable Network network) {
         try {
-            return mService.getNetworkCapabilities(network);
+            return mService.getNetworkCapabilities(network, mContext.getOpPackageName());
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
@@ -4039,10 +4040,9 @@ public class ConnectivityManager {
             @NonNull PendingIntent operation) {
         printStackTrace();
         checkPendingIntentNotNull(operation);
-        final String callingPackageName = mContext.getOpPackageName();
         try {
             mService.pendingRequestForNetwork(
-                    request.networkCapabilities, operation, callingPackageName);
+                    request.networkCapabilities, operation, mContext.getOpPackageName());
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         } catch (ServiceSpecificException e) {
@@ -4154,10 +4154,9 @@ public class ConnectivityManager {
             @NonNull PendingIntent operation) {
         printStackTrace();
         checkPendingIntentNotNull(operation);
-        final String callingPackageName = mContext.getOpPackageName();
         try {
             mService.pendingListenForNetwork(
-                    request.networkCapabilities, operation, callingPackageName);
+                    request.networkCapabilities, operation, mContext.getOpPackageName());
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         } catch (ServiceSpecificException e) {

core/java/android/net/IConnectivityManager.aidl

@@ -60,7 +60,8 @@ interface IConnectivityManager
     NetworkInfo[] getAllNetworkInfo();
     Network getNetworkForType(int networkType);
     Network[] getAllNetworks();
-    NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(int userId);
+    NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(
+            int userId, String callingPackageName);
 
     boolean isNetworkSupported(int networkType);
 
@@ -69,7 +70,7 @@ interface IConnectivityManager
     LinkProperties getLinkPropertiesForType(int networkType);
     LinkProperties getLinkProperties(in Network network);
 
-    NetworkCapabilities getNetworkCapabilities(in Network network);
+    NetworkCapabilities getNetworkCapabilities(in Network network, String callingPackageName);
 
     @UnsupportedAppUsage
     NetworkState[] getAllNetworkState();

core/java/android/net/NetworkCapabilities.java

@@ -830,6 +830,23 @@ public final class NetworkCapabilities implements Parcelable {
      * <p>This field keeps track of the UID of the app that created this network and is in charge of
      * its lifecycle. This could be the UID of apps such as the Wifi network suggestor, the running
      * VPN, or Carrier Service app managing a cellular data connection.
+     *
+     * <p>For NetworkCapability instances being sent from ConnectivityService, this value MUST be
+     * reset to Process.INVALID_UID unless all the following conditions are met:
+     *
+     * <ol>
+     *   <li>The destination app is the network owner
+     *   <li>The destination app has the ACCESS_FINE_LOCATION permission granted
+     *   <li>The user's location toggle is on
+     * </ol>
+     *
+     * This is because the owner UID is location-sensitive. The apps that request a network could
+     * know where the device is if they can tell for sure the system has connected to the network
+     * they requested.
+     *
+     * <p>This is populated by the network agents and for the NetworkCapabilities instance sent by
+     * an app to the System Server, the value MUST be reset to Process.INVALID_UID by the system
+     * server.
      */
     private int mOwnerUid = Process.INVALID_UID;
 
@@ -842,7 +859,16 @@ public final class NetworkCapabilities implements Parcelable {
     }
 
     /**
-     * Retrieves the UID of the owner app.
+     * Retrieves the UID of the app that owns this network.
+     *
+     * <p>For user privacy reasons, this field will only be populated if:
+     *
+     * <ol>
+     *   <li>The calling app is the network owner
+     *   <li>The calling app has the ACCESS_FINE_LOCATION permission granted
+     *   <li>The user's location toggle is on
+     * </ol>
+     *
      */
     public int getOwnerUid() {
         return mOwnerUid;
@@ -880,8 +906,9 @@ public final class NetworkCapabilities implements Parcelable {
      * @param administratorUids the UIDs to be set as administrators of this Network.
      * @hide
      */
+    @NonNull
     @SystemApi
-    public @NonNull NetworkCapabilities setAdministratorUids(
+    public NetworkCapabilities setAdministratorUids(
             @NonNull final List<Integer> administratorUids) {
         mAdministratorUids.clear();
         mAdministratorUids.addAll(administratorUids);

services/core/java/com/android/server/ConnectivityService.java

@@ -1534,7 +1534,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     @Override
-    public NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(int userId) {
+    public NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(
+                int userId, String callingPackageName) {
         // The basic principle is: if an app's traffic could possibly go over a
         // network, without the app doing anything multinetwork-specific,
         // (hence, by "default"), then include that network's capabilities in
@@ -1556,7 +1557,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
         NetworkAgentInfo nai = getDefaultNetwork();
         NetworkCapabilities nc = getNetworkCapabilitiesInternal(nai);
         if (nc != null) {
-            result.put(nai.network, nc);
+            result.put(
+                    nai.network,
+                    maybeSanitizeLocationInfoForCaller(
+                            nc, Binder.getCallingUid(), callingPackageName));
         }
 
         synchronized (mVpns) {
@@ -1566,10 +1570,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     Network[] networks = vpn.getUnderlyingNetworks();
                     if (networks != null) {
                         for (Network network : networks) {
-                            nai = getNetworkAgentInfoForNetwork(network);
+                            nc = getNetworkCapabilitiesInternal(network);
-                            nc = getNetworkCapabilitiesInternal(nai);
                             if (nc != null) {
-                                result.put(network, nc);
+                                result.put(
+                                        network,
+                                        maybeSanitizeLocationInfoForCaller(
+                                                nc, Binder.getCallingUid(), callingPackageName));
                             }
                         }
                     }
@@ -1636,20 +1642,26 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    private NetworkCapabilities getNetworkCapabilitiesInternal(Network network) {
+        return getNetworkCapabilitiesInternal(getNetworkAgentInfoForNetwork(network));
+    }
+
     private NetworkCapabilities getNetworkCapabilitiesInternal(NetworkAgentInfo nai) {
         if (nai == null) return null;
         synchronized (nai) {
             if (nai.networkCapabilities == null) return null;
             return networkCapabilitiesRestrictedForCallerPermissions(
-                    nai.networkCapabilities,
+                    nai.networkCapabilities, Binder.getCallingPid(), Binder.getCallingUid());
-                    Binder.getCallingPid(), Binder.getCallingUid());
         }
     }
 
     @Override
-    public NetworkCapabilities getNetworkCapabilities(Network network) {
+    public NetworkCapabilities getNetworkCapabilities(Network network, String callingPackageName) {
+        mAppOpsManager.checkPackage(Binder.getCallingUid(), callingPackageName);
         enforceAccessPermission();
-        return getNetworkCapabilitiesInternal(getNetworkAgentInfoForNetwork(network));
+        return maybeSanitizeLocationInfoForCaller(
+                getNetworkCapabilitiesInternal(network),
+                Binder.getCallingUid(), callingPackageName);
     }
 
     @VisibleForTesting
@@ -1665,20 +1677,34 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
         newNc.setAdministratorUids(Collections.EMPTY_LIST);
 
-        maybeSanitizeLocationInfoForCaller(newNc, callerUid);
-
         return newNc;
     }
 
-    private void maybeSanitizeLocationInfoForCaller(
+    @VisibleForTesting
-            NetworkCapabilities nc, int callerUid) {
+    @Nullable
-        // TODO(b/142072839): Conditionally reset the owner UID if the following
+    NetworkCapabilities maybeSanitizeLocationInfoForCaller(
-        // conditions are not met:
+            @Nullable NetworkCapabilities nc, int callerUid, @NonNull String callerPkgName) {
-        // 1. The destination app is the network owner
+        if (nc == null) {
-        // 2. The destination app has the ACCESS_COARSE_LOCATION permission granted
+            return null;
-        // if target SDK<29 or otherwise has the ACCESS_FINE_LOCATION permission granted
+        }
-        // 3. The user's location toggle is on
+        final NetworkCapabilities newNc = new NetworkCapabilities(nc);
-        nc.setOwnerUid(INVALID_UID);
+        if (callerUid != newNc.getOwnerUid()) {
+            newNc.setOwnerUid(INVALID_UID);
+            return newNc;
+        }
+
+        Binder.withCleanCallingIdentity(
+                () -> {
+                    if (!mLocationPermissionChecker.checkLocationPermission(
+                            callerPkgName, null /* featureId */, callerUid, null /* message */)) {
+                        // Caller does not have the requisite location permissions. Reset the
+                        // owner's UID in the NetworkCapabilities.
+                        newNc.setOwnerUid(INVALID_UID);
+                    }
+                }
+        );
+
+        return newNc;
     }
 
     private LinkProperties linkPropertiesRestrictedForCallerPermissions(
@@ -1753,7 +1779,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
     public boolean isActiveNetworkMetered() {
         enforceAccessPermission();
 
-        final NetworkCapabilities caps = getNetworkCapabilities(getActiveNetwork());
+        final NetworkCapabilities caps = getNetworkCapabilitiesInternal(getActiveNetwork());
         if (caps != null) {
             return !caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED);
         } else {
@@ -5330,8 +5356,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
 
         public String toString() {
-            return "uid/pid:" + mUid + "/" + mPid + " " + request +
+            return "uid/pid:" + mUid + "/" + mPid + " " + request
-                    (mPendingIntent == null ? "" : " to trigger " + mPendingIntent);
+                    + (mPendingIntent == null ? "" : " to trigger " + mPendingIntent);
         }
     }
 
@@ -6408,8 +6434,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
         switch (notificationType) {
             case ConnectivityManager.CALLBACK_AVAILABLE: {
-                putParcelable(bundle, networkCapabilitiesRestrictedForCallerPermissions(
+                final NetworkCapabilities nc =
-                        networkAgent.networkCapabilities, nri.mPid, nri.mUid));
+                        networkCapabilitiesRestrictedForCallerPermissions(
+                                networkAgent.networkCapabilities, nri.mPid, nri.mUid);
+                putParcelable(
+                        bundle,
+                        maybeSanitizeLocationInfoForCaller(
+                                nc, nri.mUid, nri.request.getRequestorPackageName()));
                 putParcelable(bundle, linkPropertiesRestrictedForCallerPermissions(
                         networkAgent.linkProperties, nri.mPid, nri.mUid));
                 // For this notification, arg1 contains the blocked status.
@@ -6422,9 +6453,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
             }
             case ConnectivityManager.CALLBACK_CAP_CHANGED: {
                 // networkAgent can't be null as it has been accessed a few lines above.
-                final NetworkCapabilities nc = networkCapabilitiesRestrictedForCallerPermissions(
+                final NetworkCapabilities netCap =
-                        networkAgent.networkCapabilities, nri.mPid, nri.mUid);
+                        networkCapabilitiesRestrictedForCallerPermissions(
-                putParcelable(bundle, nc);
+                                networkAgent.networkCapabilities, nri.mPid, nri.mUid);
+                putParcelable(
+                        bundle,
+                        maybeSanitizeLocationInfoForCaller(
+                                netCap, nri.mUid, nri.request.getRequestorPackageName()));
                 break;
             }
             case ConnectivityManager.CALLBACK_IP_CHANGED: {

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -1180,6 +1180,10 @@ public class ConnectivityServiceTest {
                 Arrays.asList(new UserInfo[] {
                         new UserInfo(VPN_USER, "", 0),
                 }));
+        final ApplicationInfo applicationInfo = new ApplicationInfo();
+        applicationInfo.targetSdkVersion = Build.VERSION_CODES.Q;
+        when(mPackageManager.getApplicationInfoAsUser(anyString(), anyInt(), any()))
+                .thenReturn(applicationInfo);
 
         // InstrumentationTestRunner prepares a looper, but AndroidJUnitRunner does not.
         // http://b/25897652 .
@@ -3042,7 +3046,7 @@ public class ConnectivityServiceTest {
             networkCapabilities.addTransportType(TRANSPORT_WIFI)
                     .setNetworkSpecifier(new MatchAllNetworkSpecifier());
             mService.requestNetwork(networkCapabilities, null, 0, null,
-                    ConnectivityManager.TYPE_WIFI, TEST_PACKAGE_NAME);
+                    ConnectivityManager.TYPE_WIFI, mContext.getPackageName());
         });
 
         class NonParcelableSpecifier extends NetworkSpecifier {
@@ -6439,17 +6443,89 @@ public class ConnectivityServiceTest {
         assertEquals(wifiLp, mService.getActiveLinkProperties());
     }
 
+    private void setupLocationPermissions(
+            int targetSdk, boolean locationToggle, String op, String perm) throws Exception {
+        final ApplicationInfo applicationInfo = new ApplicationInfo();
+        applicationInfo.targetSdkVersion = targetSdk;
+        when(mPackageManager.getApplicationInfoAsUser(anyString(), anyInt(), any()))
+                .thenReturn(applicationInfo);
+
+        when(mLocationManager.isLocationEnabledForUser(any())).thenReturn(locationToggle);
+
+        if (op != null) {
+            when(mAppOpsManager.noteOp(eq(op), eq(Process.myUid()), eq(mContext.getPackageName())))
+                .thenReturn(AppOpsManager.MODE_ALLOWED);
+        }
+
+        if (perm != null) {
+            mServiceContext.setPermission(perm, PERMISSION_GRANTED);
+        }
+    }
+
+    private int getOwnerUidNetCapsForCallerPermission(int ownerUid, int callerUid) {
+        final NetworkCapabilities netCap = new NetworkCapabilities().setOwnerUid(ownerUid);
+
+        return mService
+                .maybeSanitizeLocationInfoForCaller(netCap, callerUid, mContext.getPackageName())
+                .getOwnerUid();
+    }
+
     @Test
-    public void testNetworkCapabilitiesRestrictedForCallerPermissions() {
+    public void testMaybeSanitizeLocationInfoForCallerWithFineLocationAfterQ() throws Exception {
-        int callerUid = Process.myUid();
+        setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
-        final NetworkCapabilities originalNc = new NetworkCapabilities();
+                Manifest.permission.ACCESS_FINE_LOCATION);
-        originalNc.setOwnerUid(callerUid);
 
-        final NetworkCapabilities newNc =
+        final int myUid = Process.myUid();
-                mService.networkCapabilitiesRestrictedForCallerPermissions(
+        assertEquals(myUid, getOwnerUidNetCapsForCallerPermission(myUid, myUid));
-                        originalNc, Process.myPid(), callerUid);
+    }
 
-        assertEquals(Process.INVALID_UID, newNc.getOwnerUid());
+    @Test
+    public void testMaybeSanitizeLocationInfoForCallerWithCoarseLocationPreQ() throws Exception {
+        setupLocationPermissions(Build.VERSION_CODES.P, true, AppOpsManager.OPSTR_COARSE_LOCATION,
+                Manifest.permission.ACCESS_COARSE_LOCATION);
+
+        final int myUid = Process.myUid();
+        assertEquals(myUid, getOwnerUidNetCapsForCallerPermission(myUid, myUid));
+    }
+
+    @Test
+    public void testMaybeSanitizeLocationInfoForCallerLocationOff() throws Exception {
+        // Test that even with fine location permission, and UIDs matching, the UID is sanitized.
+        setupLocationPermissions(Build.VERSION_CODES.Q, false, AppOpsManager.OPSTR_FINE_LOCATION,
+                Manifest.permission.ACCESS_FINE_LOCATION);
+
+        final int myUid = Process.myUid();
+        assertEquals(Process.INVALID_UID, getOwnerUidNetCapsForCallerPermission(myUid, myUid));
+    }
+
+    @Test
+    public void testMaybeSanitizeLocationInfoForCallerWrongUid() throws Exception {
+        // Test that even with fine location permission, not being the owner leads to sanitization.
+        setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
+                Manifest.permission.ACCESS_FINE_LOCATION);
+
+        final int myUid = Process.myUid();
+        assertEquals(Process.INVALID_UID, getOwnerUidNetCapsForCallerPermission(myUid + 1, myUid));
+    }
+
+    @Test
+    public void testMaybeSanitizeLocationInfoForCallerWithCoarseLocationAfterQ() throws Exception {
+        // Test that not having fine location permission leads to sanitization.
+        setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_COARSE_LOCATION,
+                Manifest.permission.ACCESS_COARSE_LOCATION);
+
+        // Test that without the location permission, the owner field is sanitized.
+        final int myUid = Process.myUid();
+        assertEquals(Process.INVALID_UID, getOwnerUidNetCapsForCallerPermission(myUid, myUid));
+    }
+
+    @Test
+    public void testMaybeSanitizeLocationInfoForCallerWithoutLocationPermission() throws Exception {
+        setupLocationPermissions(Build.VERSION_CODES.Q, true, null /* op */, null /* perm */);
+
+        // Test that without the location permission, the owner field is sanitized.
+        final int myUid = Process.myUid();
+        assertEquals(Process.INVALID_UID, getOwnerUidNetCapsForCallerPermission(myUid, myUid));
     }
 
     private void setupConnectionOwnerUid(int vpnOwnerUid, @VpnManager.VpnType int vpnType)
@@ -6735,21 +6811,6 @@ public class ConnectivityServiceTest {
                         mContext.getOpPackageName()));
     }
 
-    private void setupLocationPermissions(
-            int targetSdk, boolean locationToggle, String op, String perm) throws Exception {
-        final ApplicationInfo applicationInfo = new ApplicationInfo();
-        applicationInfo.targetSdkVersion = targetSdk;
-        when(mPackageManager.getApplicationInfoAsUser(anyString(), anyInt(), any()))
-                .thenReturn(applicationInfo);
-
-        when(mLocationManager.isLocationEnabledForUser(any())).thenReturn(locationToggle);
-
-        when(mAppOpsManager.noteOp(eq(op), eq(Process.myUid()), eq(mContext.getPackageName())))
-                .thenReturn(AppOpsManager.MODE_ALLOWED);
-
-        mServiceContext.setPermission(perm, PERMISSION_GRANTED);
-    }
-
     private void setUpConnectivityDiagnosticsCallback() throws Exception {
         final NetworkRequest request = new NetworkRequest.Builder().build();
         when(mConnectivityDiagnosticsCallback.asBinder()).thenReturn(mIBinder);