Merge changes Ifbd15bf9,I985e6861,I54860c7c into nyc-dev
* changes:
Make FirewallController::createChain use replaceUidChain.
Make firewallReplaceUidChain match the behaviour of createChain.
Don't crash the test if expecting more commands than were run.
diff --git a/server/FirewallController.cpp b/server/FirewallController.cpp
index 7bb883d..0a06b9d 100644
--- a/server/FirewallController.cpp
+++ b/server/FirewallController.cpp
@@ -290,59 +290,44 @@
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);
+ std::vector<int32_t> uids;
+ return replaceUidChain(childChain, type == WHITELIST, uids);
+}
+
+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.
- 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::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 commands;
-
- StringAppendF(&commands, "*filter\n:%s -\n", name);
+ 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 +337,7 @@
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
--- a/server/FirewallController.h
+++ b/server/FirewallController.h
@@ -83,7 +83,8 @@
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..c1226b2 100644
--- a/server/FirewallControllerTest.cpp
+++ b/server/FirewallControllerTest.cpp
@@ -22,6 +22,8 @@
#include <gtest/gtest.h>
+#include <android-base/strings.h>
+
#include "FirewallController.h"
#include "IptablesBaseTest.h"
@@ -35,46 +37,73 @@
}
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);
}
};
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<std::string> 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<std::string> 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<std::pair<IptablesTarget, std::string>> 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<std::string> expectedRestore = {
+ "*filter",
+ ":fw_blacklist -",
+ "-A fw_blacklist -p tcp --tcp-flags RST RST -j RETURN",
+ "COMMIT\n\x04"
+ };
+ std::vector<std::pair<IptablesTarget, std::string>> 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) {
@@ -109,6 +138,13 @@
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 +157,19 @@
"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/server/IptablesBaseTest.cpp b/server/IptablesBaseTest.cpp
index 5ce6667..1502c4b 100644
--- a/server/IptablesBaseTest.cpp
+++ b/server/IptablesBaseTest.cpp
@@ -70,12 +70,20 @@
int IptablesBaseTest::expectIptablesCommand(IptablesTarget target, int pos,
const std::string& cmd) {
+
+ if ((unsigned) pos >= sCmds.size()) {
+ ADD_FAILURE() << "Expected too many iptables commands, want command "
+ << pos + 1 << "/" << sCmds.size();
+ return -1;
+ }
+
if (target == V4 || target == V4V6) {
EXPECT_EQ("/system/bin/iptables -w " + cmd, sCmds[pos++]);
}
if (target == V6 || target == V4V6) {
EXPECT_EQ("/system/bin/ip6tables -w " + cmd, sCmds[pos++]);
}
+
return target == V4V6 ? 2 : 1;
}
@@ -92,7 +100,12 @@
for (size_t i = 0; i < expectedCmds.size(); i ++) {
auto target = expectedCmds[i].first;
auto cmd = expectedCmds[i].second;
- pos += expectIptablesCommand(target, pos, cmd);
+ int numConsumed = expectIptablesCommand(target, pos, cmd);
+ if (numConsumed < 0) {
+ // Read past the end of the array.
+ break;
+ }
+ pos += numConsumed;
}
EXPECT_EQ(pos, sCmds.size());
diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp
index bdc147a..20bfc29 100644
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -140,15 +140,15 @@
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 @@
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";