From 8b42e6d431ba7d76d583038f6a11fb185bc0df86 Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Thu, 19 May 2022 06:23:40 +0000 Subject: [PATCH 1/2] Remove LOCKDOWN from FirewallChain IntDef LOCKDOWN_VPN was in the FirewallChain IntDef but this was not a right place because LOCKDOWN_VPN was not a valid value for Connectivity APIs that take an argument annotated with @FirewallChain(setUidFirewallRule, setFirewallChainEnabled, replaceFirewallChain). LOCKDOWN_VPN was in the FirewallChain IntDef because BpfNetMaps#setUidRule was used to add/remove LOCKDOWN_VPN entries. This commit adds BpfNetMaps#updateUidLockdownRule and uses this to add/remove LOCKDOWN_VPN entries instead of BpfNetMaps#setUidRule and removes LOCKDOWN from FirewallChain. Bug: 206482423 Test: atest TrafficControllerTest ConnectivityServiceTest PermissionMonitorTest HostsideVpnTests#testBlockIncomingPacket Change-Id: Iff9b9792fc0f208f153e10e396c6d5034b412d7c --- .../src/android/net/ConnectivityManager.java | 11 ---- service/jni/com_android_server_BpfNetMaps.cpp | 8 +++ service/native/TrafficController.cpp | 17 +++++-- service/native/TrafficControllerTest.cpp | 19 +++++-- service/native/include/Common.h | 1 - service/native/include/TrafficController.h | 2 + .../src/com/android/server/BpfNetMaps.java | 14 ++++++ .../connectivity/PermissionMonitor.java | 9 +--- .../server/ConnectivityServiceTest.java | 13 ++--- .../connectivity/PermissionMonitorTest.java | 50 +++++-------------- 10 files changed, 70 insertions(+), 74 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index 5769b92642..d1c202cac3 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -982,16 +982,6 @@ public class ConnectivityManager { @SystemApi(client = MODULE_LIBRARIES) public static final int FIREWALL_CHAIN_LOW_POWER_STANDBY = 5; - /** - * Firewall chain used for lockdown VPN. - * Denylist of apps that cannot receive incoming packets except on loopback because they are - * subject to an always-on VPN which is not currently connected. - * - * @see #BLOCKED_REASON_LOCKDOWN_VPN - * @hide - */ - public static final int FIREWALL_CHAIN_LOCKDOWN_VPN = 6; - /** * Firewall chain used for OEM-specific application restrictions. * Denylist of apps that will not have network access due to OEM-specific restrictions. @@ -1021,7 +1011,6 @@ public class ConnectivityManager { FIREWALL_CHAIN_POWERSAVE, FIREWALL_CHAIN_RESTRICTED, FIREWALL_CHAIN_LOW_POWER_STANDBY, - FIREWALL_CHAIN_LOCKDOWN_VPN, FIREWALL_CHAIN_OEM_DENY_1, FIREWALL_CHAIN_OEM_DENY_2, FIREWALL_CHAIN_OEM_DENY_3 diff --git a/service/jni/com_android_server_BpfNetMaps.cpp b/service/jni/com_android_server_BpfNetMaps.cpp index bc70c93469..2780044ec6 100644 --- a/service/jni/com_android_server_BpfNetMaps.cpp +++ b/service/jni/com_android_server_BpfNetMaps.cpp @@ -151,6 +151,12 @@ static jint native_removeUidInterfaceRules(JNIEnv* env, jobject self, jintArray return (jint)status.code(); } +static jint native_updateUidLockdownRule(JNIEnv* env, jobject self, jint uid, jboolean add) { + Status status = mTc.updateUidLockdownRule(uid, add); + CHECK_LOG(status); + return (jint)status.code(); +} + static jint native_swapActiveStatsMap(JNIEnv* env, jobject self) { Status status = mTc.swapActiveStatsMap(); CHECK_LOG(status); @@ -203,6 +209,8 @@ static const JNINativeMethod gMethods[] = { (void*)native_addUidInterfaceRules}, {"native_removeUidInterfaceRules", "([I)I", (void*)native_removeUidInterfaceRules}, + {"native_updateUidLockdownRule", "(IZ)I", + (void*)native_updateUidLockdownRule}, {"native_swapActiveStatsMap", "()I", (void*)native_swapActiveStatsMap}, {"native_setPermissionForUids", "(I[I)V", diff --git a/service/native/TrafficController.cpp b/service/native/TrafficController.cpp index d05e6fa21b..8ad42c7c0b 100644 --- a/service/native/TrafficController.cpp +++ b/service/native/TrafficController.cpp @@ -340,8 +340,6 @@ FirewallType TrafficController::getFirewallType(ChildChain chain) { return ALLOWLIST; case LOW_POWER_STANDBY: return ALLOWLIST; - case LOCKDOWN: - return DENYLIST; case OEM_DENY_1: return DENYLIST; case OEM_DENY_2: @@ -373,9 +371,6 @@ int TrafficController::changeUidOwnerRule(ChildChain chain, uid_t uid, FirewallR case LOW_POWER_STANDBY: res = updateOwnerMapEntry(LOW_POWER_STANDBY_MATCH, uid, rule, type); break; - case LOCKDOWN: - res = updateOwnerMapEntry(LOCKDOWN_VPN_MATCH, uid, rule, type); - break; case OEM_DENY_1: res = updateOwnerMapEntry(OEM_DENY_1_MATCH, uid, rule, type); break; @@ -447,6 +442,18 @@ Status TrafficController::removeUidInterfaceRules(const std::vector& ui return netdutils::status::ok; } +Status TrafficController::updateUidLockdownRule(const uid_t uid, const bool add) { + std::lock_guard guard(mMutex); + + netdutils::Status result = add ? addRule(uid, LOCKDOWN_VPN_MATCH) + : removeRule(uid, LOCKDOWN_VPN_MATCH); + if (!isOk(result)) { + ALOGW("%s Lockdown rule failed(%d): uid=%d", + (add ? "add": "remove"), result.code(), uid); + } + return result; +} + int TrafficController::replaceUidOwnerMap(const std::string& name, bool isAllowlist __unused, const std::vector& uids) { // FirewallRule rule = isAllowlist ? ALLOW : DENY; diff --git a/service/native/TrafficControllerTest.cpp b/service/native/TrafficControllerTest.cpp index 1aca633644..0b4550e227 100644 --- a/service/native/TrafficControllerTest.cpp +++ b/service/native/TrafficControllerTest.cpp @@ -215,7 +215,7 @@ class TrafficControllerTest : public ::testing::Test { checkEachUidValue(uids, match); } - void expectUidOwnerMapValues(const std::vector& appUids, uint8_t expectedRule, + void expectUidOwnerMapValues(const std::vector& appUids, uint32_t expectedRule, uint32_t expectedIif) { for (uint32_t uid : appUids) { Result value = mFakeUidOwnerMap.readValue(uid); @@ -389,7 +389,6 @@ TEST_F(TrafficControllerTest, TestChangeUidOwnerRule) { checkUidOwnerRuleForChain(POWERSAVE, POWERSAVE_MATCH); checkUidOwnerRuleForChain(RESTRICTED, RESTRICTED_MATCH); checkUidOwnerRuleForChain(LOW_POWER_STANDBY, LOW_POWER_STANDBY_MATCH); - checkUidOwnerRuleForChain(LOCKDOWN, LOCKDOWN_VPN_MATCH); checkUidOwnerRuleForChain(OEM_DENY_1, OEM_DENY_1_MATCH); checkUidOwnerRuleForChain(OEM_DENY_2, OEM_DENY_2_MATCH); checkUidOwnerRuleForChain(OEM_DENY_3, OEM_DENY_3_MATCH); @@ -521,6 +520,21 @@ TEST_F(TrafficControllerTest, TestRemoveUidInterfaceFilteringRules) { expectMapEmpty(mFakeUidOwnerMap); } +TEST_F(TrafficControllerTest, TestUpdateUidLockdownRule) { + // Add Lockdown rules + ASSERT_TRUE(isOk(mTc.updateUidLockdownRule(1000, true /* add */))); + ASSERT_TRUE(isOk(mTc.updateUidLockdownRule(1001, true /* add */))); + expectUidOwnerMapValues({1000, 1001}, LOCKDOWN_VPN_MATCH, 0); + + // Remove one of Lockdown rules + ASSERT_TRUE(isOk(mTc.updateUidLockdownRule(1000, false /* add */))); + expectUidOwnerMapValues({1001}, LOCKDOWN_VPN_MATCH, 0); + + // Remove remaining Lockdown rule + ASSERT_TRUE(isOk(mTc.updateUidLockdownRule(1001, false /* add */))); + expectMapEmpty(mFakeUidOwnerMap); +} + TEST_F(TrafficControllerTest, TestUidInterfaceFilteringRulesCoexistWithExistingMatches) { // Set up existing PENALTY_BOX_MATCH rules ASSERT_TRUE(isOk(updateUidOwnerMaps({1000, 1001, 10012}, PENALTY_BOX_MATCH, @@ -802,7 +816,6 @@ TEST_F(TrafficControllerTest, getFirewallType) { {POWERSAVE, ALLOWLIST}, {RESTRICTED, ALLOWLIST}, {LOW_POWER_STANDBY, ALLOWLIST}, - {LOCKDOWN, DENYLIST}, {OEM_DENY_1, DENYLIST}, {OEM_DENY_2, DENYLIST}, {INVALID_CHAIN, DENYLIST}, diff --git a/service/native/include/Common.h b/service/native/include/Common.h index 2427aa906c..3f28991bea 100644 --- a/service/native/include/Common.h +++ b/service/native/include/Common.h @@ -35,7 +35,6 @@ enum ChildChain { POWERSAVE = 3, RESTRICTED = 4, LOW_POWER_STANDBY = 5, - LOCKDOWN = 6, OEM_DENY_1 = 7, OEM_DENY_2 = 8, OEM_DENY_3 = 9, diff --git a/service/native/include/TrafficController.h b/service/native/include/TrafficController.h index c019ce78b0..c921ff2df0 100644 --- a/service/native/include/TrafficController.h +++ b/service/native/include/TrafficController.h @@ -71,6 +71,8 @@ class TrafficController { EXCLUDES(mMutex); netdutils::Status removeUidInterfaceRules(const std::vector& uids) EXCLUDES(mMutex); + netdutils::Status updateUidLockdownRule(const uid_t uid, const bool add) EXCLUDES(mMutex); + netdutils::Status updateUidOwnerMap(const uint32_t uid, UidOwnerMatchType matchType, IptOp op) EXCLUDES(mMutex); diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java index c006bc605f..151d0e3da4 100644 --- a/service/src/com/android/server/BpfNetMaps.java +++ b/service/src/com/android/server/BpfNetMaps.java @@ -215,6 +215,19 @@ public class BpfNetMaps { maybeThrow(err, "Unable to remove uid interface rules"); } + /** + * Update lockdown rule for uid + * + * @param uid target uid to add/remove the rule + * @param add {@code true} to add the rule, {@code false} to remove the rule. + * @throws ServiceSpecificException in case of failure, with an error code indicating the + * cause of the failure. + */ + public void updateUidLockdownRule(final int uid, final boolean add) { + final int err = native_updateUidLockdownRule(uid, add); + maybeThrow(err, "Unable to update lockdown rule"); + } + /** * Request netd to change the current active network stats map. * @@ -271,6 +284,7 @@ public class BpfNetMaps { private native int native_setUidRule(int childChain, int uid, int firewallRule); private native int native_addUidInterfaceRules(String ifName, int[] uids); private native int native_removeUidInterfaceRules(int[] uids); + private native int native_updateUidLockdownRule(int uid, boolean add); private native int native_swapActiveStatsMap(); private native void native_setPermissionForUids(int permissions, int[] uids); private native void native_dump(FileDescriptor fd, boolean verbose); diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index e4a2c20bd5..10db088a53 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -23,9 +23,6 @@ import static android.Manifest.permission.NETWORK_STACK; import static android.Manifest.permission.UPDATE_DEVICE_STATS; import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; -import static android.net.ConnectivityManager.FIREWALL_CHAIN_LOCKDOWN_VPN; -import static android.net.ConnectivityManager.FIREWALL_RULE_ALLOW; -import static android.net.ConnectivityManager.FIREWALL_RULE_DENY; import static android.net.ConnectivitySettingsManager.UIDS_ALLOWED_ON_RESTRICTED_NETWORKS; import static android.net.INetd.PERMISSION_INTERNET; import static android.net.INetd.PERMISSION_NETWORK; @@ -1079,11 +1076,7 @@ public class PermissionMonitor { private void updateLockdownUidRule(int uid, boolean add) { try { - if (add) { - mBpfNetMaps.setUidRule(FIREWALL_CHAIN_LOCKDOWN_VPN, uid, FIREWALL_RULE_DENY); - } else { - mBpfNetMaps.setUidRule(FIREWALL_CHAIN_LOCKDOWN_VPN, uid, FIREWALL_RULE_ALLOW); - } + mBpfNetMaps.updateUidLockdownRule(uid, add); } catch (ServiceSpecificException e) { loge("Failed to " + (add ? "add" : "remove") + " Lockdown rule: " + e); } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index e84df162e1..b0b07505b1 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -52,7 +52,6 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.EXTRA_NETWORK_INFO; import static android.net.ConnectivityManager.EXTRA_NETWORK_TYPE; import static android.net.ConnectivityManager.FIREWALL_CHAIN_DOZABLE; -import static android.net.ConnectivityManager.FIREWALL_CHAIN_LOCKDOWN_VPN; import static android.net.ConnectivityManager.FIREWALL_CHAIN_LOW_POWER_STANDBY; import static android.net.ConnectivityManager.FIREWALL_CHAIN_OEM_DENY_1; import static android.net.ConnectivityManager.FIREWALL_CHAIN_OEM_DENY_2; @@ -9531,10 +9530,8 @@ public class ConnectivityServiceTest { waitForIdle(); // Lockdown rule is set to apps uids - verify(mBpfNetMaps).setUidRule( - eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(APP1_UID), eq(FIREWALL_RULE_DENY)); - verify(mBpfNetMaps).setUidRule( - eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(APP2_UID), eq(FIREWALL_RULE_DENY)); + verify(mBpfNetMaps).updateUidLockdownRule(APP1_UID, true /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(APP2_UID, true /* add */); reset(mBpfNetMaps); @@ -9544,10 +9541,8 @@ public class ConnectivityServiceTest { waitForIdle(); // Lockdown rule is removed from apps uids - verify(mBpfNetMaps).setUidRule( - eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(APP1_UID), eq(FIREWALL_RULE_ALLOW)); - verify(mBpfNetMaps).setUidRule( - eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(APP2_UID), eq(FIREWALL_RULE_ALLOW)); + verify(mBpfNetMaps).updateUidLockdownRule(APP1_UID, false /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(APP2_UID, false /* add */); // Interface rules are not changed by Lockdown mode enable/disable verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index ecd17bab69..7716522fea 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -30,9 +30,6 @@ 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.net.ConnectivityManager.FIREWALL_CHAIN_LOCKDOWN_VPN; -import static android.net.ConnectivityManager.FIREWALL_RULE_ALLOW; -import static android.net.ConnectivityManager.FIREWALL_RULE_DENY; import static android.net.ConnectivitySettingsManager.UIDS_ALLOWED_ON_RESTRICTED_NETWORKS; import static android.net.INetd.PERMISSION_INTERNET; import static android.net.INetd.PERMISSION_NETWORK; @@ -872,19 +869,14 @@ public class PermissionMonitorTest { // Add Lockdown uid range, expect a rule to be set up for user app MOCK_UID11 mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange1); - verify(mBpfNetMaps) - .setUidRule( - eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_DENY)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange1)); reset(mBpfNetMaps); // Remove Lockdown uid range, expect rules to be torn down mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange1); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_ALLOW)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @@ -902,9 +894,7 @@ public class PermissionMonitorTest { // Add Lockdown uid range at 1st time, expect a rule to be set up mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_DENY)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); reset(mBpfNetMaps); @@ -912,7 +902,7 @@ public class PermissionMonitorTest { // Add Lockdown uid range at 2nd time, expect a rule not to be set up because the uid // already has the rule mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange); - verify(mBpfNetMaps, never()).setUidRule(anyInt(), anyInt(), anyInt()); + verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); reset(mBpfNetMaps); @@ -920,7 +910,7 @@ public class PermissionMonitorTest { // Remove Lockdown uid range at 1st time, expect a rule not to be torn down because we added // the range 2 times. mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); - verify(mBpfNetMaps, never()).setUidRule(anyInt(), anyInt(), anyInt()); + verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); reset(mBpfNetMaps); @@ -928,9 +918,7 @@ public class PermissionMonitorTest { // Remove Lockdown uid range at 2nd time, expect a rule to be torn down because we added // twice and we removed twice. mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_ALLOW)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @@ -949,9 +937,7 @@ public class PermissionMonitorTest { // Add Lockdown uid ranges which contains duplicated uid ranges mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRangeDuplicates); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_DENY)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); reset(mBpfNetMaps); @@ -959,16 +945,14 @@ public class PermissionMonitorTest { // Remove Lockdown uid range at 1st time, expect a rule not to be torn down because uid // ranges we added contains duplicated uid ranges. mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); - verify(mBpfNetMaps, never()).setUidRule(anyInt(), anyInt(), anyInt()); + verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); reset(mBpfNetMaps); // Remove Lockdown uid range at 2nd time, expect a rule to be torn down. mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_ALLOW)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @@ -989,23 +973,15 @@ public class PermissionMonitorTest { // Installing package should add Lockdown rules addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_APPID1); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_DENY)); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID21), - eq(FIREWALL_RULE_DENY)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID21, true /* add */); reset(mBpfNetMaps); // Uninstalling package should remove Lockdown rules mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - verify(mBpfNetMaps) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID11), - eq(FIREWALL_RULE_ALLOW)); - verify(mBpfNetMaps, never()) - .setUidRule(eq(FIREWALL_CHAIN_LOCKDOWN_VPN), eq(MOCK_UID21), - eq(FIREWALL_RULE_ALLOW)); + verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); + verify(mBpfNetMaps, never()).updateUidLockdownRule(MOCK_UID21, false /* add */); } // Normal package add/remove operations will trigger multiple intent for uids corresponding to From f8bd82cd6737408990acdd864bcd235e5db6beac Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Mon, 30 May 2022 12:28:04 +0000 Subject: [PATCH 2/2] Refactor code and improve tests for VPN filtering Bug: 206482423 Test: atest ConnectivityServiceTest PermissionMonitorTest Change-Id: Ic6ff7a3d7695ad6ce96764a9bab2c0a641ba2ba6 --- .../android/server/ConnectivityService.java | 22 ++-- .../connectivity/PermissionMonitor.java | 10 +- .../server/ConnectivityServiceTest.java | 98 +++++++---------- .../connectivity/PermissionMonitorTest.java | 101 +++++++++++------- 4 files changed, 116 insertions(+), 115 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 848901fdec..853a1a2782 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -7760,10 +7760,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // when the old rules are removed and the time when new rules are added. To fix this, // make eBPF support two allowlisted interfaces so here new rules can be added before the // old rules are being removed. - - // Null iface given to onVpnUidRangesAdded/Removed is a wildcard to allow apps to receive - // packets on all interfaces. This is required to accept incoming traffic in Lockdown mode - // by overriding the Lockdown blocking rule. if (wasFiltering) { mPermissionMonitor.onVpnUidRangesRemoved(oldIface, ranges, vpnAppUid); } @@ -8089,12 +8085,14 @@ public class ConnectivityService extends IConnectivityManager.Stub * Returns whether we need to set interface filtering rule or not */ private boolean requiresVpnAllowRule(NetworkAgentInfo nai, LinkProperties lp, - String filterIface) { - // Only filter if lp has an interface. - if (lp == null || lp.getInterfaceName() == null) return false; - // Before T, allow rules are only needed if VPN isolation is enabled. - // T and After T, allow rules are needed for all VPNs. - return filterIface != null || (nai.isVPN() && SdkLevel.isAtLeastT()); + String isolationIface) { + // Allow rules are always needed if VPN isolation is enabled. + if (isolationIface != null) return true; + + // On T and above, allow rules are needed for all VPNs. Allow rule with null iface is a + // wildcard to allow apps to receive packets on all interfaces. This is required to accept + // incoming traffic in Lockdown mode by overriding the Lockdown blocking rule. + return SdkLevel.isAtLeastT() && nai.isVPN() && lp != null && lp.getInterfaceName() != null; } private static UidRangeParcel[] toUidRangeStableParcels(final @NonNull Set ranges) { @@ -8237,10 +8235,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // above, where the addition of new ranges happens before the removal of old ranges. // TODO Fix this window by computing an accurate diff on Set, so the old range // to be removed will never overlap with the new range to be added. - - // Null iface given to onVpnUidRangesAdded/Removed is a wildcard to allow apps to - // receive packets on all interfaces. This is required to accept incoming traffic in - // Lockdown mode by overriding the Lockdown blocking rule. if (wasFiltering && !prevRanges.isEmpty()) { mPermissionMonitor.onVpnUidRangesRemoved(oldIface, prevRanges, prevNc.getOwnerUid()); diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index 10db088a53..dedeb380ea 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -681,8 +681,12 @@ public class PermissionMonitor { } private synchronized void updateLockdownUid(int uid, boolean add) { - if (UidRange.containsUid(mVpnLockdownUidRanges.getSet(), uid) - && !hasRestrictedNetworksPermission(uid)) { + // Apps that can use restricted networks can always bypass VPNs. + if (hasRestrictedNetworksPermission(uid)) { + return; + } + + if (UidRange.containsUid(mVpnLockdownUidRanges.getSet(), uid)) { updateLockdownUidRule(uid, add); } } @@ -1252,7 +1256,7 @@ public class PermissionMonitor { pw.println("Lockdown filtering rules:"); pw.increaseIndent(); for (final UidRange range : mVpnLockdownUidRanges.getSet()) { - pw.println("UIDs: " + range.toString()); + pw.println("UIDs: " + range); } pw.decreaseIndent(); diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index b0b07505b1..900ee5a2d7 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -9515,34 +9515,28 @@ public class ConnectivityServiceTest { @Test @IgnoreUpTo(Build.VERSION_CODES.S_V2) public void testLockdownSetFirewallUidRule() throws Exception { - // For ConnectivityService#setAlwaysOnVpnPackage. - mServiceContext.setPermission( - Manifest.permission.CONTROL_ALWAYS_ON_VPN, PERMISSION_GRANTED); - // Needed to call Vpn#setAlwaysOnPackage. - mServiceContext.setPermission(Manifest.permission.CONTROL_VPN, PERMISSION_GRANTED); - // Needed to call Vpn#isAlwaysOnPackageSupported. - mServiceContext.setPermission(NETWORK_SETTINGS, PERMISSION_GRANTED); - + final Set> lockdownRange = UidRange.toIntRanges(Set.of(PRIMARY_UIDRANGE)); // Enable Lockdown - final ArrayList allowList = new ArrayList<>(); - mVpnManagerService.setAlwaysOnVpnPackage(PRIMARY_USER, ALWAYS_ON_PACKAGE, - true /* lockdown */, allowList); + mCm.setRequireVpnForUids(true /* requireVpn */, lockdownRange); waitForIdle(); // Lockdown rule is set to apps uids + verify(mBpfNetMaps, times(3)).updateUidLockdownRule(anyInt(), eq(true) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(APP1_UID, true /* add */); verify(mBpfNetMaps).updateUidLockdownRule(APP2_UID, true /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(VPN_UID, true /* add */); reset(mBpfNetMaps); // Disable lockdown - mVpnManagerService.setAlwaysOnVpnPackage(PRIMARY_USER, null, false /* lockdown */, - allowList); + mCm.setRequireVpnForUids(false /* requireVPN */, lockdownRange); waitForIdle(); // Lockdown rule is removed from apps uids + verify(mBpfNetMaps, times(3)).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(APP1_UID, false /* add */); verify(mBpfNetMaps).updateUidLockdownRule(APP2_UID, false /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(VPN_UID, false /* add */); // Interface rules are not changed by Lockdown mode enable/disable verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); @@ -10523,27 +10517,28 @@ public class ConnectivityServiceTest { assertNull(mService.mPermissionMonitor.getVpnInterfaceUidRanges("tun0")); } - @Test - public void testLegacyVpnInterfaceFilteringRule() throws Exception { - LinkProperties lp = new LinkProperties(); - lp.setInterfaceName("tun0"); - lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null)); - lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), null)); + private void checkInterfaceFilteringRuleWithNullInterface(final LinkProperties lp, + final int uid) throws Exception { // 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, Process.SYSTEM_UID, vpnRange); - assertVpnUidRangesUpdated(true, vpnRange, Process.SYSTEM_UID); + mMockVpn.establish(lp, uid, vpnRange); + assertVpnUidRangesUpdated(true, vpnRange, uid); if (SdkLevel.isAtLeastT()) { - // On T and above, A connected Legacy VPN should have interface rules with null - // interface. Null Interface is a wildcard and this accepts traffic from all the - // interfaces. There are two expected invocations, one during the VPN initial + // On T and above, VPN should have rules for null interface. Null Interface is a + // wildcard and this accepts traffic from all the interfaces. + // There are two expected invocations, one during the VPN initial // connection, one during the VPN LinkProperties update. ArgumentCaptor uidCaptor = ArgumentCaptor.forClass(int[].class); verify(mBpfNetMaps, times(2)).addUidInterfaceRules( eq(null) /* iface */, uidCaptor.capture()); - assertContainsExactly(uidCaptor.getAllValues().get(0), APP1_UID, APP2_UID, VPN_UID); - assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID, VPN_UID); + if (uid == VPN_UID) { + assertContainsExactly(uidCaptor.getAllValues().get(0), APP1_UID, APP2_UID); + assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID); + } else { + assertContainsExactly(uidCaptor.getAllValues().get(0), APP1_UID, APP2_UID, VPN_UID); + assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID, VPN_UID); + } assertEquals(mService.mPermissionMonitor.getVpnInterfaceUidRanges(null /* iface */), vpnRange); @@ -10552,50 +10547,37 @@ public class ConnectivityServiceTest { // Disconnected VPN should have interface rules removed verify(mBpfNetMaps).removeUidInterfaceRules(uidCaptor.capture()); - assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID, VPN_UID); + if (uid == VPN_UID) { + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID); + } else { + assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID, VPN_UID); + } assertNull(mService.mPermissionMonitor.getVpnInterfaceUidRanges(null /* iface */)); } else { - // Before T, Legacy VPN should not have interface rules. + // Before T, rules are not configured for null interface. verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); } } + @Test + public void testLegacyVpnInterfaceFilteringRule() throws Exception { + LinkProperties lp = new LinkProperties(); + lp.setInterfaceName("tun0"); + lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null)); + lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), null)); + // Legacy VPN should have interface filtering with null interface. + checkInterfaceFilteringRuleWithNullInterface(lp, Process.SYSTEM_UID); + } + @Test public void testLocalIpv4OnlyVpnInterfaceFilteringRule() throws Exception { LinkProperties lp = new LinkProperties(); lp.setInterfaceName("tun0"); lp.addRoute(new RouteInfo(new IpPrefix("192.0.2.0/24"), null, "tun0")); 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, Process.SYSTEM_UID, vpnRange); - assertVpnUidRangesUpdated(true, vpnRange, Process.SYSTEM_UID); - - if (SdkLevel.isAtLeastT()) { - // IPv6 unreachable route should not be misinterpreted as a default route - // On T and above, A connected VPN that does not provide a default route should have - // interface rules with null interface. Null Interface is a wildcard and this accepts - // traffic from all the interfaces. There are two expected invocations, one during the - // VPN initial connection, one during the VPN LinkProperties update. - ArgumentCaptor uidCaptor = ArgumentCaptor.forClass(int[].class); - verify(mBpfNetMaps, times(2)).addUidInterfaceRules( - eq(null) /* iface */, uidCaptor.capture()); - assertContainsExactly(uidCaptor.getAllValues().get(0), APP1_UID, APP2_UID, VPN_UID); - assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID, VPN_UID); - assertEquals(mService.mPermissionMonitor.getVpnInterfaceUidRanges(null /* iface */), - vpnRange); - - mMockVpn.disconnect(); - waitForIdle(); - - // Disconnected VPN should have interface rules removed - verify(mBpfNetMaps).removeUidInterfaceRules(uidCaptor.capture()); - assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID, VPN_UID); - assertNull(mService.mPermissionMonitor.getVpnInterfaceUidRanges(null /* iface */)); - } else { - // Before T, VPN with IPv6 unreachable route should not have interface rules. - verify(mBpfNetMaps, never()).addUidInterfaceRules(any(), any()); - } + // VPN that does not provide a default route should have interface filtering with null + // interface. + checkInterfaceFilteringRuleWithNullInterface(lp, VPN_UID); } @Test diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index 7716522fea..354e79a4a1 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -695,7 +695,8 @@ public class PermissionMonitorTest { mNetdMonitor.expectNetworkPerm(PERMISSION_SYSTEM, new UserHandle[]{MOCK_USER1}, SYSTEM_APPID1); - final List pkgs = List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID21, + final List pkgs = List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID21, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(SYSTEM_PACKAGE2, SYSTEM_APP_UID21, CHANGE_NETWORK_STATE)); doReturn(pkgs).when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), @@ -761,9 +762,10 @@ public class PermissionMonitorTest { MOCK_APPID1); } - private void doTestuidFilteringDuringVpnConnectDisconnectAndUidUpdates(@Nullable String ifName) + private void doTestUidFilteringDuringVpnConnectDisconnectAndUidUpdates(@Nullable String ifName) throws Exception { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID12), @@ -771,7 +773,7 @@ public class PermissionMonitorTest { .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); buildAndMockPackageInfoWithPermissions(MOCK_PACKAGE1, MOCK_UID11); mPermissionMonitor.startMonitoring(); - // Every app on user 0 except MOCK_UID12 are under VPN. + // Every app on user 0 except MOCK_UID12 is subject to the VPN. final Set vpnRange1 = Set.of( new UidRange(0, MOCK_UID12 - 1), new UidRange(MOCK_UID12 + 1, UserHandle.PER_USER_RANGE - 1)); @@ -808,18 +810,19 @@ public class PermissionMonitorTest { @Test public void testUidFilteringDuringVpnConnectDisconnectAndUidUpdates() throws Exception { - doTestuidFilteringDuringVpnConnectDisconnectAndUidUpdates("tun0"); + doTestUidFilteringDuringVpnConnectDisconnectAndUidUpdates("tun0"); } @Test public void testUidFilteringDuringVpnConnectDisconnectAndUidUpdatesWithWildcard() throws Exception { - doTestuidFilteringDuringVpnConnectDisconnectAndUidUpdates(null /* ifName */); + doTestUidFilteringDuringVpnConnectDisconnectAndUidUpdates(null /* ifName */); } private void doTestUidFilteringDuringPackageInstallAndUninstall(@Nullable String ifName) throws Exception { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, NETWORK_STACK, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); @@ -854,125 +857,140 @@ public class PermissionMonitorTest { @Test public void testLockdownUidFilteringWithLockdownEnableDisable() { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID12), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); - // Every app on user 0 except MOCK_UID12 are under VPN. - final UidRange[] vpnRange1 = { + // Every app on user 0 except MOCK_UID12 is subject to the VPN. + final UidRange[] lockdownRange = { new UidRange(0, MOCK_UID12 - 1), new UidRange(MOCK_UID12 + 1, UserHandle.PER_USER_RANGE - 1) }; - // Add Lockdown uid range, expect a rule to be set up for user app MOCK_UID11 - mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange1); + // Add Lockdown uid range, expect a rule to be set up for MOCK_UID11 and VPN_UID + mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, lockdownRange); + verify(mBpfNetMaps, times(2)).updateUidLockdownRule(anyInt(), eq(true) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange1)); + verify(mBpfNetMaps).updateUidLockdownRule(VPN_UID, true /* add */); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Remove Lockdown uid range, expect rules to be torn down - mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange1); + mPermissionMonitor.updateVpnLockdownUidRanges(false /* add */, lockdownRange); + verify(mBpfNetMaps, times(2)).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); + verify(mBpfNetMaps).updateUidLockdownRule(VPN_UID, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @Test public void testLockdownUidFilteringWithLockdownEnableDisableWithMultiAdd() { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); - // MOCK_UID11 is under VPN. + // MOCK_UID11 is subject to the VPN. final UidRange range = new UidRange(MOCK_UID11, MOCK_UID11); - final UidRange[] vpnRange = {range}; + final UidRange[] lockdownRange = {range}; // Add Lockdown uid range at 1st time, expect a rule to be set up - mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, lockdownRange); + verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(true) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Add Lockdown uid range at 2nd time, expect a rule not to be set up because the uid // already has the rule - mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, lockdownRange); verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Remove Lockdown uid range at 1st time, expect a rule not to be torn down because we added // the range 2 times. - mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(false /* add */, lockdownRange); verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Remove Lockdown uid range at 2nd time, expect a rule to be torn down because we added // twice and we removed twice. - mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(false /* add */, lockdownRange); + verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @Test public void testLockdownUidFilteringWithLockdownEnableDisableWithDuplicates() { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); - // MOCK_UID11 is under VPN. + // MOCK_UID11 is subject to the VPN. final UidRange range = new UidRange(MOCK_UID11, MOCK_UID11); - final UidRange[] vpnRangeDuplicates = {range, range}; - final UidRange[] vpnRange = {range}; + final UidRange[] lockdownRangeDuplicates = {range, range}; + final UidRange[] lockdownRange = {range}; // Add Lockdown uid ranges which contains duplicated uid ranges - mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRangeDuplicates); + mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, lockdownRangeDuplicates); + verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(true) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Remove Lockdown uid range at 1st time, expect a rule not to be torn down because uid // ranges we added contains duplicated uid ranges. - mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(false /* add */, lockdownRange); verify(mBpfNetMaps, never()).updateUidLockdownRule(anyInt(), anyBoolean()); - assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(vpnRange)); + assertEquals(mPermissionMonitor.getVpnLockdownUidRanges(), Set.of(lockdownRange)); reset(mBpfNetMaps); // Remove Lockdown uid range at 2nd time, expect a rule to be torn down. - mPermissionMonitor.updateVpnLockdownUidRanges(false /* false */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(false /* add */, lockdownRange); + verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); assertTrue(mPermissionMonitor.getVpnLockdownUidRanges().isEmpty()); } @Test public void testLockdownUidFilteringWithInstallAndUnInstall() { - doReturn(List.of(buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, + doReturn(List.of( + buildPackageInfo(SYSTEM_PACKAGE1, SYSTEM_APP_UID11, CHANGE_NETWORK_STATE, NETWORK_STACK, CONNECTIVITY_USE_RESTRICTED_NETWORKS), buildPackageInfo(SYSTEM_PACKAGE2, VPN_UID))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); doReturn(List.of(MOCK_USER1, MOCK_USER2)).when(mUserManager).getUserHandles(eq(true)); mPermissionMonitor.startMonitoring(); - final UidRange[] vpnRange = { + final UidRange[] lockdownRange = { UidRange.createForUser(MOCK_USER1), UidRange.createForUser(MOCK_USER2) }; - mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, vpnRange); + mPermissionMonitor.updateVpnLockdownUidRanges(true /* add */, lockdownRange); + + reset(mBpfNetMaps); // Installing package should add Lockdown rules addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_APPID1); + verify(mBpfNetMaps, times(2)).updateUidLockdownRule(anyInt(), eq(true) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, true /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID21, true /* add */); @@ -980,8 +998,8 @@ public class PermissionMonitorTest { // Uninstalling package should remove Lockdown rules mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); + verify(mBpfNetMaps).updateUidLockdownRule(anyInt(), eq(false) /* add */); verify(mBpfNetMaps).updateUidLockdownRule(MOCK_UID11, false /* add */); - verify(mBpfNetMaps, never()).updateUidLockdownRule(MOCK_UID21, false /* add */); } // Normal package add/remove operations will trigger multiple intent for uids corresponding to @@ -1305,7 +1323,8 @@ public class PermissionMonitorTest { public void testOnExternalApplicationsAvailable() throws Exception { // Initial the permission state. MOCK_PACKAGE1 and MOCK_PACKAGE2 are installed on external // and have different uids. There has no permission for both uids. - doReturn(List.of(buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), + doReturn(List.of( + buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID12))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); @@ -1363,7 +1382,8 @@ public class PermissionMonitorTest { throws Exception { // Initial the permission state. MOCK_PACKAGE1 and MOCK_PACKAGE2 are installed on external // storage and shared on MOCK_UID11. There has no permission for MOCK_UID11. - doReturn(List.of(buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), + doReturn(List.of( + buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID11))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); @@ -1389,7 +1409,8 @@ public class PermissionMonitorTest { // Initial the permission state. MOCK_PACKAGE1 is installed on external storage and // MOCK_PACKAGE2 is installed on device. These two packages are shared on MOCK_UID11. // MOCK_UID11 has NETWORK and INTERNET permissions. - doReturn(List.of(buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), + doReturn(List.of( + buildPackageInfo(MOCK_PACKAGE1, MOCK_UID11), buildPackageInfo(MOCK_PACKAGE2, MOCK_UID11, CHANGE_NETWORK_STATE, INTERNET))) .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring();