diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc index 8b365de2..c425123e 100644 --- a/sandboxed_api/sandbox2/mounts.cc +++ b/sandboxed_api/sandbox2/mounts.cc @@ -94,9 +94,38 @@ absl::Status VerifyProcMount(const MountTree& mount_tree) { } absl::Status VerifySharedMountNamespace(const MountTree& mount_tree) { - if (mount_tree.has_node() && mount_tree.node().has_tmpfs_node()) { - return absl::FailedPreconditionError( - "Shared mount namespace cannot be used with tmpfs mounts."); + if (mount_tree.has_node()) { + const MountTree::Node& node = mount_tree.node(); + if (node.has_tmpfs_node()) { + return absl::FailedPreconditionError( + "Shared mount namespace cannot be used with tmpfs mounts."); + } + // A writable root is implicit, unnamespaced-looking state: nothing in + // the policy marks it as something instances end up sharing, so a + // caller has no reason to expect that a write from one Sandbox2 + // instance becomes visible to every other instance sharing this mount + // namespace -- confirmed directly: one instance's write to its + // writable root is readable by a second, independently-launched + // instance. Rejected here for the same reason tmpfs is rejected above: + // both are per-instance-looking state that a shared mount namespace + // would silently turn into cross-instance state. + // + // If per-instance writable storage is needed alongside a shared mount + // namespace, bind-mount a separate, per-instance directory from the + // host instead of making the root writable, e.g. + // `PolicyBuilder::AddDirectoryAt(per_instance_outside_path, + // "/some/inside/path", /*is_ro=*/false)` with a distinct + // `per_instance_outside_path` per Sandbox2 instance. A regular + // directory bind-mount is not a tmpfs node and is not the root node, + // so it isn't rejected by either check above, and each instance's + // writes stay confined to its own outside directory. `AddTmpfs()` is + // not a substitute here: this function recurses into every entry in + // the mount tree, not only the root, so a tmpfs mounted at any + // interior path is rejected the same way a tmpfs root is. + if (node.has_root_node() && node.root_node().writable()) { + return absl::FailedPreconditionError( + "Shared mount namespace cannot be used with a writable root."); + } } ABSL_RETURN_IF_ERROR(VerifyProcMount(mount_tree)); for (const auto& [name, subtree] : mount_tree.entries()) { diff --git a/sandboxed_api/sandbox2/sandbox2_test.cc b/sandboxed_api/sandbox2/sandbox2_test.cc index 363f35ba..59ea5a3c 100644 --- a/sandboxed_api/sandbox2/sandbox2_test.cc +++ b/sandboxed_api/sandbox2/sandbox2_test.cc @@ -389,6 +389,27 @@ TEST(Sandbox2Test, SharedMountNamespaceWorks) { EXPECT_EQ(result.reason_code(), 0); } +TEST(Sandbox2Test, SharedMountNamespaceRejectsWritableRoot) { + SKIP_SANITIZERS; + + // A writable root, like tmpfs, is implicit per-instance state that + // should not be silently sharable -- EnableSharedMountNamespace() must + // reject it the same way it already rejects tmpfs. The check runs + // during policy validation, before the sandboxee is ever executed, so + // this reuses the existing minimal testcase rather than needing a + // purpose-built binary. + const std::string path = GetTestSourcePath("sandbox2/testcases/minimal"); + auto executor = std::make_unique(path, std::vector{path}); + SAPI_ASSERT_OK_AND_ASSIGN( + auto policy, sandbox2::PolicyBuilder() + .DefaultAction(sandbox2::AllowAllSyscalls()) + .SetRootWritable() + .TryBuild()); + Sandbox2 sandbox(std::move(executor), std::move(policy)); + EXPECT_THAT(sandbox.EnableSharedMountNamespace(), + StatusIs(absl::StatusCode::kFailedPrecondition)); +} + TEST(SharedMemoryTest, SharedMemoryDataTransferWorks) { SKIP_SANITIZERS; const std::string path =