Run firewalld as a regular user, in a PID namespace.
Now that we have ambient capabilities, we don't need to run firewalld
as root.
BUG=chromium:715678
TEST=platform_Firewall
TEST=readlink /proc/`pgrep firewalld`/ns/pid
TEST=pid:[4026532158]
TEST=readlink /proc/1/ns/pid
TEST=pid:[4026531836]
CQ-DEPEND=CL:494127
Change-Id: I5e65c56886e8d57bb261edb171ff16dd931d7f1d
Reviewed-on: https://chromium-review.googlesource.com/488701
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
diff --git a/dbus/org.chromium.Firewalld.conf b/dbus/org.chromium.Firewalld.conf
index 000b74e..82da784 100644
--- a/dbus/org.chromium.Firewalld.conf
+++ b/dbus/org.chromium.Firewalld.conf
@@ -2,7 +2,7 @@
"-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
<busconfig>
- <policy user="root">
+ <policy user="firewall">
<allow own="org.chromium.Firewalld"/>
</policy>
diff --git a/firewalld.conf b/firewalld.conf
index bfa521a..d7d9b28 100644
--- a/firewalld.conf
+++ b/firewalld.conf
@@ -21,4 +21,5 @@
stop on stopping system-services
respawn
-exec firewalld
+# Keep CAP_NET_ADMIN|CAP_NET_RAW, run in a new PID/mount namespace.
+exec /sbin/minijail0 -u firewall -g firewall -pvr -n -c 3000 --ambient -- /usr/bin/firewalld
diff --git a/iptables.cc b/iptables.cc
index 93d2f95..13bb697 100644
--- a/iptables.cc
+++ b/iptables.cc
@@ -35,9 +35,6 @@
const char kIPv4[] = "IPv4";
const char kIPv6[] = "IPv6";
-const uint64_t kIpTablesCapMask =
- CAP_TO_MASK(CAP_NET_ADMIN) | CAP_TO_MASK(CAP_NET_RAW);
-
// Interface names must be shorter than 'IFNAMSIZ' chars.
// See http://man7.org/linux/man-pages/man7/netdevice.7.html
// 'IFNAMSIZ' is 16 in recent kernels.
@@ -333,7 +330,7 @@
argv.push_back("-w"); // Wait for xtables lock.
// Use CAP_NET_ADMIN|CAP_NET_RAW.
- return ExecvNonRoot(argv, kIpTablesCapMask) == 0;
+ return RunInMinijail(argv) == 0;
}
bool IpTables::DeleteAcceptRule(const std::string& executable_path,
@@ -357,7 +354,7 @@
argv.push_back("-w"); // Wait for xtables lock.
// Use CAP_NET_ADMIN|CAP_NET_RAW.
- return ExecvNonRoot(argv, kIpTablesCapMask) == 0;
+ return RunInMinijail(argv) == 0;
}
bool IpTables::ApplyMasqueradeWithExecutable(const std::string& interface,
@@ -378,7 +375,7 @@
argv.push_back("-w"); // Wait for xtables lock
// Use CAP_NET_ADMIN|CAP_NET_RAW.
- if (ExecvNonRoot(argv, kIpTablesCapMask) != 0) {
+ if (RunInMinijail(argv)) {
LOG(ERROR) << (add ? "Adding" : "Removing")
<< " masquerade failed for interface " << interface
<< " using '" << executable_path << "'";
@@ -405,7 +402,7 @@
argv.push_back("--clamp-mss-to-pmtu");
argv.push_back("-w"); // Wait for xtables lock
- if (ExecvNonRoot(argv, kIpTablesCapMask) != 0) {
+ if (RunInMinijail(argv)) {
LOG(ERROR) << (add ? "Adding" : "Removing")
<< " tcpmss rule failed for interface " << interface
<< " using '" << executable_path << "'";
@@ -434,7 +431,7 @@
argv.push_back("-w"); // Wait for xtables lock
// Use CAP_NET_ADMIN|CAP_NET_RAW.
- bool success = ExecvNonRoot(argv, kIpTablesCapMask) == 0;
+ bool success = RunInMinijail(argv) == 0;
if (!success) {
LOG(ERROR) << (add ? "Adding" : "Removing")
@@ -457,7 +454,7 @@
argv.push_back("table");
argv.push_back(kTableIdForUserTraffic);
- bool success = ExecvNonRoot(argv, kIpTablesCapMask) == 0;
+ bool success = RunInMinijail(argv) == 0;
if (!success) {
LOG(ERROR) << (add ? "Adding" : "Removing")
@@ -468,16 +465,9 @@
return success;
}
-int IpTables::ExecvNonRoot(const std::vector<std::string>& argv,
- uint64_t capmask) {
+int IpTables::RunInMinijail(const std::vector<std::string>& argv) {
brillo::Minijail* m = brillo::Minijail::GetInstance();
minijail* jail = m->New();
-#if !defined(__ANDROID__)
- // TODO(garnold) This needs to be re-enabled once we figure out which
- // unprivileged user we want to use.
- CHECK(m->DropRoot(jail, kUnprivilegedUser, kUnprivilegedUser));
-#endif // __ANDROID__
- m->UseCapabilities(jail, capmask);
std::vector<char*> args;
for (const auto& arg : argv) {
diff --git a/iptables.h b/iptables.h
index beca79c..d53b90f 100644
--- a/iptables.h
+++ b/iptables.h
@@ -115,8 +115,9 @@
bool ApplyRuleForUserTrafficWithVersion(const std::string& ip_version,
bool add);
- virtual int ExecvNonRoot(const std::vector<std::string>& argv,
- uint64_t capmask);
+ // Even though firewalld runs as a regular user, it can still add other
+ // restrictions when launching 'iptables'.
+ virtual int RunInMinijail(const std::vector<std::string>& argv);
// Keep track of firewall holes to avoid adding redundant firewall rules.
std::set<Hole> tcp_holes_;
diff --git a/iptables_unittest.cc b/iptables_unittest.cc
index bb3cff6..41319a4 100644
--- a/iptables_unittest.cc
+++ b/iptables_unittest.cc
@@ -31,19 +31,19 @@
protected:
void SetMockExpectations(MockIpTables* iptables, bool success) {
// Empty criterion matches all commands.
- iptables->SetExecvNonRootFailCriterion(std::vector<std::string>(),
- true, success);
+ iptables->SetRunInMinijailFailCriterion(std::vector<std::string>(),
+ true, success);
}
void SetMockExpectationsPerExecutable(MockIpTables* iptables,
bool ip4_success,
bool ip6_success) {
if (!ip4_success)
- iptables->SetExecvNonRootFailCriterion(
+ iptables->SetRunInMinijailFailCriterion(
std::vector<std::string>({kIpTablesPath}), true /* repeat */,
false /* omit_failure */);
if (!ip6_success)
- iptables->SetExecvNonRootFailCriterion(
+ iptables->SetRunInMinijailFailCriterion(
std::vector<std::string>({kIp6TablesPath}), true, false);
}
@@ -54,7 +54,7 @@
TEST_F(IpTablesTest, Port0Fails) {
MockIpTables mock_iptables;
// Don't fail on anything.
- int id = mock_iptables.SetExecvNonRootFailCriterion(
+ int id = mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>(), true, true);
// Try to punch hole for TCP port 0, port 0 is not a valid port.
@@ -63,7 +63,7 @@
EXPECT_FALSE(mock_iptables.PunchUdpHole(0, "iface"));
// We should not be adding any rules for port 0.
- EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 0);
+ EXPECT_TRUE(mock_iptables.GetRunInMinijailCriterionMatchCount(id) == 0);
}
TEST_F(IpTablesTest, ValidInterfaceName) {
@@ -80,7 +80,7 @@
TEST_F(IpTablesTest, InvalidInterfaceName) {
MockIpTables mock_iptables;
- int id = mock_iptables.SetExecvNonRootFailCriterion(
+ int id = mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>(), true, true);
EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "reallylonginterfacename"));
@@ -100,7 +100,7 @@
EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "enddot."));
// We should not be adding any rules for invalid interface names.
- EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 0);
+ EXPECT_TRUE(mock_iptables.GetRunInMinijailCriterionMatchCount(id) == 0);
}
TEST_F(IpTablesTest, PunchTcpHoleSucceeds) {
@@ -198,16 +198,16 @@
MockIpTables mock_iptables;
// Fail on all commands issued for testuser1.
- int id = mock_iptables.SetExecvNonRootFailCriterion(
+ int id = mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({usernames[1], "-A"}), true, false);
- // Fail on all the calls to ExecvNonRoot issued by the
+ // Fail on all the calls to RunInMinijail issued by the
// attempt to apply marks for the last user.
ASSERT_FALSE(
mock_iptables.ApplyVpnSetup(usernames, interface, add));
EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
// Should have failed only on the first testuser1 command.
- EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
+ EXPECT_TRUE(mock_iptables.GetRunInMinijailCriterionMatchCount(id) == 1);
}
TEST_F(IpTablesTest, ApplyVpnSetupAdd_FailureInMasquerade) {
@@ -219,12 +219,12 @@
MockIpTables mock_iptables;
// Fail on calls adding MASQUERADE.
- int id = mock_iptables.SetExecvNonRootFailCriterion(
+ int id = mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"MASQUERADE", "-A"}), true, false);
ASSERT_FALSE(
mock_iptables.ApplyVpnSetup(usernames, interface, add));
EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
- EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
+ EXPECT_TRUE(mock_iptables.GetRunInMinijailCriterionMatchCount(id) == 1);
}
TEST_F(IpTablesTest, ApplyVpnSetupAdd_FailureInRuleForUserTraffic) {
@@ -233,11 +233,11 @@
const bool add = true;
MockIpTables mock_iptables;
- int id = mock_iptables.SetExecvNonRootFailCriterion(
+ int id = mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"rule", "add", "fwmark"}), true, false);
ASSERT_FALSE(mock_iptables.ApplyVpnSetup(usernames, interface, add));
EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
- EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
+ EXPECT_TRUE(mock_iptables.GetRunInMinijailCriterionMatchCount(id) == 1);
}
TEST_F(IpTablesTest, ApplyVpnSetupRemove_Success) {
@@ -249,11 +249,11 @@
// The fail criterions ensure that the ApplyVpnSetup function will delete
// the rules that are added. In addition to this, test that when all of
// these commands succeed, ApplyVpnSetup returns true.
- mock_iptables.SetExecvNonRootFailCriterion(
+ mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"MASQUERADE", "-D"}), true, true);
- mock_iptables.SetExecvNonRootFailCriterion(
+ mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"--uid-owner", "-D"}), true, true);
- mock_iptables.SetExecvNonRootFailCriterion(
+ mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"rule", "delete", "fwmark"}), true, true);
ASSERT_TRUE(mock_iptables.ApplyVpnSetup(usernames, interface, remove));
@@ -266,9 +266,9 @@
MockIpTables mock_iptables;
// Make all removing commands fail.
- mock_iptables.SetExecvNonRootFailCriterion(
+ mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"delete"}), true, false);
- mock_iptables.SetExecvNonRootFailCriterion(
+ mock_iptables.SetRunInMinijailFailCriterion(
std::vector<std::string>({"-D"}), true, false);
ASSERT_FALSE(mock_iptables.ApplyVpnSetup(usernames, interface, remove));
}
diff --git a/mock_iptables.cc b/mock_iptables.cc
index 8fe817b..e9a43cf 100644
--- a/mock_iptables.cc
+++ b/mock_iptables.cc
@@ -22,14 +22,14 @@
PlugAllHoles();
}
-int MockIpTables::SetExecvNonRootFailCriterion(
+int MockIpTables::SetRunInMinijailFailCriterion(
const std::vector<std::string>& keywords, bool repeat,
bool omit_failure) {
match_criteria_.push_back(Criterion{keywords, repeat, omit_failure, 0});
return match_criteria_.size() - 1;
}
-int MockIpTables::GetExecvNonRootCriterionMatchCount(int id) {
+int MockIpTables::GetRunInMinijailCriterionMatchCount(int id) {
if (id < 0 || id >= (int) match_criteria_.size())
return -1;
return match_criteria_[id].match_count;
@@ -38,7 +38,7 @@
bool MockIpTables::MatchAndUpdate(const std::vector<std::string>& argv) {
for (auto& criterion : match_criteria_) {
bool match = true;
- // Empty criterion is a catch all -- fail on any ExecvNonRoot.
+ // Empty criterion is a catch all -- fail on any RunInMinijail.
for (const std::string& keyword : criterion.keywords) {
match = match &&
(std::find(argv.begin(), argv.end(), keyword) != argv.end());
@@ -59,8 +59,7 @@
return false;
}
-int MockIpTables::ExecvNonRoot(
- const std::vector<std::string>& argv, uint64_t capmask) {
+int MockIpTables::RunInMinijail(const std::vector<std::string>& argv) {
if (MatchAndUpdate(argv))
return 1;
commands_.push_back(argv);
diff --git a/mock_iptables.h b/mock_iptables.h
index cb0b5df..ae88aa8 100644
--- a/mock_iptables.h
+++ b/mock_iptables.h
@@ -29,9 +29,9 @@
MockIpTables() = default;
~MockIpTables() override;
- int SetExecvNonRootFailCriterion(
+ int SetRunInMinijailFailCriterion(
const std::vector<std::string>& keywords, bool repeat, bool omit_failure);
- int GetExecvNonRootCriterionMatchCount(int id);
+ int GetRunInMinijailCriterionMatchCount(int id);
bool CheckCommandsUndone();
// Check if the current command matches a failure rule,
@@ -65,12 +65,12 @@
// must have in order for it to fail.
std::vector<Criterion> match_criteria_;
- // A log of commands issued with ExecvNonRoot during the test.
+ // A log of commands issued with RunInMinijail during the test.
std::vector<std::vector<std::string>> commands_;
// The mock's implementation simply logs the command issued if
// successful.
- int ExecvNonRoot(const std::vector<std::string>& argv, uint64_t capmask) override;
+ int RunInMinijail(const std::vector<std::string>& argv) override;
// Given an ip/iptables command, return the command that undoes it's effect.
std::vector<std::string> GetInverseCommand(const std::vector<std::string>& command);
