From aa24fdeb5d0156b14e308e73239d61ba7247e998 Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Thu, 17 Dec 2020 14:53:09 -0800 Subject: [PATCH] ConnectivityService: Plumb attribution tag for location permission checks Not currently setting the atttribution tag for location permission checks. Plumb the attribution tag for all location permision checks (so that location access is correctly attributed to individual components within an app) Bug: 162602799 Test: atest android.net Test: atest com.android.server Change-Id: Iee95f05204f51a4f8cb1f36acfb60e8cdeb156f4 --- .../src/android/net/ConnectivityManager.java | 13 ++-- .../src/android/net/IConnectivityManager.aidl | 11 +-- .../android/server/ConnectivityService.java | 68 ++++++++++++------- .../android/net/ConnectivityManagerTest.java | 10 +-- .../server/ConnectivityServiceTest.java | 9 +-- 5 files changed, 71 insertions(+), 40 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index fef41522bd..d04a5bee51 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -1368,7 +1368,7 @@ public class ConnectivityManager { public NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser(int userId) { try { return mService.getDefaultNetworkCapabilitiesForUser( - userId, mContext.getOpPackageName()); + userId, mContext.getOpPackageName(), getAttributionTag()); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } @@ -1450,7 +1450,8 @@ public class ConnectivityManager { @Nullable public NetworkCapabilities getNetworkCapabilities(@Nullable Network network) { try { - return mService.getNetworkCapabilities(network, mContext.getOpPackageName()); + return mService.getNetworkCapabilities( + network, mContext.getOpPackageName(), getAttributionTag()); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } @@ -2142,7 +2143,7 @@ public class ConnectivityManager { */ // TODO: Remove method and replace with direct call once R code is pushed to AOSP private @Nullable String getAttributionTag() { - return null; + return mContext.getAttributionTag(); } /** @@ -3735,7 +3736,8 @@ public class ConnectivityManager { Binder binder = new Binder(); if (reqType == LISTEN) { request = mService.listenForNetwork( - need, messenger, binder, callingPackageName); + need, messenger, binder, callingPackageName, + getAttributionTag()); } else { request = mService.requestNetwork( need, reqType.ordinal(), messenger, timeoutMs, binder, legacyType, @@ -4180,7 +4182,8 @@ public class ConnectivityManager { checkPendingIntentNotNull(operation); try { mService.pendingListenForNetwork( - request.networkCapabilities, operation, mContext.getOpPackageName()); + request.networkCapabilities, operation, mContext.getOpPackageName(), + getAttributionTag()); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } catch (ServiceSpecificException e) { diff --git a/framework/src/android/net/IConnectivityManager.aidl b/framework/src/android/net/IConnectivityManager.aidl index e2672c480c..f909d13625 100644 --- a/framework/src/android/net/IConnectivityManager.aidl +++ b/framework/src/android/net/IConnectivityManager.aidl @@ -66,7 +66,7 @@ interface IConnectivityManager Network getNetworkForType(int networkType); Network[] getAllNetworks(); NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser( - int userId, String callingPackageName); + int userId, String callingPackageName, String callingAttributionTag); boolean isNetworkSupported(int networkType); @@ -75,7 +75,8 @@ interface IConnectivityManager LinkProperties getLinkPropertiesForType(int networkType); LinkProperties getLinkProperties(in Network network); - NetworkCapabilities getNetworkCapabilities(in Network network, String callingPackageName); + NetworkCapabilities getNetworkCapabilities(in Network network, String callingPackageName, + String callingAttributionTag); @UnsupportedAppUsage(maxTargetSdk = 30, trackingBug = 170729553) NetworkState[] getAllNetworkState(); @@ -176,10 +177,12 @@ interface IConnectivityManager void releasePendingNetworkRequest(in PendingIntent operation); NetworkRequest listenForNetwork(in NetworkCapabilities networkCapabilities, - in Messenger messenger, in IBinder binder, String callingPackageName); + in Messenger messenger, in IBinder binder, String callingPackageName, + String callingAttributionTag); void pendingListenForNetwork(in NetworkCapabilities networkCapabilities, - in PendingIntent operation, String callingPackageName); + in PendingIntent operation, String callingPackageName, + String callingAttributionTag); void releaseNetworkRequest(in NetworkRequest networkRequest); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 3933e379e8..eaeb20cb11 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1038,7 +1038,8 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetworkRanker = new NetworkRanker(); final NetworkRequest defaultInternetRequest = createDefaultInternetRequestForTransport( -1, NetworkRequest.Type.REQUEST); - mDefaultRequest = new NetworkRequestInfo(null, defaultInternetRequest, new Binder()); + mDefaultRequest = new NetworkRequestInfo(null, defaultInternetRequest, new Binder(), + null /* attributionTag */); mNetworkRequests.put(defaultInternetRequest, mDefaultRequest); mDefaultNetworkRequests.add(mDefaultRequest); mNetworkRequestInfoLogs.log("REGISTER " + mDefaultRequest); @@ -1308,7 +1309,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (enable) { handleRegisterNetworkRequest(new NetworkRequestInfo( - null, networkRequest, new Binder())); + null, networkRequest, new Binder(), null /* attributionTag */)); } else { handleReleaseNetworkRequest(networkRequest, Process.SYSTEM_UID, /* callOnUnavailable */ false); @@ -1643,7 +1644,7 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public NetworkCapabilities[] getDefaultNetworkCapabilitiesForUser( - int userId, String callingPackageName) { + int userId, String callingPackageName, @Nullable String callingAttributionTag) { // 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 @@ -1674,7 +1675,8 @@ public class ConnectivityService extends IConnectivityManager.Stub result.put( nai.network, createWithLocationInfoSanitizedIfNecessaryWhenParceled( - nc, mDeps.getCallingUid(), callingPackageName)); + nc, mDeps.getCallingUid(), callingPackageName, + callingAttributionTag)); } } @@ -1687,7 +1689,8 @@ public class ConnectivityService extends IConnectivityManager.Stub result.put( network, createWithLocationInfoSanitizedIfNecessaryWhenParceled( - nc, mDeps.getCallingUid(), callingPackageName)); + nc, mDeps.getCallingUid(), callingPackageName, + callingAttributionTag)); } } } @@ -1762,12 +1765,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } @Override - public NetworkCapabilities getNetworkCapabilities(Network network, String callingPackageName) { + public NetworkCapabilities getNetworkCapabilities(Network network, String callingPackageName, + @Nullable String callingAttributionTag) { mAppOpsManager.checkPackage(mDeps.getCallingUid(), callingPackageName); enforceAccessPermission(); return createWithLocationInfoSanitizedIfNecessaryWhenParceled( getNetworkCapabilitiesInternal(network), - mDeps.getCallingUid(), callingPackageName); + mDeps.getCallingUid(), callingPackageName, callingAttributionTag); } @VisibleForTesting @@ -1786,11 +1790,12 @@ public class ConnectivityService extends IConnectivityManager.Stub return newNc; } - private boolean hasLocationPermission(int callerUid, @NonNull String callerPkgName) { + private boolean hasLocationPermission(int callerUid, @NonNull String callerPkgName, + @Nullable String callingAttributionTag) { final long token = Binder.clearCallingIdentity(); try { return mLocationPermissionChecker.checkLocationPermission( - callerPkgName, null /* featureId */, callerUid, null /* message */); + callerPkgName, callingAttributionTag, callerUid, null /* message */); } finally { Binder.restoreCallingIdentity(token); } @@ -1799,7 +1804,8 @@ public class ConnectivityService extends IConnectivityManager.Stub @VisibleForTesting @Nullable NetworkCapabilities createWithLocationInfoSanitizedIfNecessaryWhenParceled( - @Nullable NetworkCapabilities nc, int callerUid, @NonNull String callerPkgName) { + @Nullable NetworkCapabilities nc, int callerUid, @NonNull String callerPkgName, + @Nullable String callingAttributionTag) { if (nc == null) { return null; } @@ -1808,7 +1814,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // Avoid doing location permission check if the transport info has no location sensitive // data. if (nc.getTransportInfo() != null && nc.getTransportInfo().hasLocationSensitiveFields()) { - hasLocationPermission = hasLocationPermission(callerUid, callerPkgName); + hasLocationPermission = + hasLocationPermission(callerUid, callerPkgName, callingAttributionTag); newNc = new NetworkCapabilities(nc, hasLocationPermission); } else { newNc = new NetworkCapabilities(nc, false /* parcelLocationSensitiveFields */); @@ -1825,7 +1832,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } if (hasLocationPermission == null) { // Location permission not checked yet, check now for masking owner UID. - hasLocationPermission = hasLocationPermission(callerUid, callerPkgName); + hasLocationPermission = + hasLocationPermission(callerUid, callerPkgName, callingAttributionTag); } // Reset owner uid if the app has no location permission. if (!hasLocationPermission) { @@ -5541,6 +5549,8 @@ public class ConnectivityService extends IConnectivityManager.Stub final int mPid; final int mUid; final Messenger messenger; + @Nullable + final String mCallingAttributionTag; /** * Get the list of UIDs this nri applies to. @@ -5554,7 +5564,8 @@ public class ConnectivityService extends IConnectivityManager.Stub return uids; } - NetworkRequestInfo(NetworkRequest r, PendingIntent pi) { + NetworkRequestInfo(NetworkRequest r, PendingIntent pi, + @Nullable String callingAttributionTag) { mRequests = initializeRequests(r); ensureAllNetworkRequestsHaveType(mRequests); mPendingIntent = pi; @@ -5563,9 +5574,11 @@ public class ConnectivityService extends IConnectivityManager.Stub mPid = getCallingPid(); mUid = mDeps.getCallingUid(); mNetworkRequestCounter.incrementCountOrThrow(mUid); + mCallingAttributionTag = callingAttributionTag; } - NetworkRequestInfo(Messenger m, NetworkRequest r, IBinder binder) { + NetworkRequestInfo(Messenger m, NetworkRequest r, IBinder binder, + @Nullable String callingAttributionTag) { super(); messenger = m; mRequests = initializeRequests(r); @@ -5575,6 +5588,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mUid = mDeps.getCallingUid(); mPendingIntent = null; mNetworkRequestCounter.incrementCountOrThrow(mUid); + mCallingAttributionTag = callingAttributionTag; try { mBinder.linkToDeath(this, 0); @@ -5584,7 +5598,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } NetworkRequestInfo(NetworkRequest r) { - this(r, null); + this(r, null /* pi */, null /* callingAttributionTag */); } // True if this NRI is being satisfied. It also accounts for if the nri has its satisifer @@ -5777,7 +5791,8 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkRequest networkRequest = new NetworkRequest(networkCapabilities, legacyType, nextNetworkRequestId(), reqType); - NetworkRequestInfo nri = new NetworkRequestInfo(messenger, networkRequest, binder); + NetworkRequestInfo nri = + new NetworkRequestInfo(messenger, networkRequest, binder, callingAttributionTag); if (DBG) log("requestNetwork for " + nri); mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_REQUEST, nri)); @@ -5866,7 +5881,8 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkRequest networkRequest = new NetworkRequest(networkCapabilities, TYPE_NONE, nextNetworkRequestId(), NetworkRequest.Type.REQUEST); - NetworkRequestInfo nri = new NetworkRequestInfo(networkRequest, operation); + NetworkRequestInfo nri = + new NetworkRequestInfo(networkRequest, operation, callingAttributionTag); if (DBG) log("pendingRequest for " + nri); mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_REQUEST_WITH_INTENT, nri)); @@ -5910,7 +5926,8 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public NetworkRequest listenForNetwork(NetworkCapabilities networkCapabilities, - Messenger messenger, IBinder binder, @NonNull String callingPackageName) { + Messenger messenger, IBinder binder, @NonNull String callingPackageName, + @Nullable String callingAttributionTag) { final int callingUid = mDeps.getCallingUid(); if (!hasWifiNetworkListenPermission(networkCapabilities)) { enforceAccessPermission(); @@ -5930,7 +5947,8 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkRequest networkRequest = new NetworkRequest(nc, TYPE_NONE, nextNetworkRequestId(), NetworkRequest.Type.LISTEN); - NetworkRequestInfo nri = new NetworkRequestInfo(messenger, networkRequest, binder); + NetworkRequestInfo nri = + new NetworkRequestInfo(messenger, networkRequest, binder, callingAttributionTag); if (VDBG) log("listenForNetwork for " + nri); mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_LISTENER, nri)); @@ -5939,7 +5957,8 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public void pendingListenForNetwork(NetworkCapabilities networkCapabilities, - PendingIntent operation, @NonNull String callingPackageName) { + PendingIntent operation, @NonNull String callingPackageName, + @Nullable String callingAttributionTag) { Objects.requireNonNull(operation, "PendingIntent cannot be null."); final int callingUid = mDeps.getCallingUid(); if (!hasWifiNetworkListenPermission(networkCapabilities)) { @@ -5953,7 +5972,8 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkRequest networkRequest = new NetworkRequest(nc, TYPE_NONE, nextNetworkRequestId(), NetworkRequest.Type.LISTEN); - NetworkRequestInfo nri = new NetworkRequestInfo(networkRequest, operation); + NetworkRequestInfo nri = + new NetworkRequestInfo(networkRequest, operation, callingAttributionTag); if (VDBG) log("pendingListenForNetwork for " + nri); mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_LISTENER, nri)); @@ -7168,7 +7188,8 @@ public class ConnectivityService extends IConnectivityManager.Stub putParcelable( bundle, createWithLocationInfoSanitizedIfNecessaryWhenParceled( - nc, nri.mUid, nrForCallback.getRequestorPackageName())); + nc, nri.mUid, nrForCallback.getRequestorPackageName(), + nri.mCallingAttributionTag)); putParcelable(bundle, linkPropertiesRestrictedForCallerPermissions( networkAgent.linkProperties, nri.mPid, nri.mUid)); // For this notification, arg1 contains the blocked status. @@ -7187,7 +7208,8 @@ public class ConnectivityService extends IConnectivityManager.Stub putParcelable( bundle, createWithLocationInfoSanitizedIfNecessaryWhenParceled( - netCap, nri.mUid, nrForCallback.getRequestorPackageName())); + netCap, nri.mUid, nrForCallback.getRequestorPackageName(), + nri.mCallingAttributionTag)); break; } case ConnectivityManager.CALLBACK_IP_CHANGED: { diff --git a/tests/net/java/android/net/ConnectivityManagerTest.java b/tests/net/java/android/net/ConnectivityManagerTest.java index c2fddf3d9e..fcfb4aa9b8 100644 --- a/tests/net/java/android/net/ConnectivityManagerTest.java +++ b/tests/net/java/android/net/ConnectivityManagerTest.java @@ -345,15 +345,17 @@ public class ConnectivityManagerTest { @Test public void testRequestType() throws Exception { final String testPkgName = "MyPackage"; + final String testAttributionTag = "MyTag"; final ConnectivityManager manager = new ConnectivityManager(mCtx, mService); when(mCtx.getOpPackageName()).thenReturn(testPkgName); + when(mCtx.getAttributionTag()).thenReturn(testAttributionTag); final NetworkRequest request = makeRequest(1); final NetworkCallback callback = new ConnectivityManager.NetworkCallback(); manager.requestNetwork(request, callback); verify(mService).requestNetwork(eq(request.networkCapabilities), eq(REQUEST.ordinal()), any(), anyInt(), any(), eq(TYPE_NONE), - eq(testPkgName), eq(null)); + eq(testPkgName), eq(testAttributionTag)); reset(mService); // Verify that register network callback does not calls requestNetwork at all. @@ -361,19 +363,19 @@ public class ConnectivityManagerTest { verify(mService, never()).requestNetwork(any(), anyInt(), any(), anyInt(), any(), anyInt(), any(), any()); verify(mService).listenForNetwork(eq(request.networkCapabilities), any(), any(), - eq(testPkgName)); + eq(testPkgName), eq(testAttributionTag)); reset(mService); manager.registerDefaultNetworkCallback(callback); verify(mService).requestNetwork(eq(null), eq(TRACK_DEFAULT.ordinal()), any(), anyInt(), any(), eq(TYPE_NONE), - eq(testPkgName), eq(null)); + eq(testPkgName), eq(testAttributionTag)); reset(mService); manager.requestBackgroundNetwork(request, null, callback); verify(mService).requestNetwork(eq(request.networkCapabilities), eq(BACKGROUND_REQUEST.ordinal()), any(), anyInt(), any(), eq(TYPE_NONE), - eq(testPkgName), eq(null)); + eq(testPkgName), eq(testAttributionTag)); reset(mService); } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index fe19f86bad..102ae928bf 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -6365,7 +6365,7 @@ public class ConnectivityServiceTest { private void assertDefaultNetworkCapabilities(int userId, NetworkAgentWrapper... networks) { final NetworkCapabilities[] defaultCaps = mService.getDefaultNetworkCapabilitiesForUser( - userId, "com.android.calling.package"); + userId, "com.android.calling.package", "com.test"); final String defaultCapsString = Arrays.toString(defaultCaps); assertEquals(defaultCapsString, defaultCaps.length, networks.length); final Set defaultCapsSet = new ArraySet<>(defaultCaps); @@ -8377,7 +8377,8 @@ public class ConnectivityServiceTest { when(mLocationManager.isLocationEnabledForUser(any())).thenReturn(locationToggle); if (op != null) { - when(mAppOpsManager.noteOp(eq(op), eq(Process.myUid()), eq(mContext.getPackageName()))) + when(mAppOpsManager.noteOp(eq(op), eq(Process.myUid()), + eq(mContext.getPackageName()), eq(getAttributionTag()), anyString())) .thenReturn(AppOpsManager.MODE_ALLOWED); } @@ -8390,7 +8391,7 @@ public class ConnectivityServiceTest { final NetworkCapabilities netCap = new NetworkCapabilities().setOwnerUid(ownerUid); return mService.createWithLocationInfoSanitizedIfNecessaryWhenParceled( - netCap, callerUid, mContext.getPackageName()).getOwnerUid(); + netCap, callerUid, mContext.getPackageName(), getAttributionTag()).getOwnerUid(); } private void verifyWifiInfoCopyNetCapsForCallerPermission( @@ -8400,7 +8401,7 @@ public class ConnectivityServiceTest { final NetworkCapabilities netCap = new NetworkCapabilities().setTransportInfo(wifiInfo); mService.createWithLocationInfoSanitizedIfNecessaryWhenParceled( - netCap, callerUid, mContext.getPackageName()); + netCap, callerUid, mContext.getPackageName(), getAttributionTag()); verify(wifiInfo).makeCopy(eq(shouldMakeCopyWithLocationSensitiveFieldsParcelable)); }