login: Explicitly remount the container with MS_RDONLY
Turns out that even if the filesystem is mounted read-only outside the
container, the same mount inside the container will preserve the flag
that was passed explicitly (0, meaning read-write), even if the
underlying filesystem was marked read-only.
This change explicitly remounts the containers' rootfs with MS_RDONLY if
the underlying filesystem is mounted read-only.
BUG=b:31771616
TEST=FEATURES="test" emerge-${BOARD} chromeos-base/chromeos-login
TEST=test_that cheets_FileSystemPermission # passes
TEST=test_that cheets_FileSystemPermission # fails with WRITABLE_MOUNT=1
Change-Id: I1d715c22779226937546e0566dceb0de29501b60
Reviewed-on: https://chromium-review.googlesource.com/390412
Commit-Ready: Luis Hector Chavez <lhchavez@google.com>
Tested-by: Luis Hector Chavez <lhchavez@google.com>
Reviewed-by: Luis Hector Chavez <lhchavez@google.com>
(cherry picked from commit debfbb561c97861f211c877d9de233bd67d5a313)
Reviewed-on: https://chromium-review.googlesource.com/392348
Trybot-Ready: Luis Hector Chavez <lhchavez@google.com>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Luis Hector Chavez <lhchavez@google.com>
diff --git a/login_manager/container_config_parser.cc b/login_manager/container_config_parser.cc
index 80ce7b8..eb0d93e 100644
--- a/login_manager/container_config_parser.cc
+++ b/login_manager/container_config_parser.cc
@@ -9,10 +9,15 @@
#include <sys/types.h>
#include <unistd.h>
+#include <algorithm>
+#include <string>
#include <vector>
#include <base/files/file_path.h>
+#include <base/files/file_util.h>
#include <base/json/json_reader.h>
+#include <base/strings/string_split.h>
+#include <base/strings/string_util.h>
#include <base/values.h>
#include <libcontainer/libcontainer.h>
@@ -21,10 +26,48 @@
namespace {
+// Parses |mountinfo_data| (the contents of /proc/self/mountinfo) to determine
+// whether |rootfs_path| was originally mounted as read-only.
+bool IsOriginalRootfsReadOnly(const std::string& mountinfo_data,
+ const base::FilePath& rootfs_path) {
+ constexpr size_t kMountinfoMountPointIndex = 4;
+ constexpr size_t kMountinfoMountOptionsIndex = 5;
+ const size_t kMountinfoMinNumberOfTokens =
+ std::max(kMountinfoMountPointIndex, kMountinfoMountOptionsIndex) + 1;
+
+ std::vector<std::string> lines =
+ base::SplitString(mountinfo_data, "\n", base::TRIM_WHITESPACE,
+ base::SPLIT_WANT_NONEMPTY);
+
+ for (const auto& line : lines) {
+ std::vector<std::string> tokens =
+ base::SplitString(line, base::kWhitespaceASCII, base::TRIM_WHITESPACE,
+ base::SPLIT_WANT_NONEMPTY);
+ // Some fields in /proc/self/mountinfo are optional. We only need the line
+ // to have |kMountinfoMinNumberOfTokens|.
+ if (tokens.size() < kMountinfoMinNumberOfTokens)
+ continue;
+ if (tokens[kMountinfoMountPointIndex] != rootfs_path.value())
+ continue;
+
+ std::vector<std::string> options =
+ base::SplitString(tokens[kMountinfoMountOptionsIndex], ",",
+ base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+ return std::find(options.begin(), options.end(), "ro") != options.end();
+ }
+
+ LOG(WARNING) << "Did not find mount information for " << rootfs_path.value()
+ << ". Assuming mounted read-only.";
+
+ return true;
+}
+
// Sets the rootfs of |config| to point to where the rootfs of the container
// is mounted.
bool ParseRootFileSystemConfig(const base::DictionaryValue& config_root_dict,
const base::FilePath& named_path,
+ const std::string& mountinfo_data,
ContainerConfigPtr* config_out) {
// |rootfs_dict| stays owned by |config_root_dict|
const base::DictionaryValue* rootfs_dict = nullptr;
@@ -39,18 +82,20 @@
}
container_config_rootfs(config_out->get(),
named_path.Append(rootfs_path).value().c_str());
- // Explicitly set the mount flags of the rootfs to 0.
+ // Explicitly set the mount flags of the rootfs.
//
// In Chrome OS, the rootfs is mounted nosuid, nodev, noexec. We need the
// filesystem to be mounted without those three flags within the container for
- // it to work correctly, so explicitly remount with no flags. Since the image
- // is originally mounted through a loopback device, and loopback devices that
- // are originally created as read-only cannot be set to writable after
- // creation, not setting the MS_RDONLY flag is safe here and will preserve the
- // read-only protection if the original filesystem is read-only, and will make
- // it writable for developers that have manually made the original filesystem
- // writable.
- container_config_rootfs_mount_flags(config_out->get(), 0);
+ // it to work correctly, so explicitly remount with none of those flags. We
+ // need to preserve the ro/rw state of the original mount, though, since the
+ // internal namespace will reflect whatever flag was passed here instead of
+ // respecting the original filesystem's ro/rw state.
+ uint32_t flags = 0;
+ if (IsOriginalRootfsReadOnly(mountinfo_data,
+ named_path.Append(rootfs_path))) {
+ flags |= MS_RDONLY;
+ }
+ container_config_rootfs_mount_flags(config_out->get(), flags);
return true;
}
@@ -458,10 +503,13 @@
bool ParseConfigDicts(const base::DictionaryValue& config_root_dict,
const base::DictionaryValue& runtime_root_dict,
const base::FilePath& named_path,
+ const std::string& mountinfo_data,
ContainerConfigPtr* config_out) {
// Root fs info
- if (!ParseRootFileSystemConfig(config_root_dict, named_path, config_out))
+ if (!ParseRootFileSystemConfig(config_root_dict, named_path,
+ mountinfo_data, config_out)) {
return false;
+ }
// Process info
uid_t uid;
@@ -490,6 +538,7 @@
bool ParseContainerConfig(const std::string& config_json_data,
const std::string& runtime_json_data,
+ const std::string& mountinfo_data,
const std::string& container_name,
const std::string& parent_cgroup_name,
const base::FilePath& named_container_path,
@@ -522,8 +571,9 @@
return false;
}
if (!ParseConfigDicts(*config_dict, *runtime_dict, named_container_path,
- config_out))
+ mountinfo_data, config_out)) {
return false;
+ }
// Set the cgroup configuration
if (container_config_set_cgroup_parent(
diff --git a/login_manager/container_config_parser.h b/login_manager/container_config_parser.h
index 84c504f..4c2b44a 100644
--- a/login_manager/container_config_parser.h
+++ b/login_manager/container_config_parser.h
@@ -5,8 +5,8 @@
#ifndef LOGIN_MANAGER_CONTAINER_CONFIG_PARSER_H_
#define LOGIN_MANAGER_CONTAINER_CONFIG_PARSER_H_
-#include <string>
#include <memory>
+#include <string>
#include <libcontainer/libcontainer.h>
#include <base/files/file_path.h>
@@ -21,12 +21,14 @@
// specified in https://github.com/opencontainers/runtime-spec/tree/v0.2.0
// |config_json_data| - The text from config.json.
// |runtime_json_data| - The text from runtime.json.
+// |mountinfo_data| - The text from /proc/self/mountinfo.
// |container_name| - Unique name for the container.
// |parent_cgroup_name| - Name of the parent cgroup for this container.
// |named_container_path| - Path to the base of the container data and rootfs.
// |config_out| - Filled with the configuration, defined in libcontainer.
bool ParseContainerConfig(const std::string& config_json_data,
const std::string& runtime_json_data,
+ const std::string& mountinfo_data,
const std::string& container_name,
const std::string& parent_cgroup_name,
const base::FilePath& named_container_path,
diff --git a/login_manager/container_config_parser_unittest.cc b/login_manager/container_config_parser_unittest.cc
index f76d9b8..c885c51 100644
--- a/login_manager/container_config_parser_unittest.cc
+++ b/login_manager/container_config_parser_unittest.cc
@@ -148,6 +148,21 @@
const char kCgroupParent[] = "test_cgroup";
+const char kTestcMountinfo[] = R"(
+104 99 0:57 / /proc rw,relatime - proc none rw
+73 15 7:1 /container.bin /var/run/containers/testc/rootfs_path ro,relatime -
+)";
+
+const char kAndroidReadonlyMountinfo[] = R"(
+104 99 0:57 / /proc rw,relatime - proc none rw
+73 15 7:1 /container.bin /var/run/containers/android/rootfs_path ro,relatime -
+)";
+
+const char kAndroidWritableMountinfo[] = R"(
+104 99 0:57 / /proc rw,relatime - proc none rw
+73 15 7:1 /container.bin /var/run/containers/android/rootfs_path rw,relatime -
+)";
+
} // anonymous namespace
namespace login_manager {
@@ -158,7 +173,8 @@
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
EXPECT_TRUE(ParseContainerConfig(kBasicJsonConfigData,
- kBasicJsonRuntimeData, "testc",
+ kBasicJsonRuntimeData,
+ kTestcMountinfo, "testc",
kCgroupParent,
kNamedContainerPath, &config));
EXPECT_EQ(kNamedContainerPath.Append("rootfs_path").value(),
@@ -185,7 +201,8 @@
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
EXPECT_TRUE(ParseContainerConfig(kBasicJsonConfigData,
- kBasicJsonRuntimeData, "android",
+ kBasicJsonRuntimeData,
+ kAndroidReadonlyMountinfo, "android",
kCgroupParent,
kNamedContainerPath, &config));
EXPECT_EQ(kNamedContainerPath.Append("rootfs_path").value(),
@@ -197,6 +214,22 @@
container_config_get_run_setfiles(config.get()));
EXPECT_EQ(std::string(kCgroupParent),
container_config_get_cgroup_parent(config.get()));
+ EXPECT_EQ(MS_BIND | MS_REMOUNT | MS_RDONLY,
+ container_config_get_rootfs_mount_flags(config.get()));
+}
+
+TEST(ContainerConfigParserTest, TestWritableMountConfigAndroid) {
+ const base::FilePath kNamedContainerPath("/var/run/containers/android");
+
+ ContainerConfigPtr config(container_config_create(),
+ &container_config_destroy);
+ EXPECT_TRUE(ParseContainerConfig(kBasicJsonConfigData,
+ kBasicJsonRuntimeData,
+ kAndroidWritableMountinfo, "android",
+ kCgroupParent,
+ kNamedContainerPath, &config));
+ EXPECT_EQ(MS_BIND | MS_REMOUNT,
+ container_config_get_rootfs_mount_flags(config.get()));
}
TEST(ContainerConfigParserTest, TestFailedConfigRootDictEmpty) {
@@ -206,7 +239,8 @@
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
EXPECT_FALSE(ParseContainerConfig(kEmptyJsonConfigData,
- kBasicJsonRuntimeData, "testc",
+ kBasicJsonRuntimeData,
+ kTestcMountinfo, "testc",
kCgroupParent,
kNamedContainerPath, &config));
}
@@ -218,7 +252,8 @@
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
EXPECT_FALSE(ParseContainerConfig(kExtraMountJsonConfigData,
- kBasicJsonRuntimeData, "testc",
+ kBasicJsonRuntimeData,
+ kTestcMountinfo, "testc",
kCgroupParent,
kNamedContainerPath, &config));
}
@@ -229,7 +264,8 @@
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
EXPECT_TRUE(ParseContainerConfig(kBasicJsonConfigData,
- kCpuCgroupJsonRuntimeData, "testc",
+ kCpuCgroupJsonRuntimeData,
+ kTestcMountinfo, "testc",
kCgroupParent,
kNamedContainerPath, &config));
EXPECT_EQ(1024, container_config_get_cpu_shares(config.get()));
diff --git a/login_manager/container_manager_impl.cc b/login_manager/container_manager_impl.cc
index d6e67f7..898ecaa 100644
--- a/login_manager/container_manager_impl.cc
+++ b/login_manager/container_manager_impl.cc
@@ -33,6 +33,7 @@
const char kCpuSharesFile[] =
"/sys/fs/cgroup/cpu/session_manager_containers/cpu.shares";
const char kCpuSharesDefault[] = "1024";
+const char kMountinfoPath[] = "/proc/self/mountinfo";
std::string libcontainer_strerror(int err) {
if (err < 0) {
@@ -141,10 +142,17 @@
return false;
}
+ std::string mountinfo_data;
+ if (!base::ReadFileToString(base::FilePath(kMountinfoPath),
+ &mountinfo_data)) {
+ LOG(WARNING) << "Fail to read mountinfo data from " << kMountinfoPath
+ << ". Assuming all mounts are read-only.";
+ }
+
ContainerConfigPtr config(container_config_create(),
&container_config_destroy);
- if (!ParseContainerConfig(config_json_data, runtime_json_data, name_,
- kSessionManagerCgroup, container_directory_,
+ if (!ParseContainerConfig(config_json_data, runtime_json_data, mountinfo_data,
+ name_, kSessionManagerCgroup, container_directory_,
&config)) {
LOG(ERROR) << "Failed to parse container configuration for " << name_;
return false;
