From a9b761d2612b59b5e7c8e1adc86b173e47e5cc57 Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Tue, 12 May 2020 18:47:10 +0000 Subject: [PATCH 1/2] Set owner and administrator UIDs for test networks. This change sets the owner and administrator UIDs for test networks when their initial values match the UID for the app creating the test network. This ensures that apps registering test networks can only make themselves owners / administrators of the network. Bug: 153449964 Test: atest NetworkAgentTest Change-Id: I3a974700aa1d83cb285295ed1de0aa263e2e5b58 Merged-In: I3a974700aa1d83cb285295ed1de0aa263e2e5b58 (cherry picked from commit 35782280a2adceec96b8e03c217788afa05894a0) --- .../java/android/net/NetworkCapabilities.java | 13 ++++- .../android/server/ConnectivityService.java | 6 +- .../android/server/TestNetworkService.java | 57 +++++++++---------- .../server/connectivity/NetworkAgentInfo.java | 7 ++- .../server/ConnectivityServiceTest.java | 13 +++-- .../connectivity/LingerMonitorTest.java | 3 +- 6 files changed, 56 insertions(+), 43 deletions(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 52d6fdfbd5..9ded22fb70 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -677,16 +677,27 @@ public final class NetworkCapabilities implements Parcelable { * restrictions. * @hide */ - public void restrictCapabilitesForTestNetwork() { + public void restrictCapabilitesForTestNetwork(int creatorUid) { final long originalCapabilities = mNetworkCapabilities; final NetworkSpecifier originalSpecifier = mNetworkSpecifier; final int originalSignalStrength = mSignalStrength; + final int originalOwnerUid = getOwnerUid(); + final int[] originalAdministratorUids = getAdministratorUids(); clearAll(); // Reset the transports to only contain TRANSPORT_TEST. mTransportTypes = (1 << TRANSPORT_TEST); mNetworkCapabilities = originalCapabilities & TEST_NETWORKS_ALLOWED_CAPABILITIES; mNetworkSpecifier = originalSpecifier; mSignalStrength = originalSignalStrength; + + // Only retain the owner and administrator UIDs if they match the app registering the remote + // caller that registered the network. + if (originalOwnerUid == creatorUid) { + setOwnerUid(creatorUid); + } + if (ArrayUtils.contains(originalAdministratorUids, creatorUid)) { + setAdministratorUids(new int[] {creatorUid}); + } } /** diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2c63c6f8a6..8ca6951aef 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2740,7 +2740,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // the Messenger, but if this ever changes, not making a defensive copy // here will give attack vectors to clients using this code path. networkCapabilities = new NetworkCapabilities(networkCapabilities); - networkCapabilities.restrictCapabilitesForTestNetwork(); + networkCapabilities.restrictCapabilitesForTestNetwork(nai.creatorUid); } updateCapabilities(nai.getCurrentScore(), nai, networkCapabilities); break; @@ -5859,7 +5859,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // the call to mixInCapabilities below anyway, but sanitizing here means the NAI never // sees capabilities that may be malicious, which might prevent mistakes in the future. networkCapabilities = new NetworkCapabilities(networkCapabilities); - networkCapabilities.restrictCapabilitesForTestNetwork(); + networkCapabilities.restrictCapabilitesForTestNetwork(Binder.getCallingUid()); } else { enforceNetworkFactoryPermission(); } @@ -5872,7 +5872,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo nai = new NetworkAgentInfo(messenger, new AsyncChannel(), new Network(mNetIdManager.reserveNetId()), new NetworkInfo(networkInfo), lp, nc, currentScore, mContext, mTrackerHandler, new NetworkAgentConfig(networkAgentConfig), - this, mNetd, mDnsResolver, mNMS, providerId); + this, mNetd, mDnsResolver, mNMS, providerId, Binder.getCallingUid()); // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says. nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc)); diff --git a/services/core/java/com/android/server/TestNetworkService.java b/services/core/java/com/android/server/TestNetworkService.java index 0ea73460e1..d6bd5a1d7c 100644 --- a/services/core/java/com/android/server/TestNetworkService.java +++ b/services/core/java/com/android/server/TestNetworkService.java @@ -317,39 +317,34 @@ class TestNetworkService extends ITestNetworkManager.Stub { "Cannot create network for non ipsec, non-testtun interface"); } - // Setup needs to be done with NETWORK_STACK privileges. - int callingUid = Binder.getCallingUid(); - Binder.withCleanCallingIdentity( - () -> { - try { - mNMS.setInterfaceUp(iface); + try { + // This requires NETWORK_STACK privileges. + Binder.withCleanCallingIdentity(() -> mNMS.setInterfaceUp(iface)); - // Synchronize all accesses to mTestNetworkTracker to prevent the case - // where: - // 1. TestNetworkAgent successfully binds to death of binder - // 2. Before it is added to the mTestNetworkTracker, binder dies, - // binderDied() is called (on a different thread) - // 3. This thread is pre-empted, put() is called after remove() - synchronized (mTestNetworkTracker) { - TestNetworkAgent agent = - registerTestNetworkAgent( - mHandler.getLooper(), - mContext, - iface, - lp, - isMetered, - callingUid, - administratorUids, - binder); + // Synchronize all accesses to mTestNetworkTracker to prevent the case where: + // 1. TestNetworkAgent successfully binds to death of binder + // 2. Before it is added to the mTestNetworkTracker, binder dies, binderDied() is called + // (on a different thread) + // 3. This thread is pre-empted, put() is called after remove() + synchronized (mTestNetworkTracker) { + TestNetworkAgent agent = + registerTestNetworkAgent( + mHandler.getLooper(), + mContext, + iface, + lp, + isMetered, + Binder.getCallingUid(), + administratorUids, + binder); - mTestNetworkTracker.put(agent.getNetwork().netId, agent); - } - } catch (SocketException e) { - throw new UncheckedIOException(e); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); - } - }); + mTestNetworkTracker.put(agent.getNetwork().netId, agent); + } + } catch (SocketException e) { + throw new UncheckedIOException(e); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } } /** Teardown a test network */ diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 15628f03ba..37b2de1070 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -168,6 +168,9 @@ public class NetworkAgentInfo implements Comparable { // Obtained by ConnectivityService and merged into NetworkAgent-provided information. public CaptivePortalData captivePortalData; + // The UID of the remote entity that created this Network. + public final int creatorUid; + // Networks are lingered when they become unneeded as a result of their NetworkRequests being // satisfied by a higher-scoring network. so as to allow communication to wrap up before the // network is taken down. This usually only happens to the default network. Lingering ends with @@ -268,7 +271,8 @@ public class NetworkAgentInfo implements Comparable { public NetworkAgentInfo(Messenger messenger, AsyncChannel ac, Network net, NetworkInfo info, LinkProperties lp, NetworkCapabilities nc, int score, Context context, Handler handler, NetworkAgentConfig config, ConnectivityService connService, INetd netd, - IDnsResolver dnsResolver, INetworkManagementService nms, int factorySerialNumber) { + IDnsResolver dnsResolver, INetworkManagementService nms, int factorySerialNumber, + int creatorUid) { this.messenger = messenger; asyncChannel = ac; network = net; @@ -282,6 +286,7 @@ public class NetworkAgentInfo implements Comparable { mHandler = handler; networkAgentConfig = config; this.factorySerialNumber = factorySerialNumber; + this.creatorUid = creatorUid; } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index a992778fd4..d2b26d3bfd 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -75,6 +75,7 @@ import static android.net.NetworkPolicyManager.RULE_NONE; import static android.net.NetworkPolicyManager.RULE_REJECT_ALL; import static android.net.NetworkPolicyManager.RULE_REJECT_METERED; import static android.net.RouteInfo.RTN_UNREACHABLE; +import static android.os.Process.INVALID_UID; import static android.system.OsConstants.IPPROTO_TCP; import static com.android.server.ConnectivityServiceTestUtilsKt.transportToLegacyType; @@ -6945,7 +6946,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithoutUid = new NetworkAgentInfo( null, null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0); + mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); mServiceContext.setPermission( android.Manifest.permission.NETWORK_STACK, PERMISSION_GRANTED); @@ -6961,7 +6962,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithoutUid = new NetworkAgentInfo( null, null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0); + mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); @@ -6977,7 +6978,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithoutUid = new NetworkAgentInfo( null, null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0); + mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); @@ -6994,7 +6995,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithoutUid = new NetworkAgentInfo( null, null, network, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0); + mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); @@ -7028,7 +7029,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithUid = new NetworkAgentInfo( null, null, null, null, null, nc, 0, mServiceContext, null, null, - mService, null, null, null, 0); + mService, null, null, null, 0, INVALID_UID); setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); @@ -7050,7 +7051,7 @@ public class ConnectivityServiceTest { final NetworkAgentInfo naiWithUid = new NetworkAgentInfo( null, null, null, null, null, nc, 0, mServiceContext, null, null, - mService, null, null, null, 0); + mService, null, null, null, 0, INVALID_UID); setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); diff --git a/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java b/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java index 24a8717722..aafa18a532 100644 --- a/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java +++ b/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java @@ -38,6 +38,7 @@ import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkInfo; import android.net.NetworkProvider; +import android.os.Binder; import android.os.INetworkManagementService; import android.text.format.DateUtils; @@ -354,7 +355,7 @@ public class LingerMonitorTest { caps.addTransportType(transport); NetworkAgentInfo nai = new NetworkAgentInfo(null, null, new Network(netId), info, null, caps, 50, mCtx, null, null /* config */, mConnService, mNetd, mDnsResolver, mNMS, - NetworkProvider.ID_NONE); + NetworkProvider.ID_NONE, Binder.getCallingUid()); nai.everValidated = true; return nai; } From 9ddf8a5953d62eefff73303bba6db40c6ac04388 Mon Sep 17 00:00:00 2001 From: Automerger Merge Worker Date: Tue, 12 May 2020 18:48:04 +0000 Subject: [PATCH 2/2] Create TestApi for simulating a Data Stall on ConnectivityService. This change adds a TestApi for simulating a Data Stall to ConnectivityService. This allows for Data Stalls to be triggered without having to manipulate the signals used by NetworkMonitor . This also allows NetworkMonitor to update the ways it detects Data Stalls without affecting CTS tests for ConnectivityDiagnosticsManager. Bug: 148032944 Test: atest ConnectivityDiagnosticsManagerTest Change-Id: Icad439efa2ab4c872c21d3ee6ceaae8c5b49f18d Merged-In: Icad439efa2ab4c872c21d3ee6ceaae8c5b49f18d (cherry picked from commit b06463a002eb6215e9dda64e599eabd74cb56382) --- .../java/android/net/ConnectivityManager.java | 25 +++++++++++ .../android/net/IConnectivityManager.aidl | 5 +++ .../android/server/ConnectivityService.java | 44 +++++++++++++++---- 3 files changed, 65 insertions(+), 9 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index 7332ede0b9..36ffe50ef8 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -48,6 +48,7 @@ import android.os.Looper; import android.os.Message; import android.os.Messenger; import android.os.ParcelFileDescriptor; +import android.os.PersistableBundle; import android.os.Process; import android.os.RemoteException; import android.os.ResultReceiver; @@ -4693,4 +4694,28 @@ public class ConnectivityManager { Log.d(TAG, "StackLog:" + sb.toString()); } } + + /** + * Simulates a Data Stall for the specified Network. + * + *

The caller must be the owner of the specified Network. + * + * @param detectionMethod The detection method used to identify the Data Stall. + * @param timestampMillis The timestamp at which the stall 'occurred', in milliseconds. + * @param network The Network for which a Data Stall is being simluated. + * @param extras The PersistableBundle of extras included in the Data Stall notification. + * @throws SecurityException if the caller is not the owner of the given network. + * @hide + */ + @TestApi + @RequiresPermission(anyOf = {android.Manifest.permission.MANAGE_TEST_NETWORKS, + android.Manifest.permission.NETWORK_STACK}) + public void simulateDataStall(int detectionMethod, long timestampMillis, + @NonNull Network network, @NonNull PersistableBundle extras) { + try { + mService.simulateDataStall(detectionMethod, timestampMillis, network, extras); + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + } + } } diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index 14345608e9..69a47f24a0 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -18,6 +18,7 @@ package android.net; import android.app.PendingIntent; import android.net.ConnectionInfo; +import android.net.ConnectivityDiagnosticsManager; import android.net.IConnectivityDiagnosticsCallback; import android.net.LinkProperties; import android.net.Network; @@ -33,6 +34,7 @@ import android.os.Bundle; import android.os.IBinder; import android.os.Messenger; import android.os.ParcelFileDescriptor; +import android.os.PersistableBundle; import android.os.ResultReceiver; import com.android.internal.net.LegacyVpnInfo; @@ -227,4 +229,7 @@ interface IConnectivityManager void unregisterConnectivityDiagnosticsCallback(in IConnectivityDiagnosticsCallback callback); IBinder startOrGetTestNetworkService(); + + void simulateDataStall(int detectionMethod, long timestampMillis, in Network network, + in PersistableBundle extras); } diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 8ca6951aef..629c580afc 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3087,10 +3087,6 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public void notifyDataStallSuspected(DataStallReportParcelable p) { - final Message msg = mConnectivityDiagnosticsHandler.obtainMessage( - ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED, - p.detectionMethod, mNetId, p.timestampMillis); - final PersistableBundle extras = new PersistableBundle(); switch (p.detectionMethod) { case DETECTION_METHOD_DNS_EVENTS: @@ -3105,12 +3101,9 @@ public class ConnectivityService extends IConnectivityManager.Stub log("Unknown data stall detection method, ignoring: " + p.detectionMethod); return; } - msg.setData(new Bundle(extras)); - // NetworkStateTrackerHandler currently doesn't take any actions based on data - // stalls so send the message directly to ConnectivityDiagnosticsHandler and avoid - // the cost of going through two handlers. - mConnectivityDiagnosticsHandler.sendMessage(msg); + proxyDataStallToConnectivityDiagnosticsHandler( + p.detectionMethod, mNetId, p.timestampMillis, extras); } @Override @@ -3124,6 +3117,19 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private void proxyDataStallToConnectivityDiagnosticsHandler(int detectionMethod, int netId, + long timestampMillis, @NonNull PersistableBundle extras) { + final Message msg = mConnectivityDiagnosticsHandler.obtainMessage( + ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED, + detectionMethod, netId, timestampMillis); + msg.setData(new Bundle(extras)); + + // NetworkStateTrackerHandler currently doesn't take any actions based on data + // stalls so send the message directly to ConnectivityDiagnosticsHandler and avoid + // the cost of going through two handlers. + mConnectivityDiagnosticsHandler.sendMessage(msg); + } + private boolean networkRequiresPrivateDnsValidation(NetworkAgentInfo nai) { return isPrivateDnsValidationRequired(nai.networkCapabilities); } @@ -8151,4 +8157,24 @@ public class ConnectivityService extends IConnectivityManager.Stub 0, callback)); } + + @Override + public void simulateDataStall(int detectionMethod, long timestampMillis, + @NonNull Network network, @NonNull PersistableBundle extras) { + enforceAnyPermissionOf(android.Manifest.permission.MANAGE_TEST_NETWORKS, + android.Manifest.permission.NETWORK_STACK); + final NetworkCapabilities nc = getNetworkCapabilitiesInternal(network); + if (!nc.hasTransport(TRANSPORT_TEST)) { + throw new SecurityException("Data Stall simluation is only possible for test networks"); + } + + final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network); + if (nai == null || nai.creatorUid != Binder.getCallingUid()) { + throw new SecurityException("Data Stall simulation is only possible for network " + + "creators"); + } + + proxyDataStallToConnectivityDiagnosticsHandler( + detectionMethod, network.netId, timestampMillis, extras); + } }