From 89d79d2ff20175e7af572cb4a2b4f652ca9febc1 Mon Sep 17 00:00:00 2001
From: Tommy Webb <tommy@calyxinstitute.org>
Date: Wed, 15 Feb 2023 19:26:02 -0500
Subject: [PATCH] Fix: Firewall: NMS inverts default rule behavior

When setting a chain's firewall rules in NetworkManagementService, do
not submit supplied UIDs to ConnectivityManager#replaceFirewallChain
directly, as this does not consider what the actual rules are for those
UIDs. Instead, supply the keys from the rules chain, which deletes
default rules when it is updated via updateFirewallUidRuleLocked.

For example, if a given UID's rule is the default rule, and it is part
of the restricted chain, then the UID should be blocked, because the
restricted chain is an allowlist. Prior to this change, the rules for
UIDs are ignored when calling replaceFirewallChain, so the UID's mere
presence among the supplied UIDs causes it to be unexpectedly added
to the restricted mode allowlist.

Test: CtsHostsideNetworkTests
Change-Id: I0a71ad376bcfda05cea151144dfab9bec8e8b749
---
 .../java/com/android/server/NetworkManagementService.java | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/services/core/java/com/android/server/NetworkManagementService.java b/services/core/java/com/android/server/NetworkManagementService.java
index 73d9cc759b109..2aa93783c5018 100644
--- a/services/core/java/com/android/server/NetworkManagementService.java
+++ b/services/core/java/com/android/server/NetworkManagementService.java
@@ -1483,6 +1483,7 @@ public class NetworkManagementService extends INetworkManagementService.Stub {
     public void setFirewallUidRules(int chain, int[] uids, int[] rules) {
         enforceSystemUid();
         synchronized (mQuotaLock) {
+            final int[] applicableUidsForChain;
             synchronized (mRulesLock) {
                 SparseIntArray uidFirewallRules = getUidFirewallRulesLR(chain);
                 SparseIntArray newRules = new SparseIntArray();
@@ -1506,10 +1507,15 @@ public class NetworkManagementService extends INetworkManagementService.Stub {
                     int uid = rulesToRemove.keyAt(index);
                     updateFirewallUidRuleLocked(chain, uid, FIREWALL_RULE_DEFAULT);
                 }
+                // Copy the keys for the firewall rules chain, which is guaranteed not to include
+                // default rules. We must not include default rules in the UIDs we send to
+                // ConnectivityManager#replaceFirewallChain, as this would have the opposite effect
+                // intended, leading such UIDs to be blocked or allowed erroneously.
+                applicableUidsForChain = uidFirewallRules.copyKeys();
             }
             final ConnectivityManager cm = mContext.getSystemService(ConnectivityManager.class);
             try {
-                cm.replaceFirewallChain(chain, uids);
+                cm.replaceFirewallChain(chain, applicableUidsForChain);
             } catch (RuntimeException e) {
                 Slog.w(TAG, "Error flushing firewall chain " + chain, e);
             }