[vold] Check incremental paths before mounting
Vold was trusting system_server too much and allowed for pretty
much any path in mount()/bindMount() calls for incremental.
This CL adds validation to make sure it's only accessing own
directories. This includes enforcing no symlinks in the paths
Ignore-AOSP-First: security fix
Bug: 198657657
Test: manual
Change-Id: I6035447f94ef44c4ae3294c3ae47de2d7210683a
diff --git a/Utils.cpp b/Utils.cpp
index f9f3058..f059c67 100644
--- a/Utils.cpp
+++ b/Utils.cpp
@@ -1695,5 +1695,55 @@
return OK;
}
+
+namespace ab = android::base;
+
+static ab::unique_fd openDirFd(int parentFd, const char* name) {
+ return ab::unique_fd{::openat(parentFd, name, O_CLOEXEC | O_DIRECTORY | O_PATH | O_NOFOLLOW)};
+}
+
+static ab::unique_fd openAbsolutePathFd(std::string_view path) {
+ if (path.empty() || path[0] != '/') {
+ errno = EINVAL;
+ return {};
+ }
+ if (path == "/") {
+ return openDirFd(-1, "/");
+ }
+
+ // first component is special - it includes the leading slash
+ auto next = path.find('/', 1);
+ auto component = std::string(path.substr(0, next));
+ if (component == "..") {
+ errno = EINVAL;
+ return {};
+ }
+ auto fd = openDirFd(-1, component.c_str());
+ if (!fd.ok()) {
+ return fd;
+ }
+ path.remove_prefix(std::min(next + 1, path.size()));
+ while (next != path.npos && !path.empty()) {
+ next = path.find('/');
+ component.assign(path.substr(0, next));
+ fd = openDirFd(fd, component.c_str());
+ if (!fd.ok()) {
+ return fd;
+ }
+ path.remove_prefix(std::min(next + 1, path.size()));
+ }
+ return fd;
+}
+
+std::pair<android::base::unique_fd, std::string> OpenDirInProcfs(std::string_view path) {
+ auto fd = openAbsolutePathFd(path);
+ if (!fd.ok()) {
+ return {};
+ }
+
+ auto linkPath = std::string("/proc/self/fd/") += std::to_string(fd.get());
+ return {std::move(fd), std::move(linkPath)};
+}
+
} // namespace vold
} // namespace android
diff --git a/Utils.h b/Utils.h
index bb6615c..8120db7 100644
--- a/Utils.h
+++ b/Utils.h
@@ -27,6 +27,7 @@
#include <chrono>
#include <string>
+#include <string_view>
#include <vector>
struct DIR;
@@ -200,6 +201,18 @@
const std::string& relative_upper_path);
status_t PrepareAndroidDirs(const std::string& volumeRoot);
+
+// Open a given directory as an FD, and return that and the corresponding procfs virtual
+// symlink path that can be used in any API that accepts a path string. Path stays valid until
+// the directory FD is closed.
+//
+// This may be useful when an API wants to restrict a path passed from an untrusted process,
+// and do it without any TOCTOU attacks possible (e.g. where an attacker replaces one of
+// the components with a symlink after the check passed). In that case opening a path through
+// this function guarantees that the target directory stays the same, and that it can be
+// referenced inside the current process via the virtual procfs symlink returned here.
+std::pair<android::base::unique_fd, std::string> OpenDirInProcfs(std::string_view path);
+
} // namespace vold
} // namespace android
diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp
index 291a0d9..757354c 100644
--- a/VoldNativeService.cpp
+++ b/VoldNativeService.cpp
@@ -993,10 +993,25 @@
const std::string& sysfsName,
::android::os::incremental::IncrementalFileSystemControlParcel* _aidl_return) {
ENFORCE_SYSTEM_OR_ROOT;
- CHECK_ARGUMENT_PATH(backingPath);
- CHECK_ARGUMENT_PATH(targetDir);
+ if (auto status = CheckIncrementalPath(IncrementalPathKind::MountTarget, targetDir);
+ !status.isOk()) {
+ return status;
+ }
+ if (auto status = CheckIncrementalPath(IncrementalPathKind::MountSource, backingPath);
+ !status.isOk()) {
+ return status;
+ }
- auto control = incfs::mount(backingPath, targetDir,
+ auto [backingFd, backingSymlink] = OpenDirInProcfs(backingPath);
+ if (!backingFd.ok()) {
+ return translate(-errno);
+ }
+ auto [targetFd, targetSymlink] = OpenDirInProcfs(targetDir);
+ if (!targetFd.ok()) {
+ return translate(-errno);
+ }
+
+ auto control = incfs::mount(backingSymlink, targetSymlink,
{.flags = IncFsMountFlags(flags),
// Mount with read timeouts.
.defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS,
@@ -1019,9 +1034,15 @@
binder::Status VoldNativeService::unmountIncFs(const std::string& dir) {
ENFORCE_SYSTEM_OR_ROOT;
- CHECK_ARGUMENT_PATH(dir);
+ if (auto status = CheckIncrementalPath(IncrementalPathKind::Any, dir); !status.isOk()) {
+ return status;
+ }
- return translate(incfs::unmount(dir));
+ auto [fd, symLink] = OpenDirInProcfs(dir);
+ if (!fd.ok()) {
+ return translate(-errno);
+ }
+ return translate(incfs::unmount(symLink));
}
binder::Status VoldNativeService::setIncFsMountOptions(
@@ -1069,10 +1090,22 @@
binder::Status VoldNativeService::bindMount(const std::string& sourceDir,
const std::string& targetDir) {
ENFORCE_SYSTEM_OR_ROOT;
- CHECK_ARGUMENT_PATH(sourceDir);
- CHECK_ARGUMENT_PATH(targetDir);
+ if (auto status = CheckIncrementalPath(IncrementalPathKind::Any, sourceDir); !status.isOk()) {
+ return status;
+ }
+ if (auto status = CheckIncrementalPath(IncrementalPathKind::Bind, targetDir); !status.isOk()) {
+ return status;
+ }
- return translate(incfs::bindMount(sourceDir, targetDir));
+ auto [sourceFd, sourceSymlink] = OpenDirInProcfs(sourceDir);
+ if (!sourceFd.ok()) {
+ return translate(-errno);
+ }
+ auto [targetFd, targetSymlink] = OpenDirInProcfs(targetDir);
+ if (!targetFd.ok()) {
+ return translate(-errno);
+ }
+ return translate(incfs::bindMount(sourceSymlink, targetSymlink));
}
binder::Status VoldNativeService::destroyDsuMetadataKey(const std::string& dsuSlot) {
diff --git a/VoldNativeServiceValidation.cpp b/VoldNativeServiceValidation.cpp
index ee1e65a..1d19141 100644
--- a/VoldNativeServiceValidation.cpp
+++ b/VoldNativeServiceValidation.cpp
@@ -105,4 +105,31 @@
return Ok();
}
+binder::Status CheckIncrementalPath(IncrementalPathKind kind, const std::string& path) {
+ if (auto status = CheckArgumentPath(path); !status.isOk()) {
+ return status;
+ }
+ if (kind == IncrementalPathKind::MountSource || kind == IncrementalPathKind::MountTarget ||
+ kind == IncrementalPathKind::Any) {
+ if (android::base::StartsWith(path, "/data/incremental/MT_")) {
+ if (kind != IncrementalPathKind::MountSource &&
+ (android::base::EndsWith(path, "/mount") || path.find("/mount/") != path.npos)) {
+ return Ok();
+ }
+ if (kind != IncrementalPathKind::MountTarget &&
+ (android::base::EndsWith(path, "/backing_store") ||
+ path.find("/backing_store/") != path.npos)) {
+ return Ok();
+ }
+ }
+ }
+ if (kind == IncrementalPathKind::Bind || kind == IncrementalPathKind::Any) {
+ if (android::base::StartsWith(path, "/data/app/")) {
+ return Ok();
+ }
+ }
+ return Exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+ StringPrintf("Path '%s' is not allowed", path.c_str()));
+}
+
} // namespace android::vold
diff --git a/VoldNativeServiceValidation.h b/VoldNativeServiceValidation.h
index d2fc9e0..7fcb738 100644
--- a/VoldNativeServiceValidation.h
+++ b/VoldNativeServiceValidation.h
@@ -34,4 +34,9 @@
binder::Status CheckArgumentPath(const std::string& path);
binder::Status CheckArgumentHex(const std::string& hex);
+// Incremental service is only allowed to touch its own directory, and the installed apps dir.
+// This function ensures the caller isn't doing anything tricky.
+enum class IncrementalPathKind { MountSource, MountTarget, Bind, Any };
+binder::Status CheckIncrementalPath(IncrementalPathKind kind, const std::string& path);
+
} // namespace android::vold