vpn: add support for spawning VPN client programs in minijail
Spawn VPN programs in a minijail if the --jail-vpn-clients command line
flag is passed. Also update unit tests.
Also make the openvpn config dir/file world-readable so it can be created
by shill user and accessed by openvpn user. If we think this file is
sensitive and shouldn't be world-readable then we could alternatively
make shill user belong to openvpn group and have shill chgrp() on the
file to openvpn. I didn't do that here for sake of simplicity but will
make that accomodation if anyone feels strongly about this.
BUG=chromium:649417
CQ-DEPEND=CL:1086231
TEST=unit tests pass, tested as part of larger sandbox shill debug CL
Change-Id: I1037c1e3b060e53a699411c197c5961f9cbc5527
Reviewed-on: https://chromium-review.googlesource.com/1087359
Commit-Ready: Micah Morton <mortonm@chromium.org>
Tested-by: Micah Morton <mortonm@chromium.org>
Reviewed-by: Micah Morton <mortonm@chromium.org>
diff --git a/mock_manager.h b/mock_manager.h
index 0a7cc2d..80d5338 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -97,6 +97,7 @@
MOCK_CONST_METHOD1(GetEnabledDeviceByLinkName,
DeviceRefPtr(const std::string& link_name));
MOCK_CONST_METHOD0(GetMinimumMTU, int());
+ MOCK_CONST_METHOD0(GetJailVpnClients, bool());
MOCK_CONST_METHOD1(ShouldAcceptHostnameFrom,
bool(const std::string& device_name));
MOCK_CONST_METHOD1(IsDHCPv6EnabledForDevice,
diff --git a/vpn/l2tp_ipsec_driver.cc b/vpn/l2tp_ipsec_driver.cc
index b8cfd0e..0394ac6 100644
--- a/vpn/l2tp_ipsec_driver.cc
+++ b/vpn/l2tp_ipsec_driver.cc
@@ -240,10 +240,23 @@
LOG(INFO) << "L2TP/IPSec VPN process options: "
<< base::JoinString(options, " ");
- if (external_task_local->Start(
- FilePath(kL2TPIPSecVPNPath), options, environment, true, error)) {
- external_task_ = std::move(external_task_local);
- return true;
+ if (manager()->GetJailVpnClients()) {
+ uint64_t capmask = CAP_TO_MASK(CAP_NET_ADMIN) | CAP_TO_MASK(CAP_NET_RAW) |
+ CAP_TO_MASK(CAP_NET_BIND_SERVICE) |
+ CAP_TO_MASK(CAP_SETUID) | CAP_TO_MASK(CAP_SETGID) |
+ CAP_TO_MASK(CAP_KILL);
+ if (external_task_local->StartInMinijail(FilePath(kL2TPIPSecVPNPath),
+ options, "shill", "shill", capmask,
+ true, error)) {
+ external_task_ = std::move(external_task_local);
+ return true;
+ }
+ } else {
+ if (external_task_local->Start(FilePath(kL2TPIPSecVPNPath), options,
+ environment, true, error)) {
+ external_task_ = std::move(external_task_local);
+ return true;
+ }
}
return false;
}
diff --git a/vpn/l2tp_ipsec_driver.h b/vpn/l2tp_ipsec_driver.h
index d23b4fe..90674e9 100644
--- a/vpn/l2tp_ipsec_driver.h
+++ b/vpn/l2tp_ipsec_driver.h
@@ -86,6 +86,7 @@
FRIEND_TEST(L2TPIPSecDriverTest, OnConnectionDisconnected);
FRIEND_TEST(L2TPIPSecDriverTest, OnL2TPIPSecVPNDied);
FRIEND_TEST(L2TPIPSecDriverTest, SpawnL2TPIPSecVPN);
+ FRIEND_TEST(L2TPIPSecDriverTest, SpawnL2TPIPSecVPNInMinijail);
static const char kL2TPIPSecVPNPath[];
static const Property kProperties[];
diff --git a/vpn/l2tp_ipsec_driver_unittest.cc b/vpn/l2tp_ipsec_driver_unittest.cc
index 47e32cd..f65df12 100644
--- a/vpn/l2tp_ipsec_driver_unittest.cc
+++ b/vpn/l2tp_ipsec_driver_unittest.cc
@@ -548,6 +548,10 @@
static const char kHost[] = "192.168.2.254";
SetArg(kProviderHostProperty, kHost);
+ EXPECT_CALL(manager_, GetJailVpnClients())
+ .Times(2)
+ .WillRepeatedly(Return(false));
+
// TODO(quiche): Instead of setting expectations based on what
// ExternalTask will call, mock out ExternalTask. Non-trivial,
// though, because ExternalTask is constructed during the
@@ -562,11 +566,41 @@
EXPECT_NE(nullptr, driver_->external_task_);
}
+TEST_F(L2TPIPSecDriverTest, SpawnL2TPIPSecVPNInMinijail) {
+ Error error;
+ // Fail without sufficient arguments.
+ EXPECT_FALSE(driver_->SpawnL2TPIPSecVPN(&error));
+ EXPECT_TRUE(error.IsFailure());
+
+ // Provide the required arguments.
+ static const char kHost[] = "192.168.2.254";
+ SetArg(kProviderHostProperty, kHost);
+
+ EXPECT_CALL(manager_, GetJailVpnClients())
+ .Times(2)
+ .WillRepeatedly(Return(true));
+
+ // TODO(mortonm): Instead of setting expectations based on what
+ // ExternalTask will call, mock out ExternalTask. Non-trivial,
+ // though, because ExternalTask is constructed during the
+ // call to driver_->Connect.
+ EXPECT_CALL(process_manager_, StartProcessInMinijail(_, _, _, _, _, _, _, _))
+ .WillOnce(Return(-1))
+ .WillOnce(Return(1));
+
+ EXPECT_FALSE(driver_->SpawnL2TPIPSecVPN(&error));
+ EXPECT_FALSE(driver_->external_task_);
+ EXPECT_TRUE(driver_->SpawnL2TPIPSecVPN(&error));
+ EXPECT_NE(nullptr, driver_->external_task_);
+}
+
TEST_F(L2TPIPSecDriverTest, Connect) {
EXPECT_CALL(*service_, SetState(Service::kStateConfiguring));
static const char kHost[] = "192.168.2.254";
SetArg(kProviderHostProperty, kHost);
+ EXPECT_CALL(manager_, GetJailVpnClients()).WillOnce(Return(false));
+
// TODO(quiche): Instead of setting expectations based on what
// ExternalTask will call, mock out ExternalTask. Non-trivial,
// though, because ExternalTask is constructed during the
diff --git a/vpn/openvpn_driver.cc b/vpn/openvpn_driver.cc
index 20dd83e..ca0a909 100644
--- a/vpn/openvpn_driver.cc
+++ b/vpn/openvpn_driver.cc
@@ -289,7 +289,14 @@
<< openvpn_config_directory_.value();
return false;
}
- if (chmod(openvpn_config_directory_.value().c_str(), S_IRWXU)) {
+ // OpenVPN running as user 'openvpn' needs access to the config directory,
+ // and openvpn user is not member of shill group so make the dir
+ // world-readable. We'd rather not have openvpn belong to shill group since
+ // shill is more privileged than openvpn, hence the idea of 'dropping'
+ // UID/GID from shill to openvpn. Moreover since shill no longer runs with
+ // CAP_CHOWN, we can't chown the dir to shill:openvpn.
+ if (chmod(openvpn_config_directory_.value().c_str(),
+ S_IRWXU | S_IRWXG | S_IROTH)) {
LOG(ERROR) << "Failed to set permissions on "
<< openvpn_config_directory_.value();
base::DeleteFile(openvpn_config_directory_, true);
@@ -301,7 +308,10 @@
contents.push_back('\n');
if (!base::CreateTemporaryFileInDir(openvpn_config_directory_, config_file) ||
base::WriteFile(*config_file, contents.data(), contents.size()) !=
- static_cast<int>(contents.size())) {
+ static_cast<int>(contents.size()) ||
+ chmod(config_file->value().c_str(), S_IRWXU | S_IRWXG | S_IROTH)) {
+ // Make the config file world-readable. Same rationale as listed above for
+ // the config directory.
LOG(ERROR) << "Unable to setup OpenVPN config file.";
return false;
}
@@ -313,6 +323,7 @@
vector<vector<string>> options;
Error error;
+ pid_t openvpn_pid;
InitOptions(&options, &error);
if (error.IsFailure()) {
return false;
@@ -329,18 +340,29 @@
vector<string> args = GetCommandLineArgs();
LOG(INFO) << "OpenVPN command line args: " << base::JoinString(args, " ");
- pid_t pid = process_manager_->StartProcess(
- FROM_HERE, FilePath(kOpenVPNPath),
- args,
- map<string, string>(), // No env vars passed.
- false, // Do not terminate with parent.
- base::Bind(&OpenVPNDriver::OnOpenVPNDied, base::Unretained(this)));
- if (pid < 0) {
- LOG(ERROR) << "Unable to spawn: " << kOpenVPNPath;
- return false;
+ if (manager()->GetJailVpnClients()) {
+ uint64_t capmask = CAP_TO_MASK(CAP_NET_ADMIN) | CAP_TO_MASK(CAP_NET_RAW) |
+ CAP_TO_MASK(CAP_SETUID) | CAP_TO_MASK(CAP_SETGID);
+ openvpn_pid = process_manager_->StartProcessInMinijail(
+ FROM_HERE, base::FilePath(kOpenVPNPath), args, "shill", "shill",
+ capmask, true,
+ base::Bind(&OpenVPNDriver::OnOpenVPNDied, base::Unretained(this)));
+ if (openvpn_pid == -1) {
+ LOG(ERROR) << "Minijail couldn't run our child process";
+ return false;
+ }
+ } else {
+ openvpn_pid = process_manager_->StartProcess(
+ FROM_HERE, FilePath(kOpenVPNPath), args,
+ map<string, string>(), // No env vars passed.
+ false, // Do not terminate with parent.
+ base::Bind(&OpenVPNDriver::OnOpenVPNDied, base::Unretained(this)));
+ if (openvpn_pid < 0) {
+ LOG(ERROR) << "Unable to spawn: " << kOpenVPNPath;
+ return false;
+ }
}
-
- pid_ = pid;
+ pid_ = openvpn_pid;
return true;
}
@@ -1054,7 +1076,7 @@
vector<string> OpenVPNDriver::GetCommandLineArgs() {
SLOG(this, 2) << __func__ << "(" << lsb_release_file_.value() << ")";
vector<string> args =
- vector<string>{"--config", openvpn_config_file_.value()};
+ vector<string>{"--config", openvpn_config_file_.value()};
string contents;
if (!base::ReadFileToString(lsb_release_file_, &contents)) {
LOG(ERROR) << "Unable to read the lsb-release file: "
diff --git a/vpn/openvpn_driver.h b/vpn/openvpn_driver.h
index 5ae9829..a727552 100644
--- a/vpn/openvpn_driver.h
+++ b/vpn/openvpn_driver.h
@@ -159,6 +159,7 @@
FRIEND_TEST(OpenVPNDriverTest, ParseRouteOption);
FRIEND_TEST(OpenVPNDriverTest, SetRoutes);
FRIEND_TEST(OpenVPNDriverTest, SpawnOpenVPN);
+ FRIEND_TEST(OpenVPNDriverTest, SpawnOpenVPNInMinijail);
FRIEND_TEST(OpenVPNDriverTest, SplitPortFromHost);
FRIEND_TEST(OpenVPNDriverTest, WriteConfigFile);
diff --git a/vpn/openvpn_driver_unittest.cc b/vpn/openvpn_driver_unittest.cc
index a045d3b..78d1175 100644
--- a/vpn/openvpn_driver_unittest.cc
+++ b/vpn/openvpn_driver_unittest.cc
@@ -1112,6 +1112,7 @@
SetArg(kProviderHostProperty, kHost);
EXPECT_CALL(*management_server_, Start(_, _, _)).WillOnce(Return(true));
EXPECT_CALL(manager_, IsConnected()).WillOnce(Return(false));
+ EXPECT_CALL(manager_, GetJailVpnClients()).WillOnce(Return(false));
EXPECT_CALL(
process_manager_,
StartProcess(_, _, _, _, false /* Don't exit with parent */, _))
@@ -1200,6 +1201,10 @@
.WillRepeatedly(Return(true));
EXPECT_CALL(manager_, IsConnected()).Times(2).WillRepeatedly(Return(false));
+ EXPECT_CALL(manager_, GetJailVpnClients())
+ .Times(2)
+ .WillRepeatedly(Return(false));
+
const int kPID = 234678;
const map<string, string> expected_env; // No env vars passed.
EXPECT_CALL(process_manager_, StartProcess(_, _, _, expected_env, _, _))
@@ -1210,6 +1215,33 @@
EXPECT_EQ(kPID, driver_->pid_);
}
+TEST_F(OpenVPNDriverTest, SpawnOpenVPNInMinijail) {
+ SetupLSBRelease();
+
+ EXPECT_FALSE(driver_->SpawnOpenVPN());
+
+ static const char kHost[] = "192.168.2.254";
+ SetArg(kProviderHostProperty, kHost);
+ driver_->tunnel_interface_ = "tun0";
+ driver_->rpc_task_.reset(new RPCTask(&control_, this));
+ EXPECT_CALL(*management_server_, Start(_, _, _))
+ .Times(2)
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(manager_, IsConnected()).Times(2).WillRepeatedly(Return(false));
+
+ EXPECT_CALL(manager_, GetJailVpnClients())
+ .Times(2)
+ .WillRepeatedly(Return(true));
+
+ const int kPID = 234678;
+ EXPECT_CALL(process_manager_, StartProcessInMinijail(_, _, _, _, _, _, _, _))
+ .WillOnce(Return(-1))
+ .WillOnce(Return(kPID));
+ EXPECT_FALSE(driver_->SpawnOpenVPN());
+ EXPECT_TRUE(driver_->SpawnOpenVPN());
+ EXPECT_EQ(kPID, driver_->pid_);
+}
+
TEST_F(OpenVPNDriverTest, OnOpenVPNDied) {
const int kPID = 99999;
driver_->device_ = device_;
