From a55388e3f3dd726e470e195770649a2797d7e02f Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 13 May 2016 17:03:42 +0900 Subject: [PATCH] Make FirewallController::createChain use replaceUidChain. This has two benefits: 1. It makes the behaviour of setting firewall chains via the firewallReplaceUidChain RPC match the behaviour of creating the chains on boot. (As a side effect, it reduces code duplication between the two.) 2. It makes creating firewall chains on boot use iptables-restore, which is substantially faster than running iptables commands one at a time. This CL will allow the framework to switch to using firewallReplaceUidChain when the framework starts, providing substantial speedups over the current behaviour of running two iptables commands for every app that is whitelisted or idle. Bug: 26675191 Change-Id: Ifbd15bf9143efd526570dde8f88effc79d164630 --- server/FirewallController.cpp | 30 ++--------------- server/FirewallControllerTest.cpp | 68 +++++++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 48 deletions(-) diff --git a/server/FirewallController.cpp b/server/FirewallController.cpp index 2d2e3b6..0a06b9d 100644 --- a/server/FirewallController.cpp +++ b/server/FirewallController.cpp @@ -290,35 +290,9 @@ int FirewallController::detachChain(const char* childChain, const char* parentCh int FirewallController::createChain(const char* childChain, const char* parentChain, FirewallType type) { - // Order is important, otherwise later steps may fail. execIptablesSilently(V4V6, "-t", TABLE, "-D", parentChain, "-j", childChain, NULL); - execIptablesSilently(V4V6, "-t", TABLE, "-F", childChain, NULL); - execIptablesSilently(V4V6, "-t", TABLE, "-X", childChain, NULL); - int res = 0; - res |= execIptables(V4V6, "-t", TABLE, "-N", childChain, NULL); - - // 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. - res |= execIptables(V4V6, "-A", childChain, "-p", "tcp", - "--tcp-flags", "RST", "RST", "-j", "RETURN", NULL); - - if (type == WHITELIST) { - // Allow ICMPv6 packets necessary to make IPv6 connectivity work. http://b/23158230 . - for (size_t i = 0; i < ARRAY_SIZE(ICMPV6_TYPES); i++) { - res |= execIptables(V6, "-A", childChain, "-p", "icmpv6", "--icmpv6-type", - ICMPV6_TYPES[i], "-j", "RETURN", NULL); - } - - // create default white list for system uid range - char uidStr[16]; - sprintf(uidStr, "0-%d", MAX_SYSTEM_UID); - res |= execIptables(V4V6, "-A", childChain, "-m", "owner", "--uid-owner", - uidStr, "-j", "RETURN", NULL); - - // create default rule to drop all traffic - res |= execIptables(V4V6, "-A", childChain, "-j", "DROP", NULL); - } - return res; + std::vector uids; + return replaceUidChain(childChain, type == WHITELIST, uids); } std::string FirewallController::makeUidRules(IptablesTarget target, const char *name, diff --git a/server/FirewallControllerTest.cpp b/server/FirewallControllerTest.cpp index 7e3686b..c1226b2 100644 --- a/server/FirewallControllerTest.cpp +++ b/server/FirewallControllerTest.cpp @@ -22,6 +22,8 @@ #include +#include + #include "FirewallController.h" #include "IptablesBaseTest.h" @@ -47,35 +49,61 @@ protected: TEST_F(FirewallControllerTest, TestCreateWhitelistChain) { - ExpectedIptablesCommands expected = { + ExpectedIptablesCommands expectedCommands = { { V4V6, "-t filter -D INPUT -j fw_whitelist" }, - { V4V6, "-t filter -F fw_whitelist" }, - { V4V6, "-t filter -X fw_whitelist" }, - { V4V6, "-t filter -N fw_whitelist" }, - { V4V6, "-A fw_whitelist -p tcp --tcp-flags RST RST -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type packet-too-big -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type router-solicitation -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type router-advertisement -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type neighbour-solicitation -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type neighbour-advertisement -j RETURN" }, - { V6, "-A fw_whitelist -p icmpv6 --icmpv6-type redirect -j RETURN" }, - { V4V6, "-A fw_whitelist -m owner --uid-owner 0-9999 -j RETURN" }, - { V4V6, "-A fw_whitelist -j DROP" }, }; + + std::vector expectedRestore4 = { + "*filter", + ":fw_whitelist -", + "-A fw_whitelist -p tcp --tcp-flags RST RST -j RETURN", + "-A fw_whitelist -m owner --uid-owner 0-9999 -j RETURN", + "-A fw_whitelist -j DROP", + "COMMIT\n\x04" + }; + std::vector expectedRestore6 = { + "*filter", + ":fw_whitelist -", + "-A fw_whitelist -p tcp --tcp-flags RST RST -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type packet-too-big -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type router-solicitation -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type router-advertisement -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type neighbour-solicitation -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type neighbour-advertisement -j RETURN", + "-A fw_whitelist -p icmpv6 --icmpv6-type redirect -j RETURN", + "-A fw_whitelist -m owner --uid-owner 0-9999 -j RETURN", + "-A fw_whitelist -j DROP", + "COMMIT\n\x04" + }; + std::vector> expectedRestoreCommands = { + { V4, android::base::Join(expectedRestore4, '\n') }, + { V6, android::base::Join(expectedRestore6, '\n') }, + }; + createChain("fw_whitelist", "INPUT", WHITELIST); - expectIptablesCommands(expected); + expectIptablesCommands(expectedCommands); + expectIptablesRestoreCommands(expectedRestoreCommands); } TEST_F(FirewallControllerTest, TestCreateBlacklistChain) { - ExpectedIptablesCommands expected = { + ExpectedIptablesCommands expectedCommands = { { V4V6, "-t filter -D INPUT -j fw_blacklist" }, - { V4V6, "-t filter -F fw_blacklist" }, - { V4V6, "-t filter -X fw_blacklist" }, - { V4V6, "-t filter -N fw_blacklist" }, - { V4V6, "-A fw_blacklist -p tcp --tcp-flags RST RST -j RETURN" }, }; + + std::vector expectedRestore = { + "*filter", + ":fw_blacklist -", + "-A fw_blacklist -p tcp --tcp-flags RST RST -j RETURN", + "COMMIT\n\x04" + }; + std::vector> expectedRestoreCommands = { + { V4, android::base::Join(expectedRestore, '\n') }, + { V6, android::base::Join(expectedRestore, '\n') }, + }; + createChain("fw_blacklist", "INPUT", BLACKLIST); - expectIptablesCommands(expected); + expectIptablesCommands(expectedCommands); + expectIptablesRestoreCommands(expectedRestoreCommands); } TEST_F(FirewallControllerTest, TestSetStandbyRule) { -- 2.11.0