From 18eb8772467d45a234cd4343a4e9a45cb938d996 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Mon, 18 Oct 2021 22:33:15 -0700 Subject: [PATCH] [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 Bug: 198657657 Bug: 216722132 Test: manual Change-Id: I6035447f94ef44c4ae3294c3ae47de2d7210683a Merged-In: I6035447f94ef44c4ae3294c3ae47de2d7210683a --- Utils.cpp | 50 +++++++++++++++++++++++++++++++++ Utils.h | 13 +++++++++ VoldNativeService.cpp | 49 ++++++++++++++++++++++++++------ VoldNativeServiceValidation.cpp | 27 ++++++++++++++++++ VoldNativeServiceValidation.h | 5 ++++ 5 files changed, 136 insertions(+), 8 deletions(-) diff --git a/Utils.cpp b/Utils.cpp index e077ead..864cbf8 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -1758,5 +1758,55 @@ status_t PrepareAndroidDirs(const std::string& volumeRoot) { 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 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 b5bb4c2..2d54639 100644 --- a/Utils.h +++ b/Utils.h @@ -27,6 +27,7 @@ #include #include +#include #include struct DIR; @@ -205,6 +206,18 @@ status_t UnmountUserFuse(userid_t userId, const std::string& absolute_lower_path 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 OpenDirInProcfs(std::string_view path); + } // namespace vold } // namespace android diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 1c94220..1033af9 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -966,10 +966,25 @@ binder::Status VoldNativeService::mountIncFs( 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, @@ -992,9 +1007,15 @@ binder::Status VoldNativeService::mountIncFs( 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( @@ -1042,10 +1063,22 @@ binder::Status VoldNativeService::setIncFsMountOptions( 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 @@ binder::Status CheckArgumentHex(const std::string& hex) { 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 CheckArgumentId(const std::string& id); 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