From b274810c34780a767f72afb4d747f5fbf4a906fa Mon Sep 17 00:00:00 2001 From: Qingxi Li Date: Wed, 8 Jan 2020 12:51:49 -0800 Subject: [PATCH] Sanitize owner UID iff owning app does not have location permissions. This change adds permission checking to ensure that the following conditions are enforced in order for apps to receive the owner UID: 1. The app must be the owner of the network 2. The app must hold the FINE_LOCATION permission/appop 3. The user must have their location toggle enabled. Bug: 142072839 Test: atest FrameworksNetTests Change-Id: I7a981a82f1219828ee89c8c96eb9d2efd153377f --- .../java/android/net/ConnectivityManager.java | 11 +- .../android/net/IConnectivityManager.aidl | 5 +- .../java/android/net/NetworkCapabilities.java | 31 ++++- .../android/server/ConnectivityService.java | 91 ++++++++++----- .../server/ConnectivityServiceTest.java | 109 ++++++++++++++---- 5 files changed, 185 insertions(+), 62 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index cb3140487f..03d5eb85cf 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/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(); } @@ -4035,10 +4036,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) { @@ -4150,10 +4150,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) { diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index 3a55461a77..14345608e9 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -59,7 +59,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); @@ -68,7 +69,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(); diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index ef4a9e5f3b..873d6e9146 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -830,6 +830,23 @@ public final class NetworkCapabilities implements Parcelable { *

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. + * + *

For NetworkCapability instances being sent from ConnectivityService, this value MUST be + * reset to Process.INVALID_UID unless all the following conditions are met: + * + *

    + *
  1. The destination app is the network owner + *
  2. The destination app has the ACCESS_FINE_LOCATION permission granted + *
  3. The user's location toggle is on + *
+ * + * 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. + * + *

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. + * + *

For user privacy reasons, this field will only be populated if: + * + *

    + *
  1. The calling app is the network owner + *
  2. The calling app has the ACCESS_FINE_LOCATION permission granted + *
  3. The user's location toggle is on + *
+ * */ 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 administratorUids) { mAdministratorUids.clear(); mAdministratorUids.addAll(administratorUids); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1b43fc0fa1..694db4a695 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1527,7 +1527,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 @@ -1549,7 +1550,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) { @@ -1559,10 +1563,12 @@ public class ConnectivityService extends IConnectivityManager.Stub Network[] networks = vpn.getUnderlyingNetworks(); if (networks != null) { for (Network network : networks) { - nai = getNetworkAgentInfoForNetwork(network); - nc = getNetworkCapabilitiesInternal(nai); + nc = getNetworkCapabilitiesInternal(network); if (nc != null) { - result.put(network, nc); + result.put( + network, + maybeSanitizeLocationInfoForCaller( + nc, Binder.getCallingUid(), callingPackageName)); } } } @@ -1629,20 +1635,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, - Binder.getCallingPid(), Binder.getCallingUid()); + nai.networkCapabilities, 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 @@ -1658,20 +1670,34 @@ public class ConnectivityService extends IConnectivityManager.Stub } newNc.setAdministratorUids(Collections.EMPTY_LIST); - maybeSanitizeLocationInfoForCaller(newNc, callerUid); - return newNc; } - private void maybeSanitizeLocationInfoForCaller( - NetworkCapabilities nc, int callerUid) { - // TODO(b/142072839): Conditionally reset the owner UID if the following - // conditions are not met: - // 1. The destination app is the network owner - // 2. The destination app has the ACCESS_COARSE_LOCATION permission granted - // if target SDK<29 or otherwise has the ACCESS_FINE_LOCATION permission granted - // 3. The user's location toggle is on - nc.setOwnerUid(INVALID_UID); + @VisibleForTesting + @Nullable + NetworkCapabilities maybeSanitizeLocationInfoForCaller( + @Nullable NetworkCapabilities nc, int callerUid, @NonNull String callerPkgName) { + if (nc == null) { + return null; + } + final NetworkCapabilities newNc = new NetworkCapabilities(nc); + 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( @@ -1746,7 +1772,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 { @@ -5293,8 +5319,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } public String toString() { - return "uid/pid:" + mUid + "/" + mPid + " " + request + - (mPendingIntent == null ? "" : " to trigger " + mPendingIntent); + return "uid/pid:" + mUid + "/" + mPid + " " + request + + (mPendingIntent == null ? "" : " to trigger " + mPendingIntent); } } @@ -6383,8 +6409,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } switch (notificationType) { case ConnectivityManager.CALLBACK_AVAILABLE: { - putParcelable(bundle, networkCapabilitiesRestrictedForCallerPermissions( - networkAgent.networkCapabilities, nri.mPid, nri.mUid)); + final NetworkCapabilities nc = + 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. @@ -6397,9 +6428,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( - networkAgent.networkCapabilities, nri.mPid, nri.mUid); - putParcelable(bundle, nc); + final NetworkCapabilities netCap = + networkCapabilitiesRestrictedForCallerPermissions( + networkAgent.networkCapabilities, nri.mPid, nri.mUid); + putParcelable( + bundle, + maybeSanitizeLocationInfoForCaller( + netCap, nri.mUid, nri.request.getRequestorPackageName())); break; } case ConnectivityManager.CALLBACK_IP_CHANGED: { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index f40e57fe46..b7e2ad9d49 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -1150,6 +1150,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 . @@ -2974,7 +2978,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 { @@ -6369,17 +6373,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() { - int callerUid = Process.myUid(); - final NetworkCapabilities originalNc = new NetworkCapabilities(); - originalNc.setOwnerUid(callerUid); + public void testMaybeSanitizeLocationInfoForCallerWithFineLocationAfterQ() throws Exception { + setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, + Manifest.permission.ACCESS_FINE_LOCATION); - final NetworkCapabilities newNc = - mService.networkCapabilitiesRestrictedForCallerPermissions( - originalNc, Process.myPid(), callerUid); + final int myUid = Process.myUid(); + assertEquals(myUid, getOwnerUidNetCapsForCallerPermission(myUid, myUid)); + } - 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 TestNetworkAgentWrapper establishVpn( @@ -6581,21 +6657,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);