From e4f5f85647bab3397ed0bd2136a251155a08fe2f Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 4 Sep 2025 00:08:50 -0700 Subject: [PATCH] Fix VPN bypass by updating VPN filter This commit addresses an issue where the VPN bypass feature was not functioning correctly. The VPN filter has been updated to properly handle bypass rules, ensuring that designated traffic can now bypass the VPN as intended. Test: TH, manual Flag: EXEMPT guarded by default-off legacy flag Change-Id: Ie6073db4e23a3132042da21c25b696fa3abc4c91 --- .../android/server/ConnectivityService.java | 75 ++++++-- .../server/connectivity/NetworkAgentInfo.java | 23 +++ .../connectivity/PermissionMonitor.java | 12 +- .../server/ConnectivityServiceTest.java | 170 +++++++++++++----- .../connectivity/PermissionMonitorTest.java | 40 ++++- .../CSCaptivePortalAppTest.kt | 121 ++++++++++++- 6 files changed, 364 insertions(+), 77 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index abe1259abf..52f0aa3f25 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -5889,6 +5889,11 @@ public class ConnectivityService extends IConnectivityManager.Stub wakeupModifyInterface(iface, nai, false); } nai.networkMonitor().notifyNetworkDisconnected(); + if (!nai.getCaptivePortalDelegateUids().isEmpty()) { + final Set oldDelegateBypassUids = getAllCaptivePortalDelegateUids(); + nai.clearCaptivePortalDelegateUids(); + updateAllVpnForDelegateUid(oldDelegateBypassUids, getAllCaptivePortalDelegateUids()); + } mNetworkAgentInfos.remove(nai); if (mDeps.isShortNetworkSuspensionEnforced(mContext)) { // Remove the message to disconnect the network if it's in suspended state for too long, @@ -6819,6 +6824,21 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private Set getAllCaptivePortalDelegateUids() { + final Set uids = new ArraySet<>(); + forEachNetworkAgentInfo(nai -> uids.addAll(nai.getCaptivePortalDelegateUids())); + return uids; + } + + private void updateAllVpnForDelegateUid(Set oldDelegateBypassUids, + Set newDelegateBypassUids) { + forEachNetworkAgentInfo(vpnNai -> { + if (!vpnNai.isVPN()) return; + updateVpnFiltering(vpnNai.linkProperties, vpnNai.linkProperties, vpnNai, + oldDelegateBypassUids, newDelegateBypassUids); + }); + } + public class CaptivePortalImpl extends ICaptivePortal.Stub implements IBinder.DeathRecipient { private final Network mNetwork; // Binder object to track the lifetime of the setDelegateUid caller for cleanup purposes. @@ -6872,7 +6892,22 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private int updateDelegateUid(@NonNull NetworkAgentInfo nai, int uid) { + if (nai.isCurrentUidDelegate(this, uid)) { + return 0; + } + // TODO: consider using exceptions instead of errnos. + final int errno = nai.removeCaptivePortalDelegateUid(this); + if (errno != 0) return errno; + + // If uid == INVALID_UID, we are done. + if (uid == INVALID_UID) return 0; + + return nai.setCaptivePortalDelegateUid(this, uid); + } + private int handleSetDelegateUid(int uid, @NonNull final IBinder callerBinder) { + ensureRunningOnConnectivityServiceThread(); if (mDelegateUidCaller == null) { mDelegateUidCaller = callerBinder; try { @@ -6884,7 +6919,6 @@ public class ConnectivityService extends IConnectivityManager.Stub return ENOTCONN; } } - final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(mNetwork); if (nai == null) return ENOENT; // network does not exist anymore. if (nai.isDestroyed()) return ENOENT; // network has already been destroyed. @@ -6892,16 +6926,13 @@ public class ConnectivityService extends IConnectivityManager.Stub // TODO: consider allowing the uid to bypass VPN on all networks before V. if (!mDeps.isAtLeastV()) return EOPNOTSUPP; - // Check whether there has already been a delegate UID configured, if so, perform - // cleanup and disallow bypassing VPN for that UID if no other caller is delegating - // this UID. - // TODO: consider using exceptions instead of errnos. - final int errno = nai.removeCaptivePortalDelegateUid(this); - if (errno != 0) return errno; - - // If uid == INVALID_UID, we are done. - if (uid == INVALID_UID) return 0; - return nai.setCaptivePortalDelegateUid(this, uid); + final Set oldDelegateBypassUids = getAllCaptivePortalDelegateUids(); + int ret = updateDelegateUid(nai, uid); + // updateDelegateUid() updates mCaptivePortalDelegateUids even if it returns non-zero + // value. Therefore, we need to call updateAllVpnForDelegateUid regardless of the + // returned value. + updateAllVpnForDelegateUid(oldDelegateBypassUids, getAllCaptivePortalDelegateUids()); + return ret; } @Override @@ -10189,7 +10220,10 @@ public class ConnectivityService extends IConnectivityManager.Stub // update filtering rules, need to happen after the interface update so netd knows about the // new interface (the interface name -> index map becomes initialized) - updateVpnFiltering(newLp, oldLp, networkAgent); + if (networkAgent.isVPN()) { + Set delegateBypassUids = getAllCaptivePortalDelegateUids(); + updateVpnFiltering(newLp, oldLp, networkAgent, delegateBypassUids, delegateBypassUids); + } updateIngressToVpnAddressFiltering(newLp, oldLp, networkAgent); @@ -10783,7 +10817,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void updateVpnFiltering(@NonNull LinkProperties newLp, @Nullable LinkProperties oldLp, - @NonNull NetworkAgentInfo nai) { + @NonNull NetworkAgentInfo nai, Set oldDelegateBypassUids, + Set newDelegateBypassUids) { final String oldIface = getVpnIsolationInterface(nai, nai.networkCapabilities, oldLp); final String newIface = getVpnIsolationInterface(nai, nai.networkCapabilities, newLp); final boolean wasFiltering = requiresVpnAllowRule(nai, oldLp, oldIface); @@ -10794,7 +10829,8 @@ public class ConnectivityService extends IConnectivityManager.Stub return; } - if (Objects.equals(oldIface, newIface) && (wasFiltering == needsFiltering)) { + if (Objects.equals(oldIface, newIface) && (wasFiltering == needsFiltering) + && oldDelegateBypassUids.equals(newDelegateBypassUids)) { // Nothing changed. return; } @@ -10810,10 +10846,12 @@ public class ConnectivityService extends IConnectivityManager.Stub // make eBPF support two allowlisted interfaces so here new rules can be added before the // old rules are being removed. if (wasFiltering) { - mPermissionMonitor.onVpnUidRangesRemoved(oldIface, ranges, vpnAppUid); + mPermissionMonitor.onVpnUidRangesRemoved(oldIface, ranges, vpnAppUid, + oldDelegateBypassUids); } if (needsFiltering) { - mPermissionMonitor.onVpnUidRangesAdded(newIface, ranges, vpnAppUid); + mPermissionMonitor.onVpnUidRangesAdded(newIface, ranges, vpnAppUid, + newDelegateBypassUids); } } @@ -11565,10 +11603,11 @@ public class ConnectivityService extends IConnectivityManager.Stub // to be removed will never overlap with the new range to be added. if (wasFiltering && !prevRanges.isEmpty()) { mPermissionMonitor.onVpnUidRangesRemoved(oldIface, prevRanges, - prevNc.getOwnerUid()); + prevNc.getOwnerUid(), getAllCaptivePortalDelegateUids()); } if (shouldFilter && !newRanges.isEmpty()) { - mPermissionMonitor.onVpnUidRangesAdded(newIface, newRanges, newNc.getOwnerUid()); + mPermissionMonitor.onVpnUidRangesAdded(newIface, newRanges, newNc.getOwnerUid(), + getAllCaptivePortalDelegateUids()); } } catch (Exception e) { // Never crash! diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java index 253ff9ed81..329f03c374 100644 --- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java +++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java @@ -25,6 +25,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_ETHERNET; import static android.net.NetworkCapabilities.TRANSPORT_TEST; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.net.NetworkCapabilities.transportNamesOf; +import static android.os.Process.INVALID_UID; import static android.system.OsConstants.EEXIST; import static android.system.OsConstants.EIO; import static android.system.OsConstants.ENOENT; @@ -93,6 +94,7 @@ import java.util.Arrays; import java.util.List; import java.util.NoSuchElementException; import java.util.Objects; +import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import java.util.function.Consumer; @@ -1653,6 +1655,27 @@ public class NetworkAgentInfo implements NetworkRanker.Scoreable { } } + /** + * Get the delegate UIDs of the apps that are allowed to perform network traffic for captive. + */ + public Set getCaptivePortalDelegateUids() { + return new ArraySet<>(mCaptivePortalDelegateUids.values()); + } + + /** + * Clear all the delegate UIDs. + */ + public void clearCaptivePortalDelegateUids() { + mCaptivePortalDelegateUids.clear(); + } + + /** + * Check if the given uid is the delegate uid of the given caller. + */ + public boolean isCurrentUidDelegate(@NonNull CaptivePortalImpl caller, int uid) { + return uid == mCaptivePortalDelegateUids.getOrDefault(caller, INVALID_UID); + } + /** * Set the delegate UID of the app that is allowed to perform network traffic for captive * portal login, and configure the netd bypass rule with this delegated UID. diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index d4d947d75f..692567865f 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -1076,14 +1076,14 @@ public class PermissionMonitor { * @param vpnAppUid The uid of the VPN app */ public synchronized void onVpnUidRangesAdded(@Nullable String iface, Set rangesToAdd, - int vpnAppUid) { + int vpnAppUid, Set delegatedBypassUids) { // Calculate the list of new app uids under the VPN due to the new UID ranges and update // Netd about them. Because mAllApps only contains appIds instead of uids, the result might // be an overestimation if an app is not installed on the user on which the VPN is running, // but that's safe: if an app is not installed, it cannot receive any packets, so dropping // packets to that UID is fine. final Set changedUids = intersectUids(rangesToAdd, mAllApps); - removeBypassingUids(changedUids, vpnAppUid); + removeBypassingUids(changedUids, vpnAppUid, delegatedBypassUids); updateVpnUidsInterfaceRules(iface, changedUids, true /* add */); if (mVpnInterfaceUidRanges.containsKey(iface)) { mVpnInterfaceUidRanges.get(iface).addAll(rangesToAdd); @@ -1101,11 +1101,11 @@ public class PermissionMonitor { * @param vpnAppUid The uid of the VPN app */ public synchronized void onVpnUidRangesRemoved(@Nullable String iface, - Set rangesToRemove, int vpnAppUid) { + Set rangesToRemove, int vpnAppUid, Set delegatedBypassUids) { // Calculate the list of app uids that are no longer under the VPN due to the removed UID // ranges and update Netd about them. final Set changedUids = intersectUids(rangesToRemove, mAllApps); - removeBypassingUids(changedUids, vpnAppUid); + removeBypassingUids(changedUids, vpnAppUid, delegatedBypassUids); updateVpnUidsInterfaceRules(iface, changedUids, false /* add */); Set existingRanges = mVpnInterfaceUidRanges.getOrDefault(iface, null); if (existingRanges == null) { @@ -1201,8 +1201,10 @@ public class PermissionMonitor { * @param uids The list of uids to operate on * @param vpnAppUid The uid of the VPN app */ - private void removeBypassingUids(Set uids, int vpnAppUid) { + private void removeBypassingUids(Set uids, int vpnAppUid, + Set delegateBypassUids) { uids.remove(vpnAppUid); + uids.removeAll(delegateBypassUids); uids.removeIf(this::hasRestrictedNetworksPermission); } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index fdfcc51337..25b564a138 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -5072,68 +5072,142 @@ public class ConnectivityServiceTest { @Test @IgnoreUpTo(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) - public void testCaptivePortalApp_SetDelegateUid() throws Exception { - final TestNetworkCallback captivePortalCallback = new TestNetworkCallback(); - final NetworkRequest captivePortalRequest = new NetworkRequest.Builder() - .addCapability(NET_CAPABILITY_CAPTIVE_PORTAL).build(); - mCm.registerNetworkCallback(captivePortalRequest, captivePortalCallback); + public void testCaptivePortalApp_SetDelegateUidWithVpn() throws Exception { + LinkProperties lp = new LinkProperties(); + InOrder inOrder = inOrder(mMockNetd, mBpfNetMaps); + lp.setInterfaceName("tun0"); + lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), null)); + lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), RTN_UNREACHABLE)); + // The uid range needs to cover the test app so the network is visible to it. + final Set vpnRange = Collections.singleton(PRIMARY_UIDRANGE); + mMockVpn.establish(lp, VPN_UID, vpnRange); + assertVpnUidRangesUpdated(true, vpnRange, VPN_UID); + // A connected VPN should have interface rules set up. There are two expected invocations, + // one during the VPN initial connection, one during the VPN LinkProperties update. + ArgumentCaptor uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(2)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getAllValues().get(0), APP1_UID, APP2_UID); + assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID); + + // bring up Wi-Fi with captive portal + final TestNetworkCallback wifiCaptivePortalCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback( + new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_CAPTIVE_PORTAL) + .addTransportType(TRANSPORT_WIFI) + .build(), + wifiCaptivePortalCallback); mWiFiAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiAgent.connectWithCaptivePortal(TEST_REDIRECT_URL, false); - captivePortalCallback.expectAvailableCallbacksUnvalidated(mWiFiAgent); - - final Intent signInIntent = startCaptivePortalApp(mWiFiAgent); - final CaptivePortal captivePortal = signInIntent + wifiCaptivePortalCallback.expectAvailableCallbacksUnvalidated(mWiFiAgent); + mCm.unregisterNetworkCallback(wifiCaptivePortalCallback); + final Intent wifiSignInIntent = startCaptivePortalApp(mWiFiAgent); + final CaptivePortal wificaptivePortal = wifiSignInIntent .getParcelableExtra(ConnectivityManager.EXTRA_CAPTIVE_PORTAL); - InOrder inOrder = inOrder(mMockNetd); + // bring up ethernet with captive portal + final TestNetworkCallback ethernetCaptivePortalCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback( + new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_CAPTIVE_PORTAL) + .addTransportType(TRANSPORT_ETHERNET) + .build(), + ethernetCaptivePortalCallback); + mEthernetAgent = new TestNetworkAgentWrapper(TRANSPORT_ETHERNET); + mEthernetAgent.connectWithCaptivePortal(TEST_REDIRECT_URL, false); + ethernetCaptivePortalCallback.expectAvailableCallbacksUnvalidated(mEthernetAgent); + mCm.unregisterNetworkCallback(ethernetCaptivePortalCallback); + final Intent ethernetSignInIntent = startCaptivePortalApp(mEthernetAgent); + final CaptivePortal ethernetCaptivePortal = ethernetSignInIntent + .getParcelableExtra(ConnectivityManager.EXTRA_CAPTIVE_PORTAL); - // Add the UID and check that it's added to the bypass list. - FakeOutcomeReceiver or = new FakeOutcomeReceiver(); - captivePortal.setDelegateUid(APP2_UID, r -> r.run(), or); + // Set delegate UID + FakeOutcomeReceiver or = new FakeOutcomeReceiver<>(); + wificaptivePortal.setDelegateUid(APP1_UID, Runnable::run, or); or.awaitOutcome(); - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP2_UID, - mWiFiAgent.getNetwork().netId); - - // Remove the UID and check that it's removed from the list. - or = new FakeOutcomeReceiver(); - captivePortal.setDelegateUid(Process.INVALID_UID, r -> r.run(), or); - or.awaitOutcome(); - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(false, APP2_UID, - mWiFiAgent.getNetwork().netId); - - // Add the UID again. - or = new FakeOutcomeReceiver(); - captivePortal.setDelegateUid(APP2_UID, r -> r.run(), or); - or.awaitOutcome(); - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP2_UID, - mWiFiAgent.getNetwork().netId); - - // Add the UID again. Nothing should change. - or = new FakeOutcomeReceiver(); - captivePortal.setDelegateUid(APP2_UID, r -> r.run(), or); - or.awaitOutcome(); - // BUG: the code should not call into netd. - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(false, APP2_UID, - mWiFiAgent.getNetwork().netId); - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP2_UID, - mWiFiAgent.getNetwork().netId); - - // Add another UID again. The old UID should be removed and the new one added. - or = new FakeOutcomeReceiver(); - captivePortal.setDelegateUid(APP1_UID, r -> r.run(), or); - or.awaitOutcome(); - inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(false, APP2_UID, - mWiFiAgent.getNetwork().netId); inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP1_UID, mWiFiAgent.getNetwork().netId); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).removeUidInterfaceRules(uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP2_UID); + // Add and remove delegate UID from another client for same UID should not trigger any VPN + // rule update. + or = new FakeOutcomeReceiver<>(); + ethernetCaptivePortal.setDelegateUid(APP1_UID, Runnable::run, or); + or.awaitOutcome(); + inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP1_UID, + mEthernetAgent.getNetwork().netId); + inOrder.verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); + inOrder.verify(mBpfNetMaps, never()).removeUidInterfaceRules(any()); + + or = new FakeOutcomeReceiver<>(); + ethernetCaptivePortal.setDelegateUid(INVALID_UID, Runnable::run, or); + or.awaitOutcome(); + inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(false, APP1_UID, + mEthernetAgent.getNetwork().netId); + inOrder.verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); + inOrder.verify(mBpfNetMaps, never()).removeUidInterfaceRules(any()); + + // Disconnect VPN + mMockVpn.disconnect(); + waitForIdle(); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).removeUidInterfaceRules(uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP2_UID); + + // Reconnect VPN + mMockVpn.establish(lp, VPN_UID, vpnRange); + assertVpnUidRangesUpdated(true, vpnRange, VPN_UID); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(2)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getAllValues().get(0), APP2_UID); + assertContainsExactly(uidCaptor.getAllValues().get(1), APP2_UID); + + // Remove delegate UID + or = new FakeOutcomeReceiver<>(); + wificaptivePortal.setDelegateUid(Process.INVALID_UID, Runnable::run, or); + or.awaitOutcome(); + inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(false, APP1_UID, + mWiFiAgent.getNetwork().netId); + inOrder.verify(mBpfNetMaps, times(1)).removeUidInterfaceRules(uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP2_UID); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID); + + // Add back delegate UID + or = new FakeOutcomeReceiver<>(); + wificaptivePortal.setDelegateUid(APP1_UID, Runnable::run, or); + or.awaitOutcome(); + inOrder.verify(mMockNetd).networkAllowBypassVpnOnNetwork(true, APP1_UID, + mWiFiAgent.getNetwork().netId); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).removeUidInterfaceRules(uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP2_UID); + + // Wi-Fi network go away should trigger VPN rule being cleaned up. mWiFiAgent.disconnect(); + waitForIdle(); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).removeUidInterfaceRules(uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP2_UID); + uidCaptor = ArgumentCaptor.forClass(int[].class); + inOrder.verify(mBpfNetMaps, times(1)).addUidInterfaceRules(eq("tun0"), uidCaptor.capture()); + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID); - // TODO: check VPN interface filtering rules, see - // testFullyRoutedVpnResultsInInterfaceFilteringRules + mMockVpn.disconnect(); + waitForIdle(); } + @Test public void testAvoidOrIgnoreCaptivePortals() throws Exception { final TestNetworkCallback captivePortalCallback = new TestNetworkCallback(); diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index a0ead9afbc..83269bdb05 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -949,7 +949,7 @@ public class PermissionMonitorTest { final Set vpnRange2 = Set.of(new UidRange(MOCK_UID12, MOCK_UID12)); // When VPN is connected, expect a rule to be set up for user app MOCK_UID11 - mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange1, VPN_UID); + mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange1, VPN_UID, Set.of()); verify(mBpfNetMaps).addUidInterfaceRules(eq(ifName), aryEq(new int[]{MOCK_UID11})); reset(mBpfNetMaps); @@ -964,15 +964,15 @@ public class PermissionMonitorTest { // During VPN uid update (vpnRange1 -> vpnRange2), ConnectivityService first deletes the // old UID rules then adds the new ones. Expect netd to be updated - mPermissionMonitor.onVpnUidRangesRemoved(ifName, vpnRange1, VPN_UID); + mPermissionMonitor.onVpnUidRangesRemoved(ifName, vpnRange1, VPN_UID, Set.of()); verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[] {MOCK_UID11})); - mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange2, VPN_UID); + mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange2, VPN_UID, Set.of()); verify(mBpfNetMaps).addUidInterfaceRules(eq(ifName), aryEq(new int[]{MOCK_UID12})); reset(mBpfNetMaps); // When VPN is disconnected, expect rules to be torn down - mPermissionMonitor.onVpnUidRangesRemoved(ifName, vpnRange2, VPN_UID); + mPermissionMonitor.onVpnUidRangesRemoved(ifName, vpnRange2, VPN_UID, Set.of()); verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[] {MOCK_UID12})); } @@ -1000,7 +1000,7 @@ public class PermissionMonitorTest { onUserAddedWithInstalledPackageList(MOCK_USER2, pkgs); final Set vpnRange = Set.of(UidRange.createForUser(MOCK_USER1), UidRange.createForUser(MOCK_USER2)); - mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange, VPN_UID); + mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange, VPN_UID, Set.of()); // Newly-installed package should have uid rules added addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_APPID1); @@ -1025,6 +1025,36 @@ public class PermissionMonitorTest { doTestUidFilteringDuringPackageInstallAndUninstall(null /* ifName */); } + @Test + public void testUidFilteringWithDelegatedBypassUids() throws Exception { + final String ifName = "tun0"; + final int delegatedUid = MOCK_UID12; + final List pkgs = List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + CONNECTIVITY_USE_RESTRICTED_NETWORKS), + buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), + buildPackageInfo(MOCK_PACKAGE2, delegatedUid), + buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID)); + initialize(); + onUserAddedWithInstalledPackageList(MOCK_USER1, pkgs); + + final Set vpnRange = Set.of(UidRange.createForUser(MOCK_USER1)); + + // When VPN is connected, expect a rule to be set up for user app MOCK_UID11, but not for + // delegatedUid. + final Set delegatedUids = Set.of(delegatedUid); + mPermissionMonitor.onVpnUidRangesAdded(ifName, vpnRange, VPN_UID, delegatedUids); + verify(mBpfNetMaps).addUidInterfaceRules(eq(ifName), aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps, never()).addUidInterfaceRules(eq(ifName), + aryEq(new int[]{delegatedUid})); + + // When VPN is disconnected, expect rules to be torn down for MOCK_UID11 and still no rule + // for delegatedUid. + mPermissionMonitor.onVpnUidRangesRemoved(ifName, vpnRange, VPN_UID, delegatedUids); + verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps, never()).removeUidInterfaceRules(aryEq(new int[]{delegatedUid})); + } + @Test @EnableCompatChanges(RESTRICT_LOCAL_NETWORK) public void testLockdownUidFilteringWithLockdownEnableDisable() { diff --git a/tests/unit/java/com/android/server/connectivityservice/CSCaptivePortalAppTest.kt b/tests/unit/java/com/android/server/connectivityservice/CSCaptivePortalAppTest.kt index b66fcc9dac..74555ccf61 100644 --- a/tests/unit/java/com/android/server/connectivityservice/CSCaptivePortalAppTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/CSCaptivePortalAppTest.kt @@ -30,17 +30,30 @@ import android.net.NetworkRequest import android.net.NetworkStack import android.os.Build import android.os.Bundle +import android.os.ConditionVariable +import android.os.OutcomeReceiver +import android.os.ServiceSpecificException import androidx.test.filters.SmallTest +import com.android.testutils.DevSdkIgnoreRule import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo import com.android.testutils.DevSdkIgnoreRunner import com.android.testutils.TestableNetworkCallback +import kotlin.test.DefaultAsserter.assertTrue import kotlin.test.assertEquals +import kotlin.test.assertTrue +import kotlin.test.fail +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyInt +import org.mockito.Mockito.inOrder import org.mockito.Mockito.never import org.mockito.Mockito.verify +private const val APP1_UID = 10001 +private const val APP2_UID = 10002 +private const val TIMEOUT_MS = 2_000L + @DevSdkIgnoreRunner.MonitorThreadLeak @RunWith(DevSdkIgnoreRunner::class) @SmallTest @@ -48,7 +61,113 @@ import org.mockito.Mockito.verify class CSCaptivePortalAppTest : CSTest() { private val WIFI_IFACE = "wifi0" private val TEST_REDIRECT_URL = "http://example.com/firstPath" - private val TIMEOUT_MS = 2_000L + + @get:Rule val ignoreRule = DevSdkIgnoreRule() + + private class FakeOutcomeReceiver : OutcomeReceiver { + private val mCv = ConditionVariable() + private var mError: E? = null + + override fun onResult(result: R) { + mCv.open() + } + + override fun onError(error: E) { + mError = error + mCv.open() + } + + fun awaitOutcome() { + assertTrue( + "OutcomeReceiver did not receive outcome after $TIMEOUT_MS ms", + mCv.block(TIMEOUT_MS) + ) + if (mError != null) { + fail("OutcomeReceiver got: " + mError!!.message) + } + } + } + + /** + * Helper extension function to reduce boilerplate in the test. + */ + private fun CaptivePortal.setDelegateUidAndAwait(uid: Int) { + val or = FakeOutcomeReceiver() + this.setDelegateUid(uid, CSTestExecutor, or) + or.awaitOutcome() + } + + @Test + @IgnoreUpTo(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) + fun testCaptivePortalApp_SetDelegateUid() { + val captivePortalCallback = TestableNetworkCallback() + val captivePortalRequest = NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_CAPTIVE_PORTAL).build() + cm.registerNetworkCallback(captivePortalRequest, captivePortalCallback) + val wifiAgent = Agent(WIFI_IFACE, TRANSPORT_WIFI, NET_CAPABILITY_INTERNET) + wifiAgent.connectWithCaptivePortal(TEST_REDIRECT_URL) + captivePortalCallback.expectAvailableCallbacksUnvalidated(wifiAgent) + + val signInIntent = startCaptivePortalApp(wifiAgent) + val captivePortal = signInIntent.getParcelableExtra( + EXTRA_CAPTIVE_PORTAL, + CaptivePortal::class.java + )!! + + val inOrder = inOrder(netd) + + // Add the UID and check that it's added to the bypass list. + captivePortal.setDelegateUidAndAwait(APP2_UID) + inOrder.verify(netd).networkAllowBypassVpnOnNetwork( + true /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + + // Remove the UID and check that it's removed from the list. + captivePortal.setDelegateUidAndAwait(android.os.Process.INVALID_UID) + inOrder.verify(netd).networkAllowBypassVpnOnNetwork( + false /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + + // Add the UID again. + captivePortal.setDelegateUidAndAwait(APP2_UID) + inOrder.verify(netd).networkAllowBypassVpnOnNetwork( + true /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + + // Add the UID again. Nothing should change. + captivePortal.setDelegateUidAndAwait(APP2_UID) + inOrder.verify(netd, never()).networkAllowBypassVpnOnNetwork( + false /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + inOrder.verify(netd, never()).networkAllowBypassVpnOnNetwork( + true /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + + // Add another UID again. The old UID should be removed and the new one added. + captivePortal.setDelegateUidAndAwait(APP1_UID) + inOrder.verify(netd).networkAllowBypassVpnOnNetwork( + false /* allow */, + APP2_UID, + wifiAgent.network.netId + ) + inOrder.verify(netd).networkAllowBypassVpnOnNetwork( + true /* allow */, + APP1_UID, + wifiAgent.network.netId + ) + + wifiAgent.disconnect() + } @Test fun testCaptivePortalApp_Reevaluate_Nopermission() {