From 6cb5a36f4c59ed64cacfca7a6178f22ec0e5d71b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 13 May 2022 19:11:42 +0000 Subject: [PATCH] init: fix mkdir to reliably detect top-level /data directories To determine the default encryption action, the mkdir command checks whether the given path is a top-level directory of /data. However, it assumed a path without any duplicate slashes or trailing slash(es). While everyone *should* be providing paths without unnecessary slashes, it is not guaranteed, as paths with unnecessary slashes still work correctly for all other parts of the mkdir command, including the SELinux label lookup and the actual directory creation. In particular, the /data/fonts directory is being created using 'mkdir /data/fonts/'. The effect is that the mkdir command thinks that /data/fonts/ is *not* a top-level directory of /data, so it defaults to no encryption action. Fortunately, the full command happens to use "encryption=Require", so we dodged a bullet there, though the warning "Inferred action different from explicit one" is still triggered. There are a few approaches we could take here, including even just fixing the /data/fonts/ command specifically, but I think the best solution is to have mkdir clean its path at the very beginning. This retains the Linux path semantics that people expect, while avoiding surprises in path processing afterwards. This CL implements that. Note, this CL intentionally changes the behavior of, and thus would break, any existing cases where mkdir is used to create a top-level /data directory using a path with unnecessary slashes and without using an explicit encryption action. There are no known cases where this already occurs, however. No cases exist in platform code, and vendor init scripts shouldn't be creating top-level /data directories anyway. Test: atest CtsInitTestCases Test: Booted and verified that a trailing slash is no longer present in the log message "Verified that /data/fonts/ has the encryption policy ...". Also verified that the message "Inferred action different ..." is no longer present just above it. Bug: 232554803 Change-Id: Ie55c3ac1a2b1cf50632d54a1e565cb98c17b2a6a --- init/util.cpp | 30 ++++++++++++++++++++++++------ init/util.h | 1 + init/util_test.cpp | 13 +++++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/init/util.cpp b/init/util.cpp index af6cf509b..20c1088ea 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -458,6 +458,24 @@ Result IsLegalPropertyValue(const std::string& name, const std::string& va return {}; } +// Remove unnecessary slashes so that any later checks (e.g., the check for +// whether the path is a top-level directory in /data) don't get confused. +std::string CleanDirPath(const std::string& path) { + std::string result; + result.reserve(path.length()); + // Collapse duplicate slashes, e.g. //data//foo// => /data/foo/ + for (char c : path) { + if (c != '/' || result.empty() || result.back() != '/') { + result += c; + } + } + // Remove trailing slash, e.g. /data/foo/ => /data/foo + if (result.length() > 1 && result.back() == '/') { + result.pop_back(); + } + return result; +} + static FscryptAction FscryptInferAction(const std::string& dir) { const std::string prefix = "/data/"; @@ -499,10 +517,11 @@ static FscryptAction FscryptInferAction(const std::string& dir) { } Result ParseMkdir(const std::vector& args) { + std::string path = CleanDirPath(args[1]); mode_t mode = 0755; Result uid = -1; Result gid = -1; - FscryptAction fscrypt_inferred_action = FscryptInferAction(args[1]); + FscryptAction fscrypt_inferred_action = FscryptInferAction(path); FscryptAction fscrypt_action = fscrypt_inferred_action; std::string ref_option = "ref"; bool set_option_encryption = false; @@ -569,14 +588,13 @@ Result ParseMkdir(const std::vector& args) { return Error() << "Key option set but encryption action is none"; } const std::string prefix = "/data/"; - if (StartsWith(args[1], prefix) && - args[1].find_first_of('/', prefix.size()) == std::string::npos) { + if (StartsWith(path, prefix) && path.find_first_of('/', prefix.size()) == std::string::npos) { if (!set_option_encryption) { - LOG(WARNING) << "Top-level directory needs encryption action, eg mkdir " << args[1] + LOG(WARNING) << "Top-level directory needs encryption action, eg mkdir " << path << " encryption=Require"; } if (fscrypt_action == FscryptAction::kNone) { - LOG(INFO) << "Not setting encryption policy on: " << args[1]; + LOG(INFO) << "Not setting encryption policy on: " << path; } } if (fscrypt_action != fscrypt_inferred_action) { @@ -585,7 +603,7 @@ Result ParseMkdir(const std::vector& args) { << static_cast(fscrypt_action); } - return MkdirOptions{args[1], mode, *uid, *gid, fscrypt_action, ref_option}; + return MkdirOptions{path, mode, *uid, *gid, fscrypt_action, ref_option}; } Result ParseMountAll(const std::vector& args) { diff --git a/init/util.h b/init/util.h index bf5367531..47d4ff518 100644 --- a/init/util.h +++ b/init/util.h @@ -69,6 +69,7 @@ bool is_android_dt_value_expected(const std::string& sub_path, const std::string bool IsLegalPropertyName(const std::string& name); Result IsLegalPropertyValue(const std::string& name, const std::string& value); +std::string CleanDirPath(const std::string& path); struct MkdirOptions { std::string target; diff --git a/init/util_test.cpp b/init/util_test.cpp index 565e7d4f0..e8144c307 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -170,5 +170,18 @@ TEST(util, mkdir_recursive_extra_slashes) { EXPECT_TRUE(is_dir(path1.c_str())); } +TEST(util, CleanDirPath) { + EXPECT_EQ("", CleanDirPath("")); + EXPECT_EQ("/", CleanDirPath("/")); + EXPECT_EQ("/", CleanDirPath("//")); + EXPECT_EQ("/foo", CleanDirPath("/foo")); + EXPECT_EQ("/foo", CleanDirPath("//foo")); + EXPECT_EQ("/foo", CleanDirPath("/foo/")); + EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar")); + EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar/")); + EXPECT_EQ("/foo/bar", CleanDirPath("/foo/bar////")); + EXPECT_EQ("/foo/bar", CleanDirPath("//foo//bar")); +} + } // namespace init } // namespace android