From 41417ca64a417a88dd4d325ae01bfdf3d36029f3 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Sat, 26 Oct 2019 01:20:57 +0900 Subject: [PATCH 1/6] Add test coverage for strict mode private DNS. Support faking out the DNS lookups used by NetworkMonitor to resolve strict mode DNS, and add more test coverage. These tests were partly adapted from tests we have in Q but also contain new coverage. This is because in Q the interface between ConnectivityService and NetworkMonitor changed substantially, and it is impractical to backport NetworkMonitorTest. Bug: 122652057 Test: atest FrameworksNetTests Change-Id: I6497b7efa539267576d38d3036eef0af0df4e9cb Merged-In: Iaa78a7edcf23755c89d7b354edbc28d37d74d891 --- .../server/ConnectivityServiceTest.java | 117 +++++++++++++++++- 1 file changed, 111 insertions(+), 6 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index e6a889b1c2..d7a64dd38d 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -165,6 +165,7 @@ import org.mockito.MockitoAnnotations; import org.mockito.Spy; import java.net.InetAddress; +import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -455,6 +456,10 @@ public class ConnectivityServiceTest { mNetworkAgent.sendNetworkScore(mScore); } + public int getScore() { + return mScore; + } + public void explicitlySelected(boolean acceptUnvalidated) { mNetworkAgent.explicitlySelected(acceptUnvalidated); } @@ -869,6 +874,7 @@ public class ConnectivityServiceTest { // HTTP response code fed back to NetworkMonitor for Internet connectivity probe. public int gen204ProbeResult = 500; public String gen204ProbeRedirectUrl = null; + public volatile InetAddress[] dnsLookupResults = null; public WrappedNetworkMonitor(Context context, Handler handler, NetworkAgentInfo networkAgentInfo, NetworkRequest defaultRequest, @@ -883,6 +889,25 @@ public class ConnectivityServiceTest { if (!mIsCaptivePortalCheckEnabled) { return new CaptivePortalProbeResult(204); } return new CaptivePortalProbeResult(gen204ProbeResult, gen204ProbeRedirectUrl, null); } + + private InetAddress[] fakeDnsLookup() throws UnknownHostException { + if (dnsLookupResults == null) { + throw new UnknownHostException(); + } + return dnsLookupResults; + } + + @Override + protected InetAddress[] getAllByName(Network network, String hostname) + throws UnknownHostException { + return fakeDnsLookup(); + } + + @Override + protected InetAddress[] resolveAllLocally(Network network, String hostname, int flags) + throws UnknownHostException { + return fakeDnsLookup(); + } } private class WrappedMultinetworkPolicyTracker extends MultinetworkPolicyTracker { @@ -4021,7 +4046,7 @@ public class ConnectivityServiceTest { cellLp.addDnsServer(InetAddress.getByName("192.0.2.1")); mCellNetworkAgent.sendLinkProperties(cellLp); - mCellNetworkAgent.connect(false); + mCellNetworkAgent.connect(true); waitForIdle(); verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( anyInt(), mStringArrayCaptor.capture(), any(), any(), @@ -4039,9 +4064,10 @@ public class ConnectivityServiceTest { mCellNetworkAgent); CallbackInfo cbi = cellNetworkCallback.expectCallback( CallbackState.LINK_PROPERTIES, mCellNetworkAgent); - cellNetworkCallback.assertNoCallback(); assertFalse(((LinkProperties)cbi.arg).isPrivateDnsActive()); assertNull(((LinkProperties)cbi.arg).getPrivateDnsServerName()); + cellNetworkCallback.expectCallback(CallbackState.NETWORK_CAPABILITIES, mCellNetworkAgent); + cellNetworkCallback.assertNoCallback(); setPrivateDnsSettings(PRIVATE_DNS_MODE_OFF, "ignored.example.com"); verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork( @@ -4066,14 +4092,45 @@ public class ConnectivityServiceTest { reset(mNetworkManagementService); cellNetworkCallback.assertNoCallback(); + // Strict mode. + mCellNetworkAgent.getWrappedNetworkMonitor().dnsLookupResults = new InetAddress[] { + InetAddress.getByName("2001:db8::66"), + InetAddress.getByName("192.0.2.44") + }; setPrivateDnsSettings(PRIVATE_DNS_MODE_PROVIDER_HOSTNAME, "strict.example.com"); - // Can't test dns configuration for strict mode without properly mocking - // out the DNS lookups, but can test that LinkProperties is updated. + + // Expect a callback saying that private DNS is now in strict mode. cbi = cellNetworkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); + LinkProperties lp = (LinkProperties) cbi.arg; + assertTrue(lp.isPrivateDnsActive()); + assertEquals("strict.example.com", lp.getPrivateDnsServerName()); cellNetworkCallback.assertNoCallback(); - assertTrue(((LinkProperties)cbi.arg).isPrivateDnsActive()); - assertEquals("strict.example.com", ((LinkProperties)cbi.arg).getPrivateDnsServerName()); + + // When the validation callback arrives, LinkProperties are updated. + // We need to wait for this callback because the test thread races with the NetworkMonitor + // thread, and if the test thread wins the race, then the times(2) verify call below will + // fail. + mService.mNetdEventCallback.onPrivateDnsValidationEvent( + mCellNetworkAgent.getNetwork().netId, "2001:db8::66", "strict.example.com", true); + cbi = cellNetworkCallback.expectCallback(CallbackState.LINK_PROPERTIES, + mCellNetworkAgent); + lp = (LinkProperties) cbi.arg; + assertTrue(lp.isPrivateDnsActive()); + assertEquals(1, lp.getValidatedPrivateDnsServers().size()); + + // setDnsConfigurationForNetwork is called twice: once when private DNS is set to strict + // mode and once when the hostname resolves. + verify(mNetworkManagementService, times(2)).setDnsConfigurationForNetwork( + anyInt(), mStringArrayCaptor.capture(), any(), any(), + eq("strict.example.com"), tlsServers.capture()); + assertEquals(2, mStringArrayCaptor.getValue().length); + assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), + new String[]{"2001:db8::1", "192.0.2.1"})); + assertEquals(2, tlsServers.getValue().length); + assertTrue(ArrayUtils.containsAll(tlsServers.getValue(), + new String[]{"2001:db8::66", "192.0.2.44"})); + reset(mNetworkManagementService); // Send the same LinkProperties and expect getting the same result including private dns. // b/118518971 @@ -4395,6 +4452,54 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(defaultCallback); } + @Test + public void testVpnUnvalidated() throws Exception { + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(callback); + + // Enable private DNS. + setPrivateDnsSettings(PRIVATE_DNS_MODE_PROVIDER_HOSTNAME, "strict.example.com"); + + // Bring up Ethernet. + mEthernetNetworkAgent = new MockNetworkAgent(TRANSPORT_ETHERNET); + mEthernetNetworkAgent.getWrappedNetworkMonitor().dnsLookupResults = + new InetAddress[]{ InetAddress.getByName("2001:db8::1") }; + mEthernetNetworkAgent.connect(true); + callback.expectAvailableThenValidatedCallbacks(mEthernetNetworkAgent); + callback.assertNoCallback(); + + // Bring up a VPN that has the INTERNET capability but does not provide Internet access. + final int uid = Process.myUid(); + final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + vpnNetworkAgent.getWrappedNetworkMonitor().gen204ProbeResult = 500; + vpnNetworkAgent.getWrappedNetworkMonitor().dnsLookupResults = null; + + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); + vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); + mMockVpn.connect(); + + // The VPN validates and becomes the default network for our app. + callback.expectAvailableCallbacksValidated(vpnNetworkAgent); + // TODO: this looks like a spurious callback. + callback.expectCallback(CallbackState.NETWORK_CAPABILITIES, vpnNetworkAgent); + callback.assertNoCallback(); + + assertTrue(vpnNetworkAgent.getScore() > mEthernetNetworkAgent.getScore()); + assertEquals(ConnectivityConstants.VPN_DEFAULT_SCORE, vpnNetworkAgent.getScore()); + assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + + NetworkCapabilities nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + assertTrue(nc.hasCapability(NET_CAPABILITY_VALIDATED)); + assertTrue(nc.hasCapability(NET_CAPABILITY_INTERNET)); + + vpnNetworkAgent.disconnect(); + callback.expectCallback(CallbackState.LOST, vpnNetworkAgent); + callback.expectAvailableCallbacksValidated(mEthernetNetworkAgent); + } + @Test public void testVpnSetUnderlyingNetworks() { final int uid = Process.myUid(); From 837030aaa7cf532a817bc6e606df1cd161b20d21 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 10 May 2019 04:33:43 -0700 Subject: [PATCH 2/6] Support strict mode private DNS on VPNs that provide Internet. Currently, strict mode private DNS does not work on VPNs because NetworkMonitor does not validate VPNs. When a VPN connects, it immediately transitions to ValidatedState, skipping private DNS hostname resolution. This change makes NetworkMonitor perform private DNS hostname resolution and evaluation even on VPNs. In order to ensure that the system always immediately switches to the VPN as soon as it connects, remove the unvalidated penalty for VPN networks. This ensures that the VPN score is always 101 and the VPN always outscores other networks as soon as it connects. Previously, it would only outscore other networks when no-op validation completed. Backport of 1a26465d925c639579e6c5f52a29bbda2d97b18e. Bug: 122652057 Test: atest FrameworksNetTests Test: manually ran a VPN with private DNS in strict mode Test: atest android.net.cts.ConnectivityManagerTest com.android.cts.net.HostsideVpnTests Change-Id: Iaa78a7edcf23755c89d7b354edbc28d37d74d891 Merged-In: Iaa78a7edcf23755c89d7b354edbc28d37d74d891 --- .../android/server/ConnectivityService.java | 11 +++----- .../server/connectivity/NetworkAgentInfo.java | 4 +-- .../server/ConnectivityServiceTest.java | 28 +++++++++++++++---- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 17cece1a22..b71fabae7f 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2349,9 +2349,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private boolean networkRequiresValidation(NetworkAgentInfo nai) { - return NetworkMonitor.isValidationRequired( - mDefaultRequest.networkCapabilities, nai.networkCapabilities); + private boolean networkRequiresPrivateDnsValidation(NetworkAgentInfo nai) { + return nai.networkMonitor.isPrivateDnsValidationRequired(); } private void handleFreshlyValidatedNetwork(NetworkAgentInfo nai) { @@ -2369,16 +2368,14 @@ public class ConnectivityService extends IConnectivityManager.Stub for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { handlePerNetworkPrivateDnsConfig(nai, cfg); - if (networkRequiresValidation(nai)) { + if (networkRequiresPrivateDnsValidation(nai)) { handleUpdateLinkProperties(nai, new LinkProperties(nai.linkProperties)); } } } private void handlePerNetworkPrivateDnsConfig(NetworkAgentInfo nai, PrivateDnsConfig cfg) { - // Private DNS only ever applies to networks that might provide - // Internet access and therefore also require validation. - if (!networkRequiresValidation(nai)) return; + if (!networkRequiresPrivateDnsValidation(nai)) return; // Notify the NetworkMonitor thread in case it needs to cancel or // schedule DNS resolutions. If a DNS resolution is required the diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 505480ea53..5ee572ecb7 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -432,11 +432,11 @@ public class NetworkAgentInfo implements Comparable { // down an explicitly selected network before the user gets a chance to prefer it when // a higher-scoring network (e.g., Ethernet) is available. if (networkMisc.explicitlySelected && (networkMisc.acceptUnvalidated || pretendValidated)) { - return ConnectivityConstants.MAXIMUM_NETWORK_SCORE; + return ConnectivityConstants.EXPLICITLY_SELECTED_NETWORK_SCORE; } int score = currentScore; - if (!lastValidated && !pretendValidated && !ignoreWifiUnvalidationPenalty()) { + if (!lastValidated && !pretendValidated && !ignoreWifiUnvalidationPenalty() && !isVPN()) { score -= ConnectivityConstants.UNVALIDATED_SCORE_PENALTY; } if (score < 0) score = 0; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index d7a64dd38d..6ceed53f4e 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -26,6 +26,7 @@ import static android.net.ConnectivityManager.TYPE_MOBILE; import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA; import static android.net.ConnectivityManager.TYPE_MOBILE_MMS; import static android.net.ConnectivityManager.TYPE_NONE; +import static android.net.ConnectivityManager.TYPE_VPN; import static android.net.ConnectivityManager.TYPE_WIFI; import static android.net.NetworkCapabilities.NET_CAPABILITY_CAPTIVE_PORTAL; import static android.net.NetworkCapabilities.NET_CAPABILITY_CBS; @@ -388,7 +389,7 @@ public class ConnectivityServiceTest { MockNetworkAgent(int transport, LinkProperties linkProperties) { final int type = transportToLegacyType(transport); - final String typeName = ConnectivityManager.getNetworkTypeName(transport); + final String typeName = ConnectivityManager.getNetworkTypeName(type); mNetworkInfo = new NetworkInfo(type, 0, typeName, "Mock"); mNetworkCapabilities = new NetworkCapabilities(); mNetworkCapabilities.addTransportType(transport); @@ -1111,6 +1112,8 @@ public class ConnectivityServiceTest { return TYPE_WIFI; case TRANSPORT_CELLULAR: return TYPE_MOBILE; + case TRANSPORT_VPN: + return TYPE_VPN; default: return TYPE_NONE; } @@ -4468,7 +4471,7 @@ public class ConnectivityServiceTest { callback.expectAvailableThenValidatedCallbacks(mEthernetNetworkAgent); callback.assertNoCallback(); - // Bring up a VPN that has the INTERNET capability but does not provide Internet access. + // Bring up a VPN that has the INTERNET capability but does not validate. final int uid = Process.myUid(); final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); vpnNetworkAgent.getWrappedNetworkMonitor().gen204ProbeResult = 500; @@ -4481,8 +4484,8 @@ public class ConnectivityServiceTest { vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); mMockVpn.connect(); - // The VPN validates and becomes the default network for our app. - callback.expectAvailableCallbacksValidated(vpnNetworkAgent); + // Even though the VPN is unvalidated, it becomes the default network for our app. + callback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); // TODO: this looks like a spurious callback. callback.expectCallback(CallbackState.NETWORK_CAPABILITIES, vpnNetworkAgent); callback.assertNoCallback(); @@ -4492,9 +4495,24 @@ public class ConnectivityServiceTest { assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); NetworkCapabilities nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); - assertTrue(nc.hasCapability(NET_CAPABILITY_VALIDATED)); + assertFalse(nc.hasCapability(NET_CAPABILITY_VALIDATED)); assertTrue(nc.hasCapability(NET_CAPABILITY_INTERNET)); + assertFalse(vpnNetworkAgent.getWrappedNetworkMonitor().isValidationRequired()); + assertTrue(vpnNetworkAgent.getWrappedNetworkMonitor().isPrivateDnsValidationRequired()); + + // Pretend that the strict mode private DNS hostname now resolves. Even though the + // connectivity probe still returns 500, the network validates because the connectivity + // probe is not used on VPNs. + vpnNetworkAgent.getWrappedNetworkMonitor().dnsLookupResults = + new InetAddress[]{ InetAddress.getByName("2001:db8::1") }; + mCm.reportNetworkConnectivity(vpnNetworkAgent.getNetwork(), true); + + // Expect to see the validated capability, but no other changes, because the VPN is already + // the default network for the app. + callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, vpnNetworkAgent); + callback.assertNoCallback(); + vpnNetworkAgent.disconnect(); callback.expectCallback(CallbackState.LOST, vpnNetworkAgent); callback.expectAvailableCallbacksValidated(mEthernetNetworkAgent); From ff7f2568b6cb62f9b4c04915f983e62df079fc39 Mon Sep 17 00:00:00 2001 From: paulhu Date: Mon, 16 Dec 2019 18:24:05 +0800 Subject: [PATCH 3/6] Fix security problem on PermissionMonitor#hasPermission PermissionMonitor#hasPermission only checks permssions that app requested but it doesn't check whether the permission can be granted to this app. If requested permission doens't be granted to app, this method still returns that app has this permission. Then PermissionMonitor will pass this info to netd that means this app still can use network even restricted network without granted privileged permission like CONNECTIVITY_INTERNAL or CONNECTIVITY_USE_RESTRICTED_NETWORKS. Bug: 144679405 Test: Build, flash, manual test Change-Id: I5eba4909e4c2e1d9f275f66be90ac36466b93e90 Merged-In: I8a1575dedd6e3b7a8b60ee2ffd475d790aec55c4 Merged-In: Iae9c273af822b18c2e6fce04848a86f8dea6410a --- .../server/connectivity/PermissionMonitor.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/PermissionMonitor.java b/services/core/java/com/android/server/connectivity/PermissionMonitor.java index e084ff827c..57dbfd1e15 100644 --- a/services/core/java/com/android/server/connectivity/PermissionMonitor.java +++ b/services/core/java/com/android/server/connectivity/PermissionMonitor.java @@ -21,6 +21,7 @@ import static android.Manifest.permission.CONNECTIVITY_INTERNAL; import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; import static android.content.pm.ApplicationInfo.FLAG_SYSTEM; import static android.content.pm.ApplicationInfo.FLAG_UPDATED_SYSTEM_APP; +import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import android.content.BroadcastReceiver; @@ -39,6 +40,8 @@ import android.os.UserManager; import android.text.TextUtils; import android.util.Log; +import com.android.internal.util.ArrayUtils; + import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; @@ -150,15 +153,13 @@ public class PermissionMonitor { update(mUsers, mApps, true); } - private boolean hasPermission(PackageInfo app, String permission) { - if (app.requestedPermissions != null) { - for (String p : app.requestedPermissions) { - if (permission.equals(p)) { - return true; - } - } + private boolean hasPermission(final PackageInfo app, final String permission) { + if (app.requestedPermissions == null || app.requestedPermissionsFlags == null) { + return false; } - return false; + final int index = ArrayUtils.indexOf(app.requestedPermissions, permission); + if (index < 0 || index >= app.requestedPermissionsFlags.length) return false; + return (app.requestedPermissionsFlags[index] & REQUESTED_PERMISSION_GRANTED) != 0; } private boolean hasNetworkPermission(PackageInfo app) { From 4c36a00d581651f50814ef4aa2b57b759b6b217e Mon Sep 17 00:00:00 2001 From: paulhu Date: Mon, 16 Dec 2019 18:24:05 +0800 Subject: [PATCH 4/6] Fix security problem on PermissionMonitor#hasPermission PermissionMonitor#hasPermission only checks permssions that app requested but it doesn't check whether the permission can be granted to this app. If requested permission doens't be granted to app, this method still returns that app has this permission. Then PermissionMonitor will pass this info to netd that means this app still can use network even restricted network without granted privileged permission like CONNECTIVITY_INTERNAL or CONNECTIVITY_USE_RESTRICTED_NETWORKS. Bug: 144679405 Test: Build, flash, manual test Change-Id: Iae9c273af822b18c2e6fce04848a86f8dea6410a Merged-In: I8a1575dedd6e3b7a8b60ee2ffd475d790aec55c4 Merged-In: I2da730feda4d7ebed1f158b073167bb3964b3e7d --- .../server/connectivity/PermissionMonitor.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/PermissionMonitor.java b/services/core/java/com/android/server/connectivity/PermissionMonitor.java index e471c7d84b..5722f5a72a 100644 --- a/services/core/java/com/android/server/connectivity/PermissionMonitor.java +++ b/services/core/java/com/android/server/connectivity/PermissionMonitor.java @@ -22,6 +22,7 @@ import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; import static android.Manifest.permission.NETWORK_STACK; import static android.content.pm.ApplicationInfo.FLAG_SYSTEM; import static android.content.pm.ApplicationInfo.FLAG_UPDATED_SYSTEM_APP; +import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import android.content.BroadcastReceiver; @@ -42,6 +43,7 @@ import android.text.TextUtils; import android.util.Log; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.util.ArrayUtils; import java.util.ArrayList; import java.util.HashMap; @@ -161,15 +163,13 @@ public class PermissionMonitor { } @VisibleForTesting - boolean hasPermission(PackageInfo app, String permission) { - if (app.requestedPermissions != null) { - for (String p : app.requestedPermissions) { - if (permission.equals(p)) { - return true; - } - } + boolean hasPermission(final PackageInfo app, final String permission) { + if (app.requestedPermissions == null || app.requestedPermissionsFlags == null) { + return false; } - return false; + final int index = ArrayUtils.indexOf(app.requestedPermissions, permission); + if (index < 0 || index >= app.requestedPermissionsFlags.length) return false; + return (app.requestedPermissionsFlags[index] & REQUESTED_PERMISSION_GRANTED) != 0; } private boolean hasNetworkPermission(PackageInfo app) { From ff042fd0d9a7d66ff8cd15a8fcaa0527584186df Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Mon, 25 Nov 2019 10:35:55 -0800 Subject: [PATCH 5/6] Fix PermissionMonitor issues PermissionMonitor#hasPermission only checks permssions that app requested but it doesn't check whether the permission can be granted to this app. If requested permission doens't be granted to app, this method still returns that app has this permission. Then PermissionMonitor will pass this info to netd that means this app still can use network even restricted network without granted privileged permission like CONNECTIVITY_INTERNAL or CONNECTIVITY_USE_RESTRICTED_NETWORKS. PermissionMonitor#hasUseBackgroundNetworksPermission only uses the first package name of the uid for checking permission. This is incorrect since each package declared different permissions. So using the mApps which already checked both network and using restricted network permissions. If uid is in the mApps list that means uid has one of permission at least. Bug: 144679405 Test: Build, flash, manual test atest FrameworksNetTests Change-Id: I2da730feda4d7ebed1f158b073167bb3964b3e7d Merged-In: I8b03c9e23ffc9ff46264d6307fb841a7eda76a76 Merged-In: Ib08a940a6e5d3365c392ab7174d8484c197e0947 (cherry picked from commit 2dd1f14da181caeb6f77304f447fa1c8523657bd) --- .../connectivity/PermissionMonitor.java | 53 ++--- .../server/ConnectivityServiceTest.java | 24 +- .../connectivity/PermissionMonitorTest.java | 217 ++++++++++++------ 3 files changed, 185 insertions(+), 109 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/PermissionMonitor.java b/services/core/java/com/android/server/connectivity/PermissionMonitor.java index fbe2589bea..c70012b5e7 100644 --- a/services/core/java/com/android/server/connectivity/PermissionMonitor.java +++ b/services/core/java/com/android/server/connectivity/PermissionMonitor.java @@ -224,22 +224,22 @@ public class PermissionMonitor { } @VisibleForTesting - boolean hasPermission(PackageInfo app, String permission) { - if (app.requestedPermissions != null) { - for (String p : app.requestedPermissions) { - if (permission.equals(p)) { - return true; - } - } + boolean hasPermission(@NonNull final PackageInfo app, @NonNull final String permission) { + if (app.requestedPermissions == null || app.requestedPermissionsFlags == null) { + return false; } - return false; + final int index = ArrayUtils.indexOf(app.requestedPermissions, permission); + if (index < 0 || index >= app.requestedPermissionsFlags.length) return false; + return (app.requestedPermissionsFlags[index] & REQUESTED_PERMISSION_GRANTED) != 0; } - private boolean hasNetworkPermission(PackageInfo app) { + @VisibleForTesting + boolean hasNetworkPermission(@NonNull final PackageInfo app) { return hasPermission(app, CHANGE_NETWORK_STATE); } - private boolean hasRestrictedNetworkPermission(PackageInfo app) { + @VisibleForTesting + boolean hasRestrictedNetworkPermission(@NonNull final PackageInfo app) { // TODO : remove this check in the future(b/31479477). All apps should just // request the appropriate permission for their use case since android Q. if (app.applicationInfo != null) { @@ -255,33 +255,18 @@ public class PermissionMonitor { } } return hasPermission(app, CONNECTIVITY_INTERNAL) + || hasPermission(app, NETWORK_STACK) || hasPermission(app, CONNECTIVITY_USE_RESTRICTED_NETWORKS); } - private boolean hasUseBackgroundNetworksPermission(PackageInfo app) { - // This function defines what it means to hold the permission to use - // background networks. - return hasPermission(app, CHANGE_NETWORK_STATE) - || hasPermission(app, NETWORK_STACK) - || hasRestrictedNetworkPermission(app); - } - - public boolean hasUseBackgroundNetworksPermission(int uid) { - final String[] names = mPackageManager.getPackagesForUid(uid); - if (null == names || names.length == 0) return false; - try { - // Only using the first package name. There may be multiple names if multiple - // apps share the same UID, but in that case they also share permissions so - // querying with any of the names will return the same results. - int userId = UserHandle.getUserId(uid); - final PackageInfo app = mPackageManager.getPackageInfoAsUser( - names[0], GET_PERMISSIONS, userId); - return hasUseBackgroundNetworksPermission(app); - } catch (NameNotFoundException e) { - // App not found. - loge("NameNotFoundException " + names[0], e); - return false; - } + /** Returns whether the given uid has using background network permission. */ + public synchronized boolean hasUseBackgroundNetworksPermission(final int uid) { + // Apps with any of the CHANGE_NETWORK_STATE, NETWORK_STACK, CONNECTIVITY_INTERNAL or + // CONNECTIVITY_USE_RESTRICTED_NETWORKS permission has the permission to use background + // networks. mApps contains the result of checks for both hasNetworkPermission and + // hasRestrictedNetworkPermission. If uid is in the mApps list that means uid has one of + // permissions at least. + return mApps.containsKey(uid); } private int[] toIntArray(Collection list) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 73ee7f544b..e313e28fbf 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -16,6 +16,9 @@ package com.android.server; +import static android.Manifest.permission.CHANGE_NETWORK_STATE; +import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; +import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import static android.content.pm.PackageManager.MATCH_ANY_USER; import static android.net.ConnectivityManager.ACTION_CAPTIVE_PORTAL_SIGN_IN; @@ -2332,9 +2335,17 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(trackDefaultCallback); } + private void grantUsingBackgroundNetworksPermissionForUid(final int uid) throws Exception { + final String testPackageName = mContext.getPackageName(); + when(mPackageManager.getPackageInfo(eq(testPackageName), eq(GET_PERMISSIONS))) + .thenReturn(buildPackageInfo(true, uid)); + mService.mPermissionMonitor.onPackageAdded(testPackageName, uid); + } + @Test - public void testNetworkGoesIntoBackgroundAfterLinger() { + public void testNetworkGoesIntoBackgroundAfterLinger() throws Exception { setAlwaysOnNetworks(true); + grantUsingBackgroundNetworksPermissionForUid(Binder.getCallingUid()); NetworkRequest request = new NetworkRequest.Builder() .clearCapabilities() .build(); @@ -3415,6 +3426,7 @@ public class ConnectivityServiceTest { // Create a background request. We can't do this ourselves because ConnectivityService // doesn't have an API for it. So just turn on mobile data always on. setAlwaysOnNetworks(true); + grantUsingBackgroundNetworksPermissionForUid(Binder.getCallingUid()); final NetworkRequest request = new NetworkRequest.Builder().build(); final NetworkRequest fgRequest = new NetworkRequest.Builder() .addCapability(NET_CAPABILITY_FOREGROUND).build(); @@ -3581,6 +3593,7 @@ public class ConnectivityServiceTest { @Test public void testMobileDataAlwaysOn() throws Exception { + grantUsingBackgroundNetworksPermissionForUid(Binder.getCallingUid()); final TestNetworkCallback cellNetworkCallback = new TestNetworkCallback(); final NetworkRequest cellRequest = new NetworkRequest.Builder() .addTransportType(TRANSPORT_CELLULAR).build(); @@ -6528,7 +6541,14 @@ public class ConnectivityServiceTest { private static PackageInfo buildPackageInfo(boolean hasSystemPermission, int uid) { final PackageInfo packageInfo = new PackageInfo(); - packageInfo.requestedPermissions = new String[0]; + if (hasSystemPermission) { + packageInfo.requestedPermissions = new String[] { + CHANGE_NETWORK_STATE, CONNECTIVITY_USE_RESTRICTED_NETWORKS }; + packageInfo.requestedPermissionsFlags = new int[] { + REQUESTED_PERMISSION_GRANTED, REQUESTED_PERMISSION_GRANTED }; + } else { + packageInfo.requestedPermissions = new String[0]; + } packageInfo.applicationInfo = new ApplicationInfo(); packageInfo.applicationInfo.privateFlags = 0; packageInfo.applicationInfo.uid = UserHandle.getUid(UserHandle.USER_SYSTEM, diff --git a/tests/net/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/net/java/com/android/server/connectivity/PermissionMonitorTest.java index cd2bd26ef4..2e892e53fa 100644 --- a/tests/net/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/net/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -27,6 +27,7 @@ import static android.content.pm.ApplicationInfo.PRIVATE_FLAG_OEM; import static android.content.pm.ApplicationInfo.PRIVATE_FLAG_PRODUCT; import static android.content.pm.ApplicationInfo.PRIVATE_FLAG_VENDOR; import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; +import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_REQUIRED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import static android.content.pm.PackageManager.MATCH_ANY_USER; import static android.os.Process.SYSTEM_UID; @@ -36,6 +37,7 @@ import static com.android.server.connectivity.PermissionMonitor.SYSTEM; import static junit.framework.Assert.fail; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -102,7 +104,6 @@ public class PermissionMonitorTest { private static final String MOCK_PACKAGE2 = "appName2"; private static final String SYSTEM_PACKAGE1 = "sysName1"; private static final String SYSTEM_PACKAGE2 = "sysName2"; - private static final String VPN_PACKAGE = "vpnApp"; private static final String PARTITION_SYSTEM = "system"; private static final String PARTITION_OEM = "oem"; private static final String PARTITION_PRODUCT = "product"; @@ -145,28 +146,31 @@ public class PermissionMonitorTest { mObserver = observerCaptor.getValue(); } - private boolean hasBgPermission(String partition, int targetSdkVersion, int uid, - String... permission) throws Exception { - final PackageInfo packageInfo = packageInfoWithPermissions(permission, partition); + private boolean hasRestrictedNetworkPermission(String partition, int targetSdkVersion, int uid, + String... permissions) { + final PackageInfo packageInfo = + packageInfoWithPermissions(REQUESTED_PERMISSION_GRANTED, permissions, partition); packageInfo.applicationInfo.targetSdkVersion = targetSdkVersion; packageInfo.applicationInfo.uid = uid; - when(mPackageManager.getPackageInfoAsUser( - eq(MOCK_PACKAGE1), eq(GET_PERMISSIONS), anyInt())).thenReturn(packageInfo); - when(mPackageManager.getPackagesForUid(anyInt())).thenReturn(new String[] {MOCK_PACKAGE1}); - return mPermissionMonitor.hasUseBackgroundNetworksPermission(uid); + return mPermissionMonitor.hasRestrictedNetworkPermission(packageInfo); } - private static PackageInfo packageInfoWithPermissions(String[] permissions, String partition) { + private static PackageInfo systemPackageInfoWithPermissions(String... permissions) { + return packageInfoWithPermissions( + REQUESTED_PERMISSION_GRANTED, permissions, PARTITION_SYSTEM); + } + + private static PackageInfo vendorPackageInfoWithPermissions(String... permissions) { + return packageInfoWithPermissions( + REQUESTED_PERMISSION_GRANTED, permissions, PARTITION_VENDOR); + } + + private static PackageInfo packageInfoWithPermissions(int permissionsFlags, + String[] permissions, String partition) { int[] requestedPermissionsFlags = new int[permissions.length]; for (int i = 0; i < permissions.length; i++) { - requestedPermissionsFlags[i] = REQUESTED_PERMISSION_GRANTED; + requestedPermissionsFlags[i] = permissionsFlags; } - return packageInfoWithPermissions(permissions, partition, - requestedPermissionsFlags); - } - - private static PackageInfo packageInfoWithPermissions(String[] permissions, String partition, - int[] requestedPermissionsFlags) { final PackageInfo packageInfo = new PackageInfo(); packageInfo.requestedPermissions = permissions; packageInfo.applicationInfo = new ApplicationInfo(); @@ -190,12 +194,10 @@ public class PermissionMonitorTest { private static PackageInfo buildPackageInfo(boolean hasSystemPermission, int uid, int userId) { final PackageInfo pkgInfo; if (hasSystemPermission) { - final String[] systemPermissions = new String[]{ - CHANGE_NETWORK_STATE, NETWORK_STACK, CONNECTIVITY_USE_RESTRICTED_NETWORKS - }; - pkgInfo = packageInfoWithPermissions(systemPermissions, PARTITION_SYSTEM); + pkgInfo = systemPackageInfoWithPermissions( + CHANGE_NETWORK_STATE, NETWORK_STACK, CONNECTIVITY_USE_RESTRICTED_NETWORKS); } else { - pkgInfo = packageInfoWithPermissions(new String[] {}, ""); + pkgInfo = packageInfoWithPermissions(REQUESTED_PERMISSION_GRANTED, new String[] {}, ""); } pkgInfo.applicationInfo.uid = UserHandle.getUid(userId, UserHandle.getAppId(uid)); return pkgInfo; @@ -203,82 +205,151 @@ public class PermissionMonitorTest { @Test public void testHasPermission() { - PackageInfo app = packageInfoWithPermissions(new String[] {}, PARTITION_SYSTEM); + PackageInfo app = systemPackageInfoWithPermissions(); assertFalse(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); assertFalse(mPermissionMonitor.hasPermission(app, NETWORK_STACK)); assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_INTERNAL)); - app = packageInfoWithPermissions(new String[] { - CHANGE_NETWORK_STATE, NETWORK_STACK - }, PARTITION_SYSTEM); + app = systemPackageInfoWithPermissions(CHANGE_NETWORK_STATE, NETWORK_STACK); assertTrue(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); assertTrue(mPermissionMonitor.hasPermission(app, NETWORK_STACK)); assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_INTERNAL)); - app = packageInfoWithPermissions(new String[] { - CONNECTIVITY_USE_RESTRICTED_NETWORKS, CONNECTIVITY_INTERNAL - }, PARTITION_SYSTEM); + app = systemPackageInfoWithPermissions( + CONNECTIVITY_USE_RESTRICTED_NETWORKS, CONNECTIVITY_INTERNAL); assertFalse(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); assertFalse(mPermissionMonitor.hasPermission(app, NETWORK_STACK)); assertTrue(mPermissionMonitor.hasPermission(app, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); assertTrue(mPermissionMonitor.hasPermission(app, CONNECTIVITY_INTERNAL)); + + app = packageInfoWithPermissions(REQUESTED_PERMISSION_REQUIRED, new String[] { + CONNECTIVITY_USE_RESTRICTED_NETWORKS, CONNECTIVITY_INTERNAL, NETWORK_STACK }, + PARTITION_SYSTEM); + assertFalse(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); + assertFalse(mPermissionMonitor.hasPermission(app, NETWORK_STACK)); + assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); + assertFalse(mPermissionMonitor.hasPermission(app, CONNECTIVITY_INTERNAL)); + + app = systemPackageInfoWithPermissions(CHANGE_NETWORK_STATE); + app.requestedPermissions = null; + assertFalse(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); + + app = systemPackageInfoWithPermissions(CHANGE_NETWORK_STATE); + app.requestedPermissionsFlags = null; + assertFalse(mPermissionMonitor.hasPermission(app, CHANGE_NETWORK_STATE)); } @Test public void testIsVendorApp() { - PackageInfo app = packageInfoWithPermissions(new String[] {}, PARTITION_SYSTEM); + PackageInfo app = systemPackageInfoWithPermissions(); assertFalse(mPermissionMonitor.isVendorApp(app.applicationInfo)); - app = packageInfoWithPermissions(new String[] {}, PARTITION_OEM); + app = packageInfoWithPermissions(REQUESTED_PERMISSION_GRANTED, + new String[] {}, PARTITION_OEM); assertTrue(mPermissionMonitor.isVendorApp(app.applicationInfo)); - app = packageInfoWithPermissions(new String[] {}, PARTITION_PRODUCT); + app = packageInfoWithPermissions(REQUESTED_PERMISSION_GRANTED, + new String[] {}, PARTITION_PRODUCT); assertTrue(mPermissionMonitor.isVendorApp(app.applicationInfo)); - app = packageInfoWithPermissions(new String[] {}, PARTITION_VENDOR); + app = vendorPackageInfoWithPermissions(); assertTrue(mPermissionMonitor.isVendorApp(app.applicationInfo)); } + @Test + public void testHasNetworkPermission() { + PackageInfo app = systemPackageInfoWithPermissions(); + assertFalse(mPermissionMonitor.hasNetworkPermission(app)); + app = systemPackageInfoWithPermissions(CHANGE_NETWORK_STATE); + assertTrue(mPermissionMonitor.hasNetworkPermission(app)); + app = systemPackageInfoWithPermissions(NETWORK_STACK); + assertFalse(mPermissionMonitor.hasNetworkPermission(app)); + app = systemPackageInfoWithPermissions(CONNECTIVITY_USE_RESTRICTED_NETWORKS); + assertFalse(mPermissionMonitor.hasNetworkPermission(app)); + } + + @Test + public void testHasRestrictedNetworkPermission() { + assertFalse(hasRestrictedNetworkPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CHANGE_NETWORK_STATE)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, MOCK_UID1, NETWORK_STACK)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CONNECTIVITY_INTERNAL)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CHANGE_WIFI_STATE)); + + assertFalse(hasRestrictedNetworkPermission(PARTITION_SYSTEM, VERSION_Q, MOCK_UID1)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_Q, MOCK_UID1, CHANGE_WIFI_STATE)); + } + + @Test + public void testHasRestrictedNetworkPermissionSystemUid() { + doReturn(VERSION_P).when(mPermissionMonitor).getDeviceFirstSdkInt(); + assertTrue(hasRestrictedNetworkPermission(PARTITION_SYSTEM, VERSION_P, SYSTEM_UID)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, SYSTEM_UID, CHANGE_WIFI_STATE)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_P, SYSTEM_UID, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); + + doReturn(VERSION_Q).when(mPermissionMonitor).getDeviceFirstSdkInt(); + assertFalse(hasRestrictedNetworkPermission(PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID, CHANGE_WIFI_STATE)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); + } + + @Test + public void testHasRestrictedNetworkPermissionVendorApp() { + assertTrue(hasRestrictedNetworkPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_P, MOCK_UID1, CHANGE_NETWORK_STATE)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_P, MOCK_UID1, NETWORK_STACK)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_P, MOCK_UID1, CONNECTIVITY_INTERNAL)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_P, MOCK_UID1, CONNECTIVITY_USE_RESTRICTED_NETWORKS)); + assertTrue(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_P, MOCK_UID1, CHANGE_WIFI_STATE)); + + assertFalse(hasRestrictedNetworkPermission(PARTITION_VENDOR, VERSION_Q, MOCK_UID1)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_Q, MOCK_UID1, CHANGE_WIFI_STATE)); + assertFalse(hasRestrictedNetworkPermission( + PARTITION_VENDOR, VERSION_Q, MOCK_UID1, CHANGE_NETWORK_STATE)); + } + + private void assertBackgroundPermission(boolean hasPermission, String name, int uid, + String... permissions) throws Exception { + when(mPackageManager.getPackageInfo(eq(name), anyInt())) + .thenReturn(packageInfoWithPermissions( + REQUESTED_PERMISSION_GRANTED, permissions, PARTITION_SYSTEM)); + mPermissionMonitor.onPackageAdded(name, uid); + assertEquals(hasPermission, mPermissionMonitor.hasUseBackgroundNetworksPermission(uid)); + } + @Test public void testHasUseBackgroundNetworksPermission() throws Exception { - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CHANGE_NETWORK_STATE)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1, NETWORK_STACK)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CONNECTIVITY_INTERNAL)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1, - CONNECTIVITY_USE_RESTRICTED_NETWORKS)); - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_P, MOCK_UID1, CHANGE_WIFI_STATE)); + assertFalse(mPermissionMonitor.hasUseBackgroundNetworksPermission(SYSTEM_UID)); + assertBackgroundPermission(false, SYSTEM_PACKAGE1, SYSTEM_UID); + assertBackgroundPermission(false, SYSTEM_PACKAGE1, SYSTEM_UID, CHANGE_WIFI_STATE); + assertBackgroundPermission(true, SYSTEM_PACKAGE1, SYSTEM_UID, CHANGE_NETWORK_STATE); + assertBackgroundPermission(true, SYSTEM_PACKAGE1, SYSTEM_UID, NETWORK_STACK); - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_Q, MOCK_UID1)); - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_Q, MOCK_UID1, CHANGE_WIFI_STATE)); - } + assertFalse(mPermissionMonitor.hasUseBackgroundNetworksPermission(MOCK_UID1)); + assertBackgroundPermission(false, MOCK_PACKAGE1, MOCK_UID1); + assertBackgroundPermission(true, MOCK_PACKAGE1, MOCK_UID1, + CONNECTIVITY_USE_RESTRICTED_NETWORKS); - @Test - public void testHasUseBackgroundNetworksPermissionSystemUid() throws Exception { - doReturn(VERSION_P).when(mPermissionMonitor).getDeviceFirstSdkInt(); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, SYSTEM_UID)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, SYSTEM_UID, CHANGE_WIFI_STATE)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_P, SYSTEM_UID, - CONNECTIVITY_USE_RESTRICTED_NETWORKS)); - - doReturn(VERSION_Q).when(mPermissionMonitor).getDeviceFirstSdkInt(); - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID)); - assertFalse(hasBgPermission(PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID, CHANGE_WIFI_STATE)); - assertTrue(hasBgPermission(PARTITION_SYSTEM, VERSION_Q, SYSTEM_UID, - CONNECTIVITY_USE_RESTRICTED_NETWORKS)); - } - - @Test - public void testHasUseBackgroundNetworksPermissionVendorApp() throws Exception { - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1)); - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1, CHANGE_NETWORK_STATE)); - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1, NETWORK_STACK)); - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1, CONNECTIVITY_INTERNAL)); - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1, - CONNECTIVITY_USE_RESTRICTED_NETWORKS)); - assertTrue(hasBgPermission(PARTITION_VENDOR, VERSION_P, MOCK_UID1, CHANGE_WIFI_STATE)); - - assertFalse(hasBgPermission(PARTITION_VENDOR, VERSION_Q, MOCK_UID1)); - assertFalse(hasBgPermission(PARTITION_VENDOR, VERSION_Q, MOCK_UID1, CHANGE_WIFI_STATE)); + assertFalse(mPermissionMonitor.hasUseBackgroundNetworksPermission(MOCK_UID2)); + assertBackgroundPermission(false, MOCK_PACKAGE2, MOCK_UID2); + assertBackgroundPermission(true, MOCK_PACKAGE2, MOCK_UID2, + CONNECTIVITY_INTERNAL); } private class NetdMonitor { @@ -563,7 +634,8 @@ public class PermissionMonitorTest { private PackageInfo addPackage(String packageName, int uid, String[] permissions) throws Exception { - PackageInfo packageInfo = packageInfoWithPermissions(permissions, PARTITION_SYSTEM); + PackageInfo packageInfo = packageInfoWithPermissions( + REQUESTED_PERMISSION_GRANTED, permissions, PARTITION_SYSTEM); when(mPackageManager.getPackageInfo(eq(packageName), anyInt())).thenReturn(packageInfo); when(mPackageManager.getPackagesForUid(eq(uid))).thenReturn(new String[]{packageName}); mObserver.onPackageAdded(packageName, uid); @@ -593,7 +665,7 @@ public class PermissionMonitorTest { // Install another package with the same uid and no permissions should not cause the UID to // lose permissions. - PackageInfo packageInfo2 = packageInfoWithPermissions(new String[]{}, PARTITION_SYSTEM); + PackageInfo packageInfo2 = systemPackageInfoWithPermissions(); when(mPackageManager.getPackageInfo(eq(MOCK_PACKAGE2), anyInt())).thenReturn(packageInfo2); when(mPackageManager.getPackagesForUid(MOCK_UID1)) .thenReturn(new String[]{MOCK_PACKAGE1, MOCK_PACKAGE2}); @@ -641,8 +713,7 @@ public class PermissionMonitorTest { | INetd.PERMISSION_UPDATE_DEVICE_STATS, new int[]{MOCK_UID1}); // Mock another package with the same uid but different permissions. - PackageInfo packageInfo2 = packageInfoWithPermissions(new String[] {INTERNET}, - PARTITION_SYSTEM); + PackageInfo packageInfo2 = systemPackageInfoWithPermissions(INTERNET); when(mPackageManager.getPackageInfo(eq(MOCK_PACKAGE2), anyInt())).thenReturn(packageInfo2); when(mPackageManager.getPackagesForUid(MOCK_UID1)).thenReturn(new String[]{ MOCK_PACKAGE2}); From 27aaa987ab0661ca1cee44ddbf78eaaaeefe99e1 Mon Sep 17 00:00:00 2001 From: Automerger Merge Worker Date: Thu, 19 Dec 2019 03:29:50 +0000 Subject: [PATCH 6/6] Fix parceling of LinkProperties Inet6Addr IPv6 addresses parceled for DNS servers, private DNS servers, PCSCF servers were parceled without the scope. This causes issues with link-local DNS servers. Test: atest FrameworksNetTests Bug: 145181158 (cherry picked from commit 0b0d019da82ae413d400aef2d0a6bd1ed911e45c) Merged-In: Ie5b7782d788717dd1cc440e502d6cdf2d1c18eaa Change-Id: I51313f50de8220988c2c1d26981c27d07dfb55f9 --- core/java/android/net/LinkProperties.java | 53 ++++++--- .../java/android/net/LinkPropertiesTest.java | 109 +++++++++++------- 2 files changed, 104 insertions(+), 58 deletions(-) diff --git a/core/java/android/net/LinkProperties.java b/core/java/android/net/LinkProperties.java index d3f48acdd4..94d4eb9762 100644 --- a/core/java/android/net/LinkProperties.java +++ b/core/java/android/net/LinkProperties.java @@ -73,6 +73,8 @@ public final class LinkProperties implements Parcelable { private static final int MIN_MTU_V6 = 1280; private static final int MAX_MTU = 10000; + private static final int INET6_ADDR_LENGTH = 16; + // Stores the properties of links that are "stacked" above this link. // Indexed by interface name to allow modification and to prevent duplicates being added. private Hashtable mStackedLinks = new Hashtable<>(); @@ -1590,20 +1592,11 @@ public final class LinkProperties implements Parcelable { dest.writeParcelable(linkAddress, flags); } - dest.writeInt(mDnses.size()); - for (InetAddress d : mDnses) { - dest.writeByteArray(d.getAddress()); - } - dest.writeInt(mValidatedPrivateDnses.size()); - for (InetAddress d : mValidatedPrivateDnses) { - dest.writeByteArray(d.getAddress()); - } + writeAddresses(dest, mDnses); + writeAddresses(dest, mValidatedPrivateDnses); dest.writeBoolean(mUsePrivateDns); dest.writeString(mPrivateDnsServerName); - dest.writeInt(mPcscfs.size()); - for (InetAddress d : mPcscfs) { - dest.writeByteArray(d.getAddress()); - } + writeAddresses(dest, mPcscfs); dest.writeString(mDomains); dest.writeInt(mMtu); dest.writeString(mTcpBufferSizes); @@ -1624,6 +1617,35 @@ public final class LinkProperties implements Parcelable { dest.writeList(stackedLinks); } + private static void writeAddresses(@NonNull Parcel dest, @NonNull List list) { + dest.writeInt(list.size()); + for (InetAddress d : list) { + writeAddress(dest, d); + } + } + + private static void writeAddress(@NonNull Parcel dest, @NonNull InetAddress addr) { + dest.writeByteArray(addr.getAddress()); + if (addr instanceof Inet6Address) { + final Inet6Address v6Addr = (Inet6Address) addr; + final boolean hasScopeId = v6Addr.getScopeId() != 0; + dest.writeBoolean(hasScopeId); + if (hasScopeId) dest.writeInt(v6Addr.getScopeId()); + } + } + + @NonNull + private static InetAddress readAddress(@NonNull Parcel p) throws UnknownHostException { + final byte[] addr = p.createByteArray(); + if (addr.length == INET6_ADDR_LENGTH) { + final boolean hasScopeId = p.readBoolean(); + final int scopeId = hasScopeId ? p.readInt() : 0; + return Inet6Address.getByAddress(null /* host */, addr, scopeId); + } + + return InetAddress.getByAddress(addr); + } + /** * Implement the Parcelable interface. */ @@ -1643,14 +1665,13 @@ public final class LinkProperties implements Parcelable { addressCount = in.readInt(); for (int i = 0; i < addressCount; i++) { try { - netProp.addDnsServer(InetAddress.getByAddress(in.createByteArray())); + netProp.addDnsServer(readAddress(in)); } catch (UnknownHostException e) { } } addressCount = in.readInt(); for (int i = 0; i < addressCount; i++) { try { - netProp.addValidatedPrivateDnsServer( - InetAddress.getByAddress(in.createByteArray())); + netProp.addValidatedPrivateDnsServer(readAddress(in)); } catch (UnknownHostException e) { } } netProp.setUsePrivateDns(in.readBoolean()); @@ -1658,7 +1679,7 @@ public final class LinkProperties implements Parcelable { addressCount = in.readInt(); for (int i = 0; i < addressCount; i++) { try { - netProp.addPcscfServer(InetAddress.getByAddress(in.createByteArray())); + netProp.addPcscfServer(readAddress(in)); } catch (UnknownHostException e) { } } netProp.setDomains(in.readString()); diff --git a/tests/net/common/java/android/net/LinkPropertiesTest.java b/tests/net/common/java/android/net/LinkPropertiesTest.java index e1c4238f12..388e9eb8e9 100644 --- a/tests/net/common/java/android/net/LinkPropertiesTest.java +++ b/tests/net/common/java/android/net/LinkPropertiesTest.java @@ -31,6 +31,7 @@ import android.util.ArraySet; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.ParcelableTestUtil; import com.android.internal.util.TestUtils; import org.junit.Test; @@ -47,25 +48,22 @@ import java.util.Set; @RunWith(AndroidJUnit4.class) @SmallTest public class LinkPropertiesTest { - private static final InetAddress ADDRV4 = InetAddresses.parseNumericAddress("75.208.6.1"); - private static final InetAddress ADDRV6 = InetAddresses.parseNumericAddress( - "2001:0db8:85a3:0000:0000:8a2e:0370:7334"); - private static final InetAddress DNS1 = InetAddresses.parseNumericAddress("75.208.7.1"); - private static final InetAddress DNS2 = InetAddresses.parseNumericAddress("69.78.7.1"); - private static final InetAddress DNS6 = InetAddresses.parseNumericAddress( - "2001:4860:4860::8888"); - private static final InetAddress PRIVDNS1 = InetAddresses.parseNumericAddress("1.1.1.1"); - private static final InetAddress PRIVDNS2 = InetAddresses.parseNumericAddress("1.0.0.1"); - private static final InetAddress PRIVDNS6 = InetAddresses.parseNumericAddress( - "2606:4700:4700::1111"); - private static final InetAddress PCSCFV4 = InetAddresses.parseNumericAddress("10.77.25.37"); - private static final InetAddress PCSCFV6 = InetAddresses.parseNumericAddress( - "2001:0db8:85a3:0000:0000:8a2e:0370:1"); - private static final InetAddress GATEWAY1 = InetAddresses.parseNumericAddress("75.208.8.1"); - private static final InetAddress GATEWAY2 = InetAddresses.parseNumericAddress("69.78.8.1"); - private static final InetAddress GATEWAY61 = InetAddresses.parseNumericAddress( - "fe80::6:0000:613"); - private static final InetAddress GATEWAY62 = InetAddresses.parseNumericAddress("fe80::6:2222"); + private static final InetAddress ADDRV4 = address("75.208.6.1"); + private static final InetAddress ADDRV6 = address("2001:0db8:85a3:0000:0000:8a2e:0370:7334"); + private static final InetAddress DNS1 = address("75.208.7.1"); + private static final InetAddress DNS2 = address("69.78.7.1"); + private static final InetAddress DNS6 = address("2001:4860:4860::8888"); + private static final InetAddress PRIVDNS1 = address("1.1.1.1"); + private static final InetAddress PRIVDNS2 = address("1.0.0.1"); + private static final InetAddress PRIVDNS6 = address("2606:4700:4700::1111"); + private static final InetAddress PCSCFV4 = address("10.77.25.37"); + private static final InetAddress PCSCFV6 = address("2001:0db8:85a3:0000:0000:8a2e:0370:1"); + private static final InetAddress GATEWAY1 = address("75.208.8.1"); + private static final InetAddress GATEWAY2 = address("69.78.8.1"); + private static final InetAddress GATEWAY61 = address("fe80::6:0000:613"); + private static final InetAddress GATEWAY62 = address("fe80::6:22%lo"); + private static final InetAddress TESTIPV4ADDR = address("192.168.47.42"); + private static final InetAddress TESTIPV6ADDR = address("fe80::7:33%43"); private static final String NAME = "qmi0"; private static final String DOMAINS = "google.com"; private static final String PRIV_DNS_SERVER_NAME = "private.dns.com"; @@ -75,8 +73,7 @@ public class LinkPropertiesTest { private static final LinkAddress LINKADDRV6 = new LinkAddress(ADDRV6, 128); private static final LinkAddress LINKADDRV6LINKLOCAL = new LinkAddress("fe80::1/64"); - // TODO: replace all calls to NetworkUtils.numericToInetAddress with calls to this method. - private InetAddress Address(String addrString) { + private static InetAddress address(String addrString) { return InetAddresses.parseNumericAddress(addrString); } @@ -223,7 +220,7 @@ public class LinkPropertiesTest { target.clear(); target.setInterfaceName(NAME); // change link addresses - target.addLinkAddress(new LinkAddress(Address("75.208.6.2"), 32)); + target.addLinkAddress(new LinkAddress(address("75.208.6.2"), 32)); target.addLinkAddress(LINKADDRV6); target.addDnsServer(DNS1); target.addDnsServer(DNS2); @@ -238,7 +235,7 @@ public class LinkPropertiesTest { target.addLinkAddress(LINKADDRV4); target.addLinkAddress(LINKADDRV6); // change dnses - target.addDnsServer(Address("75.208.7.2")); + target.addDnsServer(address("75.208.7.2")); target.addDnsServer(DNS2); target.addPcscfServer(PCSCFV6); target.addRoute(new RouteInfo(GATEWAY1)); @@ -250,10 +247,10 @@ public class LinkPropertiesTest { target.setInterfaceName(NAME); target.addLinkAddress(LINKADDRV4); target.addLinkAddress(LINKADDRV6); - target.addDnsServer(Address("75.208.7.2")); + target.addDnsServer(address("75.208.7.2")); target.addDnsServer(DNS2); // change pcscf - target.addPcscfServer(Address("2001::1")); + target.addPcscfServer(address("2001::1")); target.addRoute(new RouteInfo(GATEWAY1)); target.addRoute(new RouteInfo(GATEWAY2)); target.setMtu(MTU); @@ -266,9 +263,9 @@ public class LinkPropertiesTest { target.addDnsServer(DNS1); target.addDnsServer(DNS2); // change gateway - target.addRoute(new RouteInfo(Address("75.208.8.2"))); - target.addRoute(new RouteInfo(GATEWAY2)); + target.addRoute(new RouteInfo(address("75.208.8.2"))); target.setMtu(MTU); + target.addRoute(new RouteInfo(GATEWAY2)); assertFalse(source.equals(target)); target.clear(); @@ -344,7 +341,7 @@ public class LinkPropertiesTest { @Test public void testRouteInterfaces() { - LinkAddress prefix = new LinkAddress(Address("2001:db8::"), 32); + LinkAddress prefix = new LinkAddress(address("2001:db8::"), 32); InetAddress address = ADDRV6; // Add a route with no interface to a LinkProperties with no interface. No errors. @@ -734,8 +731,7 @@ public class LinkPropertiesTest { // Add an on-link route, making the on-link DNS server reachable, // but there is still no IPv4 address. - assertTrue(v4lp.addRoute(new RouteInfo( - new IpPrefix(NetworkUtils.numericToInetAddress("75.208.0.0"), 16)))); + assertTrue(v4lp.addRoute(new RouteInfo(new IpPrefix(address("75.208.0.0"), 16)))); assertFalse(v4lp.isReachable(DNS1)); assertFalse(v4lp.isReachable(DNS2)); @@ -751,9 +747,9 @@ public class LinkPropertiesTest { assertTrue(v4lp.isReachable(DNS2)); final LinkProperties v6lp = new LinkProperties(); - final InetAddress kLinkLocalDns = Address("fe80::6:1"); - final InetAddress kLinkLocalDnsWithScope = Address("fe80::6:2%43"); - final InetAddress kOnLinkDns = Address("2001:db8:85a3::53"); + final InetAddress kLinkLocalDns = address("fe80::6:1"); + final InetAddress kLinkLocalDnsWithScope = address("fe80::6:2%43"); + final InetAddress kOnLinkDns = address("2001:db8:85a3::53"); assertFalse(v6lp.isReachable(kLinkLocalDns)); assertFalse(v6lp.isReachable(kLinkLocalDnsWithScope)); assertFalse(v6lp.isReachable(kOnLinkDns)); @@ -762,7 +758,7 @@ public class LinkPropertiesTest { // Add a link-local route, making the link-local DNS servers reachable. Because // we assume the presence of an IPv6 link-local address, link-local DNS servers // are considered reachable, but only those with a non-zero scope identifier. - assertTrue(v6lp.addRoute(new RouteInfo(new IpPrefix(Address("fe80::"), 64)))); + assertTrue(v6lp.addRoute(new RouteInfo(new IpPrefix(address("fe80::"), 64)))); assertFalse(v6lp.isReachable(kLinkLocalDns)); assertTrue(v6lp.isReachable(kLinkLocalDnsWithScope)); assertFalse(v6lp.isReachable(kOnLinkDns)); @@ -778,7 +774,7 @@ public class LinkPropertiesTest { // Add a global route on link, but no global address yet. DNS servers reachable // via a route that doesn't require a gateway: give them the benefit of the // doubt and hope the link-local source address suffices for communication. - assertTrue(v6lp.addRoute(new RouteInfo(new IpPrefix(Address("2001:db8:85a3::"), 64)))); + assertTrue(v6lp.addRoute(new RouteInfo(new IpPrefix(address("2001:db8:85a3::"), 64)))); assertFalse(v6lp.isReachable(kLinkLocalDns)); assertTrue(v6lp.isReachable(kLinkLocalDnsWithScope)); assertTrue(v6lp.isReachable(kOnLinkDns)); @@ -807,7 +803,7 @@ public class LinkPropertiesTest { stacked.setInterfaceName("v4-test0"); v6lp.addStackedLink(stacked); - InetAddress stackedAddress = Address("192.0.0.4"); + InetAddress stackedAddress = address("192.0.0.4"); LinkAddress stackedLinkAddress = new LinkAddress(stackedAddress, 32); assertFalse(v6lp.isReachable(stackedAddress)); stacked.addLinkAddress(stackedLinkAddress); @@ -840,7 +836,7 @@ public class LinkPropertiesTest { LinkProperties rmnet1 = new LinkProperties(); rmnet1.setInterfaceName("rmnet1"); rmnet1.addLinkAddress(new LinkAddress("10.0.0.3/8")); - RouteInfo defaultRoute1 = new RouteInfo((IpPrefix) null, Address("10.0.0.1"), + RouteInfo defaultRoute1 = new RouteInfo((IpPrefix) null, address("10.0.0.1"), rmnet1.getInterfaceName()); RouteInfo directRoute1 = new RouteInfo(new IpPrefix("10.0.0.0/8"), null, rmnet1.getInterfaceName()); @@ -859,7 +855,7 @@ public class LinkPropertiesTest { rmnet2.setInterfaceName("rmnet2"); rmnet2.addLinkAddress(new LinkAddress("fe80::cafe/64")); rmnet2.addLinkAddress(new LinkAddress("2001:db8::2/64")); - RouteInfo defaultRoute2 = new RouteInfo((IpPrefix) null, Address("2001:db8::1"), + RouteInfo defaultRoute2 = new RouteInfo((IpPrefix) null, address("2001:db8::1"), rmnet2.getInterfaceName()); RouteInfo directRoute2 = new RouteInfo(new IpPrefix("2001:db8::/64"), null, rmnet2.getInterfaceName()); @@ -925,24 +921,53 @@ public class LinkPropertiesTest { public void testLinkPropertiesParcelable() throws Exception { LinkProperties source = new LinkProperties(); source.setInterfaceName(NAME); - // set 2 link addresses + source.addLinkAddress(LINKADDRV4); source.addLinkAddress(LINKADDRV6); - // set 2 dnses + source.addDnsServer(DNS1); source.addDnsServer(DNS2); - // set 2 gateways + source.addDnsServer(GATEWAY62); + + source.addPcscfServer(TESTIPV4ADDR); + source.addPcscfServer(TESTIPV6ADDR); + + source.setUsePrivateDns(true); + source.setPrivateDnsServerName(PRIV_DNS_SERVER_NAME); + + source.setDomains(DOMAINS); + source.addRoute(new RouteInfo(GATEWAY1)); source.addRoute(new RouteInfo(GATEWAY2)); - // set 2 validated private dnses + source.addValidatedPrivateDnsServer(DNS6); source.addValidatedPrivateDnsServer(GATEWAY61); + source.addValidatedPrivateDnsServer(TESTIPV6ADDR); + + source.setHttpProxy(ProxyInfo.buildDirectProxy("test", 8888)); source.setMtu(MTU); + source.setTcpBufferSizes(TCP_BUFFER_SIZES); + source.setNat64Prefix(new IpPrefix("2001:db8:1:2:64:64::/96")); + final LinkProperties stacked = new LinkProperties(); + stacked.setInterfaceName("test-stacked"); + source.addStackedLink(stacked); + TestUtils.assertParcelingIsLossless(source); + ParcelableTestUtil.assertFieldCountEquals(14, LinkProperties.class); + } + + @Test + public void testLinkLocalDnsServerParceling() throws Exception { + final String strAddress = "fe80::1%lo"; + final LinkProperties lp = new LinkProperties(); + lp.addDnsServer(address(strAddress)); + final LinkProperties unparceled = TestUtils.parcelingRoundTrip(lp); + // Inet6Address#equals does not test for the scope id + assertEquals(strAddress, unparceled.getDnsServers().get(0).getHostAddress()); } @Test