[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

@@ -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<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

Utils.h

@@ -27,6 +27,7 @@
 
 #include <chrono>
 #include <string>
+#include <string_view>
 #include <vector>
 
 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<android::base::unique_fd, std::string> OpenDirInProcfs(std::string_view path);
+
 }  // namespace vold
 }  // namespace android
 

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) {

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

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