From 8a5740fbb1158744a9c639d01a4a96c36d3d1953 Mon Sep 17 00:00:00 2001
From: Wiktor Garbacz <wiktorg@google.com>
Date: Mon, 7 Mar 2022 23:57:28 -0800
Subject: [PATCH] Better handle invalid read-write mounts

PiperOrigin-RevId: 433136095
Change-Id: I17eb347c0a5cfef5e05c3717dfdd83055d967e35
---
 sandboxed_api/sandbox2/mounts.cc        | 11 +++++++++--
 sandboxed_api/sandbox2/policybuilder.cc | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc
index 73972bd..e55c382 100644
--- a/sandboxed_api/sandbox2/mounts.cc
+++ b/sandboxed_api/sandbox2/mounts.cc
@@ -479,6 +479,7 @@ uint64_t GetMountFlagsFor(const std::string& path) {
   uint64_t flags = 0;
   using MountPair = std::pair<uint64_t, uint64_t>;
   for (const auto& [mount_flag, vfs_flag] : {
+           MountPair(MS_RDONLY, ST_RDONLY),
            MountPair(MS_NOSUID, ST_NOSUID),
            MountPair(MS_NODEV, ST_NODEV),
            MountPair(MS_NOEXEC, ST_NOEXEC),
@@ -562,8 +563,14 @@ void MountWithDefaults(const std::string& source, const std::string& target,
   // Flags are ignored for a bind mount, a remount is needed to set the flags.
   if (extra_flags & MS_BIND) {
     // Get actual mount flags.
-    flags |= GetMountFlagsFor(target);
-    res = mount("", target.c_str(), "", flags | MS_REMOUNT, nullptr);
+    uint64_t target_flags = GetMountFlagsFor(target);
+    if ((target_flags & MS_RDONLY) != 0 && (flags & MS_RDONLY) == 0) {
+      SAPI_RAW_LOG(FATAL,
+                   "cannot remount %s as read-write as it's on read-only dev",
+                   target.c_str());
+    }
+    res = mount("", target.c_str(), "", flags | target_flags | MS_REMOUNT,
+                nullptr);
     SAPI_RAW_PCHECK(res != -1, "remounting %s with flags=%s failed", target,
                     MountFlagsToString(flags));
   }
diff --git a/sandboxed_api/sandbox2/policybuilder.cc b/sandboxed_api/sandbox2/policybuilder.cc
index c42fdb7..ea35fb5 100644
--- a/sandboxed_api/sandbox2/policybuilder.cc
+++ b/sandboxed_api/sandbox2/policybuilder.cc
@@ -22,6 +22,7 @@
 #include <linux/random.h>  // For GRND_NONBLOCK
 #include <sys/mman.h>      // For mmap arguments
 #include <sys/socket.h>
+#include <sys/statvfs.h>
 #include <syscall.h>
 
 #include <array>
@@ -72,6 +73,15 @@ bool CheckBpfBounds(const sock_filter& filter, size_t max_jmp) {
   return true;
 }
 
+bool IsOnReadOnlyDev(const std::string& path) {
+  struct statvfs vfs;
+  if (TEMP_FAILURE_RETRY(statvfs(path.c_str(), &vfs)) == -1) {
+    PLOG(ERROR) << "Could not statvfs: " << path.c_str();
+    return false;
+  }
+  return vfs.f_flag & ST_RDONLY;
+}
+
 }  // namespace
 
 PolicyBuilder& PolicyBuilder::AllowSyscall(uint32_t num) {
@@ -939,6 +949,13 @@ PolicyBuilder& PolicyBuilder::AddFileAt(absl::string_view outside,
     return *this;
   }
 
+  if (!is_ro && IsOnReadOnlyDev(*valid_outside)) {
+    SetError(absl::FailedPreconditionError(
+        absl::StrCat("Cannot add ", outside,
+                     " as read-write as it's on a read-only device")));
+    return *this;
+  }
+
   if (auto status = mounts_.AddFileAt(*valid_outside, inside, is_ro);
       !status.ok()) {
     SetError(
@@ -995,6 +1012,13 @@ PolicyBuilder& PolicyBuilder::AddDirectoryAt(absl::string_view outside,
     return *this;
   }
 
+  if (!is_ro && IsOnReadOnlyDev(*valid_outside)) {
+    SetError(absl::FailedPreconditionError(
+        absl::StrCat("Cannot add ", outside,
+                     " as read-write as it's on a read-only device")));
+    return *this;
+  }
+
   if (absl::Status status =
           mounts_.AddDirectoryAt(*valid_outside, inside, is_ro);
       !status.ok()) {