Make firewallReplaceUidChain match the behaviour of createChain.

The behaviour of the firewallReplaceUidChain was incorrect in
several ways:

1. It was missing the "always allow TCP RST packets" rules which
   were added in http://ag/963000 .
2. It included a RETURN statement at the end of blacklist chains,
   which is superfluous since all user-defined chains implicitly
   return, and became incorrect when http://ag/963000 switched the
   behaviour of blacklist chains from inserting new rules at the
   beginning to appending them at the end.
3. It was missing the rules to allow the types of ICMPv6 packets
   that are critical in maintaining connectivity.

By itself, this change is a no-op since nothing currently calls
firewallReplaceUidRule.

Bug: 26675191
Change-Id: I985e6861812908cbe7eaf0f54ca0ad39c22bbfeb

diff --git a/server/FirewallController.cpp b/server/FirewallController.cpp
index 7bb883d..2d2e3b6 100644 (file)
--- a/server/FirewallController.cpp
+++ b/server/FirewallController.cpp
@@ -321,28 +321,39 @@ int FirewallController::createChain(const char* childChain,
     return res;
 }
 
-std::string FirewallController::makeUidRules(
-        const char *name, bool isWhitelist, const std::vector<int32_t>& uids) {
-    const char *action = isWhitelist ? "RETURN" : "DROP";
-    const char *defaultAction = isWhitelist ? "DROP" : "RETURN";
-
+std::string FirewallController::makeUidRules(IptablesTarget target, const char *name,
+        bool isWhitelist, const std::vector<int32_t>& uids) {
     std::string commands;
-
     StringAppendF(&commands, "*filter\n:%s -\n", name);
 
+    // Allow TCP RSTs so we can cleanly close TCP connections of apps that no longer have network
+    // access. Both incoming and outgoing RSTs are allowed.
+    StringAppendF(&commands, "-A %s -p tcp --tcp-flags RST RST -j RETURN\n", name);
+
     if (isWhitelist) {
+        // Allow ICMPv6 packets necessary to make IPv6 connectivity work. http://b/23158230 .
+        if (target == V6) {
+            for (size_t i = 0; i < ARRAY_SIZE(ICMPV6_TYPES); i++) {
+                StringAppendF(&commands, "-A %s -p icmpv6 --icmpv6-type %s -j RETURN\n",
+                       name, ICMPV6_TYPES[i]);
+            }
+        }
+
         // Always whitelist system UIDs.
         StringAppendF(&commands,
-                "-A %s -m owner --uid-owner %d-%d -j %s\n", name, 0, MAX_SYSTEM_UID, action);
+                "-A %s -m owner --uid-owner %d-%d -j RETURN\n", name, 0, MAX_SYSTEM_UID);
     }
 
+    // Whitelist or blacklist the specified UIDs.
+    const char *action = isWhitelist ? "RETURN" : "DROP";
     for (auto uid : uids) {
         StringAppendF(&commands, "-A %s -m owner --uid-owner %d -j %s\n", name, uid, action);
     }
 
-    // If it's a blacklist chain that blacklists nothing, then don't add a default action.
-    if (isWhitelist || uids.size() > 0) {
-        StringAppendF(&commands, "-A %s -j %s\n", name, defaultAction);
+    // If it's a whitelist chain, add a default DROP at the end. This is not necessary for a
+    // blacklist chain, because all user-defined chains implicitly RETURN at the end.
+    if (isWhitelist) {
+        StringAppendF(&commands, "-A %s -j DROP\n", name);
     }
 
     StringAppendF(&commands, "COMMIT\n\x04");  // EOT.
@@ -352,6 +363,7 @@ std::string FirewallController::makeUidRules(
 
 int FirewallController::replaceUidChain(
         const char *name, bool isWhitelist, const std::vector<int32_t>& uids) {
-   std::string commands = makeUidRules(name, isWhitelist, uids);
-   return execIptablesRestore(V4V6, commands.c_str());
+   std::string commands4 = makeUidRules(V4, name, isWhitelist, uids);
+   std::string commands6 = makeUidRules(V6, name, isWhitelist, uids);
+   return execIptablesRestore(V4, commands4.c_str()) | execIptablesRestore(V6, commands6.c_str());
 }

diff --git a/server/FirewallController.h b/server/FirewallController.h
index 0854c20..d78b461 100644 (file)
--- a/server/FirewallController.h
+++ b/server/FirewallController.h
@@ -83,7 +83,8 @@ public:
 
 protected:
     friend class FirewallControllerTest;
-    std::string makeUidRules(const char *name, bool isWhitelist, const std::vector<int32_t>& uids);
+    std::string makeUidRules(IptablesTarget target, const char *name, bool isWhitelist,
+                             const std::vector<int32_t>& uids);
     static int (*execIptables)(IptablesTarget target, ...);
     static int (*execIptablesSilently)(IptablesTarget target, ...);
     static int (*execIptablesRestore)(IptablesTarget target, const std::string& commands);

diff --git a/server/FirewallControllerTest.cpp b/server/FirewallControllerTest.cpp
index b909833..7e3686b 100644 (file)
--- a/server/FirewallControllerTest.cpp
+++ b/server/FirewallControllerTest.cpp
@@ -35,11 +35,12 @@ protected:
     }
     FirewallController mFw;
 
-    std::string makeUidRules(const char *a, bool b, const std::vector<int32_t>& c) {
-        return mFw.makeUidRules(a, b, c);
+    std::string makeUidRules(IptablesTarget a, const char* b, bool c,
+                             const std::vector<int32_t>& d) {
+        return mFw.makeUidRules(a, b, c, d);
     }
 
-    int createChain(const char* a, const char*b , FirewallType c) {
+    int createChain(const char* a, const char* b , FirewallType c) {
         return mFw.createChain(a, b, c);
     }
 };
@@ -109,6 +110,13 @@ TEST_F(FirewallControllerTest, TestReplaceWhitelistUidRule) {
     std::string expected =
             "*filter\n"
             ":FW_whitechain -\n"
+            "-A FW_whitechain -p tcp --tcp-flags RST RST -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type packet-too-big -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type router-solicitation -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type router-advertisement -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type neighbour-solicitation -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type neighbour-advertisement -j RETURN\n"
+            "-A FW_whitechain -p icmpv6 --icmpv6-type redirect -j RETURN\n"
             "-A FW_whitechain -m owner --uid-owner 0-9999 -j RETURN\n"
             "-A FW_whitechain -m owner --uid-owner 10023 -j RETURN\n"
             "-A FW_whitechain -m owner --uid-owner 10059 -j RETURN\n"
@@ -121,19 +129,19 @@ TEST_F(FirewallControllerTest, TestReplaceWhitelistUidRule) {
             "COMMIT\n\x04";
 
     std::vector<int32_t> uids = { 10023, 10059, 10124, 10111, 110122, 210153, 210024 };
-    EXPECT_EQ(expected, makeUidRules("FW_whitechain", true, uids));
+    EXPECT_EQ(expected, makeUidRules(V6, "FW_whitechain", true, uids));
 }
 
 TEST_F(FirewallControllerTest, TestReplaceBlacklistUidRule) {
     std::string expected =
             "*filter\n"
             ":FW_blackchain -\n"
+            "-A FW_blackchain -p tcp --tcp-flags RST RST -j RETURN\n"
             "-A FW_blackchain -m owner --uid-owner 10023 -j DROP\n"
             "-A FW_blackchain -m owner --uid-owner 10059 -j DROP\n"
             "-A FW_blackchain -m owner --uid-owner 10124 -j DROP\n"
-            "-A FW_blackchain -j RETURN\n"
             "COMMIT\n\x04";
 
     std::vector<int32_t> uids = { 10023, 10059, 10124 };
-    EXPECT_EQ(expected, makeUidRules("FW_blackchain", false, uids));
+    EXPECT_EQ(expected, makeUidRules(V4 ,"FW_blackchain", false, uids));
 }

diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp
index bdc147a..20bfc29 100644 (file)
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -140,15 +140,15 @@ TEST_F(BinderTest, TestFirewallReplaceUidChain) {
         mNetd->firewallReplaceUidChain(String16(chainName.c_str()), true, uids, &ret);
     }
     EXPECT_EQ(true, ret);
-    EXPECT_EQ((int) uids.size() + 4, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
-    EXPECT_EQ((int) uids.size() + 4, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
+    EXPECT_EQ((int) uids.size() + 5, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
+    EXPECT_EQ((int) uids.size() + 11, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
     {
         TimedOperation op("Clearing whitelist chain");
         mNetd->firewallReplaceUidChain(String16(chainName.c_str()), false, noUids, &ret);
     }
     EXPECT_EQ(true, ret);
-    EXPECT_EQ(2, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
-    EXPECT_EQ(2, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
+    EXPECT_EQ(3, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
+    EXPECT_EQ(3, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
 
     {
         TimedOperation op(StringPrintf("Programming %d-UID blacklist chain", kNumUids));
@@ -163,8 +163,8 @@ TEST_F(BinderTest, TestFirewallReplaceUidChain) {
         mNetd->firewallReplaceUidChain(String16(chainName.c_str()), false, noUids, &ret);
     }
     EXPECT_EQ(true, ret);
-    EXPECT_EQ(2, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
-    EXPECT_EQ(2, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
+    EXPECT_EQ(3, iptablesRuleLineLength(IPTABLES_PATH, chainName.c_str()));
+    EXPECT_EQ(3, iptablesRuleLineLength(IP6TABLES_PATH, chainName.c_str()));
 
     // Check that the call fails if iptables returns an error.
     std::string veryLongStringName = "netd_binder_test_UnacceptablyLongIptablesChainName";