From 687aa58f2e063683b4b4b0afa4a5a86928be56fb Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 3 May 2018 21:07:58 -0700 Subject: [PATCH 1/2] DO NOT MERGE: Fix ConnectivityController meteredness checks This patch corrects ConnectivityController's meteredness checks to perform correct meteredness checks while VPNs are running. This fixes a bug in O-MR1 where any apps using the DownloadProvider with unmetered network constraints fail to start while the VPN is enabled. This change adds a bespoke method for ConnectivityController, allowing it to correctly identify the meteredness without affecting public API surfaces. Bug: 78644887 Test: Built, flashed on Walleye, and tested. Test: Additional test coverage in subsequent patch(es). Change-Id: Ie1d11d93d51d936ce81cd5984af61bde30325983 --- .../java/android/net/ConnectivityManager.java | 22 +++++++++++++++++++ .../android/net/IConnectivityManager.aidl | 1 + .../android/server/ConnectivityService.java | 12 +++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index d7ecc81ffd..467eb9b0b0 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -2504,6 +2504,28 @@ public class ConnectivityManager { } } + /** + * Returns if the active data network for the given UID is metered. A network + * is classified as metered when the user is sensitive to heavy data usage on + * that connection due to monetary costs, data limitations or + * battery/performance issues. You should check this before doing large + * data transfers, and warn the user or delay the operation until another + * network is available. + * + * @return {@code true} if large transfers should be avoided, otherwise + * {@code false}. + * + * @hide + */ + @RequiresPermission(android.Manifest.permission.CONNECTIVITY_INTERNAL) + public boolean isActiveNetworkMeteredForUid(int uid) { + try { + return mService.isActiveNetworkMeteredForUid(uid); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } + } + /** * If the LockdownVpn mechanism is enabled, updates the vpn * with a reload of its profile. diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index a6fe7389bc..f11372c2b3 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -66,6 +66,7 @@ interface IConnectivityManager NetworkQuotaInfo getActiveNetworkQuotaInfo(); boolean isActiveNetworkMetered(); + boolean isActiveNetworkMeteredForUid(int uid); boolean requestRouteToHostAddress(int networkType, in byte[] hostAddress); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index c1801b80af..c2b83d98a8 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1339,7 +1339,17 @@ public class ConnectivityService extends IConnectivityManager.Stub public boolean isActiveNetworkMetered() { enforceAccessPermission(); - final int uid = Binder.getCallingUid(); + return isActiveNetworkMeteredCommon(Binder.getCallingUid()); + } + + @Override + public boolean isActiveNetworkMeteredForUid(int uid) { + enforceConnectivityInternalPermission(); + + return isActiveNetworkMeteredCommon(uid); + } + + private boolean isActiveNetworkMeteredCommon(int uid) { final NetworkCapabilities caps = getUnfilteredActiveNetworkState(uid).networkCapabilities; if (caps != null) { return !caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED); From f0713b549144f8d4ebbe2bbb834b3de3a3494f1b Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 20 Feb 2018 15:19:59 -0800 Subject: [PATCH 2/2] DO NOT MERGE: Add unit tests to ensure VPN meteredness These new tests ensure that VPNs report the meteredness of their underlying networks correctly. The added test verifies VPN meteredness for cases of metered and unmetered WiFi and Cell Bug: 78644887 Test: This; ran on walleye-eng Change-Id: I28bdc71a336bfd97f7908455d4781d774df44b87 --- .../android/server/ConnectivityService.java | 58 ++++++++----- .../server/ConnectivityServiceTest.java | 87 +++++++++++++++++++ 2 files changed, 124 insertions(+), 21 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index c2b83d98a8..6e8c0d4a55 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -969,7 +969,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!mLockdownEnabled) { int user = UserHandle.getUserId(uid); synchronized (mVpns) { - Vpn vpn = mVpns.get(user); + Vpn vpn = getVpn(user); if (vpn != null && vpn.appliesToUid(uid)) { return vpn.getUnderlyingNetworks(); } @@ -1017,7 +1017,7 @@ public class ConnectivityService extends IConnectivityManager.Stub return false; } synchronized (mVpns) { - final Vpn vpn = mVpns.get(UserHandle.getUserId(uid)); + final Vpn vpn = getVpn(UserHandle.getUserId(uid)); if (vpn != null && vpn.isBlockingUid(uid)) { return true; } @@ -1094,7 +1094,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final int user = UserHandle.getUserId(uid); int vpnNetId = NETID_UNSET; synchronized (mVpns) { - final Vpn vpn = mVpns.get(user); + final Vpn vpn = getVpn(user); if (vpn != null && vpn.appliesToUid(uid)) vpnNetId = vpn.getNetId(); } NetworkAgentInfo nai; @@ -1224,7 +1224,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!mLockdownEnabled) { synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn != null) { Network[] networks = vpn.getUnderlyingNetworks(); if (networks != null) { @@ -3424,7 +3424,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn != null) { return vpn.prepare(oldPackage, newPackage); } else { @@ -3451,7 +3451,7 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceCrossUserPermission(userId); synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn != null) { vpn.setPackageAuthorization(packageName, authorized); } @@ -3470,7 +3470,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { - return mVpns.get(user).establish(config); + return getVpn(user).establish(config); } } @@ -3487,7 +3487,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { - mVpns.get(user).startLegacyVpn(profile, mKeyStore, egress); + getVpn(user).startLegacyVpn(profile, mKeyStore, egress); } } @@ -3501,7 +3501,7 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceCrossUserPermission(userId); synchronized (mVpns) { - return mVpns.get(userId).getLegacyVpnInfo(); + return getVpn(userId).getLegacyVpnInfo(); } } @@ -3565,7 +3565,7 @@ public class ConnectivityService extends IConnectivityManager.Stub public VpnConfig getVpnConfig(int userId) { enforceCrossUserPermission(userId); synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn != null) { return vpn.getVpnConfig(); } else { @@ -3599,7 +3599,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { - Vpn vpn = mVpns.get(user); + Vpn vpn = getVpn(user); if (vpn == null) { Slog.w(TAG, "VPN for user " + user + " not ready yet. Skipping lockdown"); return false; @@ -3646,7 +3646,7 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private boolean startAlwaysOnVpn(int userId) { synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn == null) { // Shouldn't happen as all codepaths that point here should have checked the Vpn // exists already. @@ -3664,7 +3664,7 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceCrossUserPermission(userId); synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn == null) { Slog.w(TAG, "User " + userId + " has no Vpn configuration"); return false; @@ -3684,7 +3684,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn == null) { Slog.w(TAG, "User " + userId + " has no Vpn configuration"); return false; @@ -3706,7 +3706,7 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceCrossUserPermission(userId); synchronized (mVpns) { - Vpn vpn = mVpns.get(userId); + Vpn vpn = getVpn(userId); if (vpn == null) { Slog.w(TAG, "User " + userId + " has no Vpn configuration"); return null; @@ -3852,22 +3852,38 @@ public class ConnectivityService extends IConnectivityManager.Stub private void onUserStart(int userId) { synchronized (mVpns) { - Vpn userVpn = mVpns.get(userId); + Vpn userVpn = getVpn(userId); if (userVpn != null) { loge("Starting user already has a VPN"); return; } userVpn = new Vpn(mHandler.getLooper(), mContext, mNetd, userId); - mVpns.put(userId, userVpn); + setVpn(userId, userVpn); } if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) { updateLockdownVpn(); } } + /** @hide */ + @VisibleForTesting + Vpn getVpn(int userId) { + synchronized (mVpns) { + return mVpns.get(userId); + } + } + + /** @hide */ + @VisibleForTesting + void setVpn(int userId, Vpn userVpn) { + synchronized (mVpns) { + mVpns.put(userId, userVpn); + } + } + private void onUserStop(int userId) { synchronized (mVpns) { - Vpn userVpn = mVpns.get(userId); + Vpn userVpn = getVpn(userId); if (userVpn == null) { loge("Stopped user has no VPN"); return; @@ -5439,7 +5455,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { - return mVpns.get(user).addAddress(address, prefixLength); + return getVpn(user).addAddress(address, prefixLength); } } @@ -5448,7 +5464,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { - return mVpns.get(user).removeAddress(address, prefixLength); + return getVpn(user).removeAddress(address, prefixLength); } } @@ -5458,7 +5474,7 @@ public class ConnectivityService extends IConnectivityManager.Stub int user = UserHandle.getUserId(Binder.getCallingUid()); boolean success; synchronized (mVpns) { - success = mVpns.get(user).setUnderlyingNetworks(networks); + success = getVpn(user).setUnderlyingNetworks(networks); } if (success) { notifyIfacesChangedForNetworkStats(); diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 6674f20317..504a2dbc36 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -22,6 +22,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.ConnectivityManager.getNetworkTypeName; import static android.net.NetworkCapabilities.*; @@ -102,6 +103,7 @@ import com.android.server.connectivity.MockableSystemProperties; import com.android.server.connectivity.NetworkAgentInfo; import com.android.server.connectivity.NetworkMonitor; import com.android.server.connectivity.NetworkMonitor.CaptivePortalProbeResult; +import com.android.server.connectivity.Vpn; import com.android.server.net.NetworkPinner; import com.android.server.net.NetworkPolicyManagerInternal; @@ -333,6 +335,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { case TRANSPORT_WIFI_AWARE: mScore = 20; break; + case TRANSPORT_VPN: + mScore = 0; + break; default: throw new UnsupportedOperationException("unimplemented network type"); } @@ -868,6 +873,8 @@ public class ConnectivityServiceTest extends AndroidTestCase { return TYPE_WIFI; case TRANSPORT_CELLULAR: return TYPE_MOBILE; + case TRANSPORT_VPN: + return TYPE_VPN; default: return TYPE_NONE; } @@ -3447,4 +3454,84 @@ public class ConnectivityServiceTest extends AndroidTestCase { return; } } + + @SmallTest + public void testVpnNetworkMetered() { + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(callback); + + final NetworkRequest cellRequest = new NetworkRequest.Builder() + .addTransportType(TRANSPORT_CELLULAR).build(); + final TestNetworkCallback cellCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback(cellRequest, cellCallback); + + // Setup cellular + mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(true); + callback.expectAvailableAndValidatedCallbacks(mCellNetworkAgent); + cellCallback.expectAvailableAndValidatedCallbacks(mCellNetworkAgent); + verifyActiveNetwork(TRANSPORT_CELLULAR); + + // Verify meteredness of cellular + assertTrue(mCm.isActiveNetworkMetered()); + + // Setup Wifi + mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(true); + callback.expectAvailableAndValidatedCallbacks(mWiFiNetworkAgent); + cellCallback.expectCallback(CallbackState.LOSING, mCellNetworkAgent); + verifyActiveNetwork(TRANSPORT_WIFI); + + // Verify meteredness of WiFi + assertTrue(mCm.isActiveNetworkMetered()); + + // Verify that setting unmetered on Wifi changes ActiveNetworkMetered + mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + callback.expectCapabilitiesWith(NET_CAPABILITY_NOT_METERED, mWiFiNetworkAgent); + assertFalse(mCm.isActiveNetworkMetered()); + + // Setup VPN + final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + vpnNetworkAgent.connect(true); + + Vpn mockVpn = mock(Vpn.class); + when(mockVpn.appliesToUid(anyInt())).thenReturn(true); + when(mockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); + + Vpn oldVpn = mService.getVpn(UserHandle.myUserId()); + mService.setVpn(UserHandle.myUserId(), mockVpn); + assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + + // Verify meteredness of VPN on default network + when(mockVpn.getUnderlyingNetworks()).thenReturn(null); + assertFalse(mCm.isActiveNetworkMetered()); + assertFalse(mCm.isActiveNetworkMeteredForUid(Process.myUid())); + + // Verify meteredness of VPN on unmetered wifi + when(mockVpn.getUnderlyingNetworks()) + .thenReturn(new Network[] {mWiFiNetworkAgent.getNetwork()}); + assertFalse(mCm.isActiveNetworkMetered()); + assertFalse(mCm.isActiveNetworkMeteredForUid(Process.myUid())); + + // Set WiFi as metered, then check to see that it has been updated on the VPN + mWiFiNetworkAgent.removeCapability(NET_CAPABILITY_NOT_METERED); + callback.expectCapabilitiesWithout(NET_CAPABILITY_NOT_METERED, mWiFiNetworkAgent); + assertTrue(mCm.isActiveNetworkMetered()); + assertTrue(mCm.isActiveNetworkMeteredForUid(Process.myUid())); + + // Switch to cellular + when(mockVpn.getUnderlyingNetworks()) + .thenReturn(new Network[] {mCellNetworkAgent.getNetwork()}); + assertTrue(mCm.isActiveNetworkMetered()); + assertTrue(mCm.isActiveNetworkMeteredForUid(Process.myUid())); + + // Test unmetered cellular + mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + cellCallback.expectCapabilitiesWith(NET_CAPABILITY_NOT_METERED, mCellNetworkAgent); + assertFalse(mCm.isActiveNetworkMetered()); + assertFalse(mCm.isActiveNetworkMeteredForUid(Process.myUid())); + + mService.setVpn(UserHandle.myUserId(), oldVpn); + mCm.unregisterNetworkCallback(callback); + } }