From bb4d45fb7049715a5b926fe148e7b4e0a7712062 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 18 Jan 2021 14:15:17 +0900 Subject: [PATCH] Enforce that NetworkAgentInfo fields are never null. These fields have been recently audited, confirmed never to be null, and annotated @NonNull. Ensure that they can never become null by throwing exceptions in the codepaths that set them. Also remove some null checks. Test: atest FrameworksNetTests Change-Id: I6ce5bb4d69a990f1c857c599b7e50e372352eb87 --- .../android/server/ConnectivityService.java | 10 +++--- .../server/connectivity/Nat464Xlat.java | 5 --- .../server/connectivity/NetworkAgentInfo.java | 15 ++++++-- .../server/ConnectivityServiceTest.java | 36 +++++++++---------- .../connectivity/LingerMonitorTest.java | 9 +++-- 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 53a8597b3f..b2fc1cf5dd 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1644,7 +1644,6 @@ public class ConnectivityService extends IConnectivityManager.Stub private NetworkCapabilities getNetworkCapabilitiesInternal(NetworkAgentInfo nai) { if (nai == null) return null; synchronized (nai) { - if (nai.networkCapabilities == null) return null; return networkCapabilitiesRestrictedForCallerPermissions( nai.networkCapabilities, Binder.getCallingPid(), mDeps.getCallingUid()); } @@ -2768,7 +2767,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } private boolean isLiveNetworkAgent(NetworkAgentInfo nai, int what) { - if (nai.network == null) return false; final NetworkAgentInfo officialNai = getNetworkAgentInfoForNetwork(nai.network); if (officialNai != null && officialNai.equals(nai)) return true; if (officialNai != null || VDBG) { @@ -6059,6 +6057,10 @@ public class ConnectivityService extends IConnectivityManager.Stub public Network registerNetworkAgent(INetworkAgent na, NetworkInfo networkInfo, LinkProperties linkProperties, NetworkCapabilities networkCapabilities, int currentScore, NetworkAgentConfig networkAgentConfig, int providerId) { + Objects.requireNonNull(networkInfo, "networkInfo must not be null"); + Objects.requireNonNull(linkProperties, "linkProperties must not be null"); + Objects.requireNonNull(networkCapabilities, "networkCapabilities must not be null"); + Objects.requireNonNull(networkAgentConfig, "networkAgentConfig must not be null"); if (networkCapabilities.hasTransport(TRANSPORT_TEST)) { enforceAnyPermissionOf(Manifest.permission.MANAGE_TEST_NETWORKS); } else { @@ -7574,10 +7576,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!networkAgent.everConnected && state == NetworkInfo.State.CONNECTED) { networkAgent.everConnected = true; - if (networkAgent.linkProperties == null) { - Log.wtf(TAG, networkAgent.toShortString() + " connected with null LinkProperties"); - } - // NetworkCapabilities need to be set before sending the private DNS config to // NetworkMonitor, otherwise NetworkMonitor cannot determine if validation is required. networkAgent.getAndSetNetworkCapabilities(networkAgent.networkCapabilities); diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index c1b1b6a2f2..952193b776 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -246,11 +246,6 @@ public class Nat464Xlat extends BaseNetworkObserver { return; } - if (mNetwork.linkProperties == null) { - Log.e(TAG, "startClat: Can't start clat with null LinkProperties"); - return; - } - String baseIface = mNetwork.linkProperties.getInterfaceName(); if (baseIface == null) { Log.e(TAG, "startClat: Can't start clat on null interface"); diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 8215e8879d..ba6cbcd3c7 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -329,6 +329,12 @@ public class NetworkAgentInfo implements Comparable { Handler handler, NetworkAgentConfig config, ConnectivityService connService, INetd netd, IDnsResolver dnsResolver, INetworkManagementService nms, int factorySerialNumber, int creatorUid) { + Objects.requireNonNull(net); + Objects.requireNonNull(info); + Objects.requireNonNull(lp); + Objects.requireNonNull(nc); + Objects.requireNonNull(context); + Objects.requireNonNull(config); networkAgent = na; network = net; networkInfo = info; @@ -536,19 +542,22 @@ public class NetworkAgentInfo implements Comparable { } @Override - public void sendNetworkCapabilities(NetworkCapabilities nc) { + public void sendNetworkCapabilities(@NonNull NetworkCapabilities nc) { + Objects.requireNonNull(nc); mHandler.obtainMessage(NetworkAgent.EVENT_NETWORK_CAPABILITIES_CHANGED, new Pair<>(NetworkAgentInfo.this, nc)).sendToTarget(); } @Override - public void sendLinkProperties(LinkProperties lp) { + public void sendLinkProperties(@NonNull LinkProperties lp) { + Objects.requireNonNull(lp); mHandler.obtainMessage(NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED, new Pair<>(NetworkAgentInfo.this, lp)).sendToTarget(); } @Override - public void sendNetworkInfo(NetworkInfo info) { + public void sendNetworkInfo(@NonNull NetworkInfo info) { + Objects.requireNonNull(info); mHandler.obtainMessage(NetworkAgent.EVENT_NETWORK_INFO_CHANGED, new Pair<>(NetworkAgentInfo.this, info)).sendToTarget(); } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index abf16f2a39..8f01745bca 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -8112,11 +8112,18 @@ public class ConnectivityServiceTest { assertTrue(mService.mConnectivityDiagnosticsCallbacks.containsKey(mIBinder)); } + public NetworkAgentInfo fakeMobileNai(NetworkCapabilities nc) { + final NetworkInfo info = new NetworkInfo(TYPE_MOBILE, TelephonyManager.NETWORK_TYPE_LTE, + ConnectivityManager.getNetworkTypeName(TYPE_MOBILE), + TelephonyManager.getNetworkTypeName(TelephonyManager.NETWORK_TYPE_LTE)); + return new NetworkAgentInfo(null, new Network(NET_ID), info, new LinkProperties(), + nc, 0, mServiceContext, null, new NetworkAgentConfig(), mService, null, null, null, + 0, INVALID_UID); + } + @Test public void testCheckConnectivityDiagnosticsPermissionsNetworkStack() throws Exception { - final NetworkAgentInfo naiWithoutUid = - new NetworkAgentInfo(null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); mServiceContext.setPermission( android.Manifest.permission.NETWORK_STACK, PERMISSION_GRANTED); @@ -8129,9 +8136,7 @@ public class ConnectivityServiceTest { @Test public void testCheckConnectivityDiagnosticsPermissionsWrongUidPackageName() throws Exception { - final NetworkAgentInfo naiWithoutUid = - new NetworkAgentInfo(null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); @@ -8144,9 +8149,7 @@ public class ConnectivityServiceTest { @Test public void testCheckConnectivityDiagnosticsPermissionsNoLocationPermission() throws Exception { - final NetworkAgentInfo naiWithoutUid = - new NetworkAgentInfo(null, null, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); @@ -8159,10 +8162,7 @@ public class ConnectivityServiceTest { @Test public void testCheckConnectivityDiagnosticsPermissionsActiveVpn() throws Exception { - final Network network = new Network(NET_ID); - final NetworkAgentInfo naiWithoutUid = - new NetworkAgentInfo(null, network, null, null, new NetworkCapabilities(), 0, - mServiceContext, null, null, mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); mMockVpn.establishForMyUid(); assertUidRangesUpdatedForMyUid(true); @@ -8172,7 +8172,7 @@ public class ConnectivityServiceTest { setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); - assertTrue(mService.setUnderlyingNetworksForVpn(new Network[] {network})); + assertTrue(mService.setUnderlyingNetworksForVpn(new Network[] {naiWithoutUid.network})); waitForIdle(); assertTrue( "Active VPN permission not applied", @@ -8193,9 +8193,7 @@ public class ConnectivityServiceTest { public void testCheckConnectivityDiagnosticsPermissionsNetworkAdministrator() throws Exception { final NetworkCapabilities nc = new NetworkCapabilities(); nc.setAdministratorUids(new int[] {Process.myUid()}); - final NetworkAgentInfo naiWithUid = - new NetworkAgentInfo(null, null, null, null, nc, 0, mServiceContext, null, null, - mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithUid = fakeMobileNai(nc); setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); @@ -8212,9 +8210,7 @@ public class ConnectivityServiceTest { final NetworkCapabilities nc = new NetworkCapabilities(); nc.setOwnerUid(Process.myUid()); nc.setAdministratorUids(new int[] {Process.myUid()}); - final NetworkAgentInfo naiWithUid = - new NetworkAgentInfo(null, null, null, null, nc, 0, mServiceContext, null, null, - mService, null, null, null, 0, INVALID_UID); + final NetworkAgentInfo naiWithUid = fakeMobileNai(nc); 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 96c56e32f1..4d151afecd 100644 --- a/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java +++ b/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java @@ -34,7 +34,9 @@ import android.content.res.Resources; import android.net.ConnectivityManager; import android.net.IDnsResolver; import android.net.INetd; +import android.net.LinkProperties; import android.net.Network; +import android.net.NetworkAgentConfig; import android.net.NetworkCapabilities; import android.net.NetworkInfo; import android.net.NetworkProvider; @@ -353,9 +355,10 @@ public class LingerMonitorTest { NetworkCapabilities caps = new NetworkCapabilities(); caps.addCapability(0); caps.addTransportType(transport); - NetworkAgentInfo nai = new NetworkAgentInfo(null, new Network(netId), info, null, - caps, 50, mCtx, null, null /* config */, mConnService, mNetd, mDnsResolver, mNMS, - NetworkProvider.ID_NONE, Binder.getCallingUid()); + NetworkAgentInfo nai = new NetworkAgentInfo(null, new Network(netId), info, + new LinkProperties(), caps, 50, mCtx, null, new NetworkAgentConfig() /* config */, + mConnService, mNetd, mDnsResolver, mNMS, NetworkProvider.ID_NONE, + Binder.getCallingUid()); nai.everValidated = true; return nai; }