Move the last StrictController command to iptables-restore
Bug: 28362720
Test: unit tests pass
Change-Id: I8a4d2b8ea66799c6c3205b00f04ee1999fc7c68b
diff --git a/server/StrictController.cpp b/server/StrictController.cpp
index eae523c..ea25b2e 100644
--- a/server/StrictController.cpp
+++ b/server/StrictController.cpp
@@ -33,7 +33,6 @@
#include "NetdConstants.h"
#include "StrictController.h"
-auto StrictController::execIptables = ::execIptables;
auto StrictController::execIptablesRestore = ::execIptablesRestore;
const char* StrictController::LOCAL_OUTPUT = "st_OUTPUT";
@@ -42,6 +41,7 @@
const char* StrictController::LOCAL_PENALTY_LOG = "st_penalty_log";
const char* StrictController::LOCAL_PENALTY_REJECT = "st_penalty_reject";
+using android::base::Join;
using android::base::StringPrintf;
StrictController::StrictController(void) {
@@ -129,8 +129,8 @@
CMD_V4V6("-A %s -p udp -j %s", LOCAL_CLEAR_DETECT, LOCAL_CLEAR_CAUGHT);
CMD_V4V6("COMMIT\n");
- res |= execIptablesRestore(V4, android::base::Join(v4, '\n'));
- res |= execIptablesRestore(V6, android::base::Join(v6, '\n'));
+ res |= execIptablesRestore(V4, Join(v4, '\n'));
+ res |= execIptablesRestore(V6, Join(v6, '\n'));
#undef CMD_V4
#undef CMD_V6
@@ -151,44 +151,38 @@
CLEAR_CHAIN(LOCAL_CLEAR_DETECT),
"COMMIT\n"
};
- const std::string commands = android::base::Join(commandList, '\n');
+ const std::string commands = Join(commandList, '\n');
return execIptablesRestore(V4V6, commands);
#undef CLEAR_CHAIN
}
int StrictController::setUidCleartextPenalty(uid_t uid, StrictPenalty penalty) {
- char uidStr[16];
- sprintf(uidStr, "%d", uid);
-
- int res = 0;
+ std::vector<std::string> commands;
if (penalty == ACCEPT) {
// Clean up any old rules
- execIptables(V4V6, "-D", LOCAL_OUTPUT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_CLEAR_DETECT, NULL);
- execIptables(V4V6, "-D", LOCAL_CLEAR_CAUGHT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_PENALTY_LOG, NULL);
- execIptables(V4V6, "-D", LOCAL_CLEAR_CAUGHT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_PENALTY_REJECT, NULL);
-
+ commands = {
+ "*filter",
+ StringPrintf("-D %s -m owner --uid-owner %d -j %s",
+ LOCAL_OUTPUT, uid, LOCAL_CLEAR_DETECT),
+ StringPrintf("-D %s -m owner --uid-owner %d -j %s",
+ LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_LOG),
+ StringPrintf("-D %s -m owner --uid-owner %d -j %s",
+ LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_REJECT),
+ };
} else {
// Always take a detour to investigate this UID
- res |= execIptables(V4V6, "-I", LOCAL_OUTPUT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_CLEAR_DETECT, NULL);
-
+ commands.push_back("*filter");
+ commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
+ LOCAL_OUTPUT, uid, LOCAL_CLEAR_DETECT));
if (penalty == LOG) {
- res |= execIptables(V4V6, "-I", LOCAL_CLEAR_CAUGHT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_PENALTY_LOG, NULL);
+ commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
+ LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_LOG));
} else if (penalty == REJECT) {
- res |= execIptables(V4V6, "-I", LOCAL_CLEAR_CAUGHT,
- "-m", "owner", "--uid-owner", uidStr,
- "-j", LOCAL_PENALTY_REJECT, NULL);
+ commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
+ LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_REJECT));
}
}
+ commands.push_back("COMMIT\n");
- return res;
+ return execIptablesRestore(V4V6, Join(commands, "\n"));
}
diff --git a/server/StrictController.h b/server/StrictController.h
index 660c9ea..aee7c7e 100644
--- a/server/StrictController.h
+++ b/server/StrictController.h
@@ -45,7 +45,6 @@
protected:
// For testing.
friend class StrictControllerTest;
- static int (*execIptables)(IptablesTarget target, ...);
static int (*execIptablesRestore)(IptablesTarget target, const std::string& commands);
};
diff --git a/server/StrictControllerTest.cpp b/server/StrictControllerTest.cpp
index 3783c30..82d0cda 100644
--- a/server/StrictControllerTest.cpp
+++ b/server/StrictControllerTest.cpp
@@ -29,7 +29,6 @@
class StrictControllerTest : public IptablesBaseTest {
public:
StrictControllerTest() {
- StrictController::execIptables = fakeExecIptables;
StrictController::execIptablesRestore = fakeExecIptablesRestore;
}
StrictController mStrictCtrl;
@@ -125,28 +124,38 @@
TEST_F(StrictControllerTest, TestSetUidCleartextPenalty) {
std::vector<std::string> acceptCommands = {
- "-D st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect",
- "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log",
- "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject",
+ "*filter\n"
+ "-D st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
+ "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log\n"
+ "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject\n"
+ "COMMIT\n"
};
std::vector<std::string> logCommands = {
- "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect",
- "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log",
+ "*filter\n"
+ "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
+ "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log\n"
+ "COMMIT\n"
};
std::vector<std::string> rejectCommands = {
- "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect",
- "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject",
+ "*filter\n"
+ "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
+ "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject\n"
+ "COMMIT\n"
};
mStrictCtrl.setUidCleartextPenalty(12345, LOG);
- expectIptablesCommands(logCommands);
+ expectIptablesRestoreCommands(logCommands);
mStrictCtrl.setUidCleartextPenalty(12345, ACCEPT);
- expectIptablesCommands(acceptCommands);
+ expectIptablesRestoreCommands(acceptCommands);
+ // StrictController doesn't keep any state and it is not correct to call its methods in the
+ // wrong order (e.g., to go from LOG to REJECT without passing through ACCEPT).
+ // NetworkManagementService does keep state (not just to ensure correctness, but also so it can
+ // reprogram the rules when netd crashes).
mStrictCtrl.setUidCleartextPenalty(12345, REJECT);
- expectIptablesCommands(rejectCommands);
+ expectIptablesRestoreCommands(rejectCommands);
mStrictCtrl.setUidCleartextPenalty(12345, ACCEPT);
- expectIptablesCommands(acceptCommands);
+ expectIptablesRestoreCommands(acceptCommands);
}