From 96bd9fe4dec806ba615691d091b2f696ecd798fe Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 8 Mar 2022 17:36:42 +0800 Subject: [PATCH] Check carrier privilege for CBS network requests synchronously Normally if an app calls requestNetwork with capabilities that it does not have permission to request, it gets a SecurityException, except if it requests NET_CAPABILITY_CBS, in which case the request will not throw but the app will get an onUnavailable callback. Make this codepath throw as well. This simplifies the code and makes the app-visible behaviour more consistent (and consistent with what happens in S and below). The reason the code was written this way is because the carrier privilege app should receive a callback if it loses permission. But onUnavailable is also not the best callback to send, since it is used very rarely and also releases the app's request. It seems better to leave the request registered and send onLost. Test: atest FrameworksNetTests Bug: 194332512 Change-Id: I5eaeb415a6654851246e38599a996fbd9366fde0 --- .../android/server/ConnectivityService.java | 57 +++---------------- .../server/ConnectivityServiceTest.java | 8 +-- 2 files changed, 12 insertions(+), 53 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index e796f5e068..10aa0bfe77 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -610,13 +610,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // Handle private DNS validation status updates. private static final int EVENT_PRIVATE_DNS_VALIDATION_UPDATE = 38; - /** - * used to remove a network request, either a listener or a real request and call unavailable - * arg1 = UID of caller - * obj = NetworkRequest - */ - private static final int EVENT_RELEASE_NETWORK_REQUEST_AND_CALL_UNAVAILABLE = 39; - /** * Event for NetworkMonitor/NetworkAgentInfo to inform ConnectivityService that the network has * been tested. @@ -4512,7 +4505,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean hasCarrierPrivilegeForNetworkCaps(final int callingUid, @NonNull final NetworkCapabilities caps) { - if (SdkLevel.isAtLeastT() && mCarrierPrivilegeAuthenticator != null) { + if (mCarrierPrivilegeAuthenticator != null) { return mCarrierPrivilegeAuthenticator.hasCarrierPrivilegeForNetworkCapabilities( callingUid, caps); } @@ -4542,7 +4535,6 @@ public class ConnectivityService extends IConnectivityManager.Stub private void handleRegisterNetworkRequests(@NonNull final Set nris) { ensureRunningOnConnectivityServiceThread(); - NetworkRequest requestToBeReleased = null; for (final NetworkRequestInfo nri : nris) { mNetworkRequestInfoLogs.log("REGISTER " + nri); checkNrisConsistency(nri); @@ -4557,13 +4549,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } } - if (req.hasCapability(NetworkCapabilities.NET_CAPABILITY_CBS)) { - if (!hasCarrierPrivilegeForNetworkCaps(nri.mUid, req.networkCapabilities) - && !checkConnectivityRestrictedNetworksPermission( - nri.mPid, nri.mUid)) { - requestToBeReleased = req; - } - } } // If this NRI has a satisfier already, it is replacing an older request that @@ -4575,11 +4560,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - if (requestToBeReleased != null) { - releaseNetworkRequestAndCallOnUnavailable(requestToBeReleased); - return; - } - if (mFlags.noRematchAllRequestsOnRegister()) { rematchNetworksAndRequests(nris); } else { @@ -5419,11 +5399,6 @@ public class ConnectivityService extends IConnectivityManager.Stub /* callOnUnavailable */ false); break; } - case EVENT_RELEASE_NETWORK_REQUEST_AND_CALL_UNAVAILABLE: { - handleReleaseNetworkRequest((NetworkRequest) msg.obj, msg.arg1, - /* callOnUnavailable */ true); - break; - } case EVENT_SET_ACCEPT_UNVALIDATED: { Network network = (Network) msg.obj; handleSetAcceptUnvalidated(network, toBool(msg.arg1), toBool(msg.arg2)); @@ -6642,7 +6617,7 @@ public class ConnectivityService extends IConnectivityManager.Stub case REQUEST: networkCapabilities = new NetworkCapabilities(networkCapabilities); enforceNetworkRequestPermissions(networkCapabilities, callingPackageName, - callingAttributionTag); + callingAttributionTag, callingUid); // TODO: this is incorrect. We mark the request as metered or not depending on // the state of the app when the request is filed, but we never change the // request if the app changes network state. http://b/29964605 @@ -6732,26 +6707,19 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void enforceNetworkRequestPermissions(NetworkCapabilities networkCapabilities, - String callingPackageName, String callingAttributionTag) { + String callingPackageName, String callingAttributionTag, final int callingUid) { if (networkCapabilities.hasCapability(NET_CAPABILITY_NOT_RESTRICTED) == false) { - if (!networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_CBS)) { - enforceConnectivityRestrictedNetworksPermission(true /* checkUidsAllowedList */); + // For T+ devices, callers with carrier privilege could request with CBS capabilities. + if (networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_CBS) + && hasCarrierPrivilegeForNetworkCaps(callingUid, networkCapabilities)) { + return; } + enforceConnectivityRestrictedNetworksPermission(true /* checkUidsAllowedList */); } else { enforceChangePermission(callingPackageName, callingAttributionTag); } } - private boolean checkConnectivityRestrictedNetworksPermission(int callerPid, int callerUid) { - if (checkAnyPermissionOf(callerPid, callerUid, - android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS) - || checkAnyPermissionOf(callerPid, callerUid, - android.Manifest.permission.CONNECTIVITY_INTERNAL)) { - return true; - } - return false; - } - @Override public boolean requestBandwidthUpdate(Network network) { enforceAccessPermission(); @@ -6810,7 +6778,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final int callingUid = mDeps.getCallingUid(); networkCapabilities = new NetworkCapabilities(networkCapabilities); enforceNetworkRequestPermissions(networkCapabilities, callingPackageName, - callingAttributionTag); + callingAttributionTag, callingUid); enforceMeteredApnPolicy(networkCapabilities); ensureRequestableCapabilities(networkCapabilities); ensureSufficientPermissionsForRequest(networkCapabilities, @@ -6933,13 +6901,6 @@ public class ConnectivityService extends IConnectivityManager.Stub EVENT_RELEASE_NETWORK_REQUEST, mDeps.getCallingUid(), 0, networkRequest)); } - private void releaseNetworkRequestAndCallOnUnavailable(NetworkRequest networkRequest) { - ensureNetworkRequestHasType(networkRequest); - mHandler.sendMessage(mHandler.obtainMessage( - EVENT_RELEASE_NETWORK_REQUEST_AND_CALL_UNAVAILABLE, mDeps.getCallingUid(), 0, - networkRequest)); - } - private void handleRegisterNetworkProvider(NetworkProviderInfo npi) { if (mNetworkProviderInfos.containsKey(npi.messenger)) { // Avoid creating duplicates. even if an app makes a direct AIDL call. diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 32ea9c30de..fb8df13471 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -5871,7 +5871,7 @@ public class ConnectivityServiceTest { } /** - * Validate the callback flow CBS request without carrier privilege. + * Validate the service throws if request with CBS but without carrier privilege. */ @Test public void testCBSRequestWithoutCarrierPrivilege() throws Exception { @@ -5880,10 +5880,8 @@ public class ConnectivityServiceTest { final TestNetworkCallback networkCallback = new TestNetworkCallback(); mServiceContext.setPermission(CONNECTIVITY_USE_RESTRICTED_NETWORKS, PERMISSION_DENIED); - // Now file the test request and expect it. - mCm.requestNetwork(nr, networkCallback); - networkCallback.expectCallback(CallbackEntry.UNAVAILABLE, (Network) null); - mCm.unregisterNetworkCallback(networkCallback); + // Now file the test request and expect the service throws. + assertThrows(SecurityException.class, () -> mCm.requestNetwork(nr, networkCallback)); } private static class TestKeepaliveCallback extends PacketKeepaliveCallback {