From c81ed83755183ea2bef6ec4011fc3b357b143df5 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Sat, 18 Jul 2026 21:26:20 +0530 Subject: [PATCH 1/2] sandbox2: reject writable root in EnableSharedMountNamespace() VerifySharedMountNamespace() rejects tmpfs and /proc mounts to prevent unintended shared state when multiple Sandbox2 instances share a mount namespace, but did not check MountTree::Node::RootNode.writable. A policy built with SetRootWritable() passed validation unconditionally, and two independently-launched Sandbox2 instances sharing a mount namespace via EnableSharedMountNamespace() with a writable root end up sharing the same writable filesystem root -- confirmed end-to-end: one instance's write is directly readable by a second, separate instance. Adds the RootNode.writable() check alongside the existing tmpfs_node() check, and a regression test. --- sandboxed_api/sandbox2/BUILD | 2 ++ sandboxed_api/sandbox2/mounts.cc | 13 ++++++-- sandboxed_api/sandbox2/sandbox2_test.cc | 20 ++++++++++++ sandboxed_api/sandbox2/testcases/BUILD | 16 ++++++++++ .../sandbox2/testcases/shared_root_read.cc | 31 +++++++++++++++++++ .../sandbox2/testcases/shared_root_write.cc | 28 +++++++++++++++++ 6 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 sandboxed_api/sandbox2/testcases/shared_root_read.cc create mode 100644 sandboxed_api/sandbox2/testcases/shared_root_write.cc diff --git a/sandboxed_api/sandbox2/BUILD b/sandboxed_api/sandbox2/BUILD index 6ead61cb..c3f0e7d1 100644 --- a/sandboxed_api/sandbox2/BUILD +++ b/sandboxed_api/sandbox2/BUILD @@ -1136,6 +1136,8 @@ cc_test( "//sandboxed_api/sandbox2/testcases:minimal", "//sandboxed_api/sandbox2/testcases:pthreads", "//sandboxed_api/sandbox2/testcases:shared_memory", + "//sandboxed_api/sandbox2/testcases:shared_root_read", + "//sandboxed_api/sandbox2/testcases:shared_root_write", "//sandboxed_api/sandbox2/testcases:sleep", "//sandboxed_api/sandbox2/testcases:starve", "//sandboxed_api/sandbox2/testcases:terminate_process_group", diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc index 8b365de2..b282e49e 100644 --- a/sandboxed_api/sandbox2/mounts.cc +++ b/sandboxed_api/sandbox2/mounts.cc @@ -94,9 +94,16 @@ 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."); + } + 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..0372d8f0 100644 --- a/sandboxed_api/sandbox2/sandbox2_test.cc +++ b/sandboxed_api/sandbox2/sandbox2_test.cc @@ -389,6 +389,26 @@ 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. + const std::string path = + GetTestSourcePath("sandbox2/testcases/shared_root_write"); + auto executor = std::make_unique(path, std::vector{path}); + SAPI_ASSERT_OK_AND_ASSIGN( + auto policy, sandbox2::PolicyBuilder() + .DefaultAction(sandbox2::AllowAllSyscalls()) + .SetRootWritable() + .UseForkServerSharedNetNs() + .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 = diff --git a/sandboxed_api/sandbox2/testcases/BUILD b/sandboxed_api/sandbox2/testcases/BUILD index 54ac3be1..c5811c5d 100644 --- a/sandboxed_api/sandbox2/testcases/BUILD +++ b/sandboxed_api/sandbox2/testcases/BUILD @@ -100,6 +100,22 @@ cc_binary( features = ["fully_static_link"], ) +cc_binary( + name = "shared_root_write", + testonly = True, + srcs = ["shared_root_write.cc"], + copts = sapi_platform_copts(), + features = ["fully_static_link"], +) + +cc_binary( + name = "shared_root_read", + testonly = True, + srcs = ["shared_root_read.cc"], + copts = sapi_platform_copts(), + features = ["fully_static_link"], +) + cc_binary( name = "personality", testonly = True, diff --git a/sandboxed_api/sandbox2/testcases/shared_root_read.cc b/sandboxed_api/sandbox2/testcases/shared_root_read.cc new file mode 100644 index 00000000..f9a116ae --- /dev/null +++ b/sandboxed_api/sandbox2/testcases/shared_root_read.cc @@ -0,0 +1,31 @@ +// Copyright 2026 The Sandboxed API Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Testcase for SharedMountNamespaceLeaksWritableRoot: checks whether the +// marker file written by shared_root_write is visible from a separate +// Sandbox2 instance. + +#include +#include + +int main() { + FILE* f = fopen("/shared_root_marker", "r"); + if (f == nullptr) { + return 1; // Not found -- root is properly isolated. + } + char buf[128] = {0}; + fgets(buf, sizeof(buf), f); + fclose(f); + return strcmp(buf, "written-by-instance-A\n") == 0 ? 0 : 3; +} diff --git a/sandboxed_api/sandbox2/testcases/shared_root_write.cc b/sandboxed_api/sandbox2/testcases/shared_root_write.cc new file mode 100644 index 00000000..cbc44754 --- /dev/null +++ b/sandboxed_api/sandbox2/testcases/shared_root_write.cc @@ -0,0 +1,28 @@ +// Copyright 2026 The Sandboxed API Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Testcase for SharedMountNamespaceLeaksWritableRoot: writes a marker file +// to the sandboxee's own writable root filesystem. + +#include + +int main() { + FILE* f = fopen("/shared_root_marker", "w"); + if (f == nullptr) { + return 2; + } + fputs("written-by-instance-A\n", f); + fclose(f); + return 0; +} From c64a027cf69d16953ea4a24ceb2f34fb32acfbbf Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Mon, 7 Sep 2026 15:43:25 +0530 Subject: [PATCH 2/2] sandbox2: address review feedback on writable-root rejection Addresses p13l13d13's review of the three points raised: 1. Both shared_root_write and shared_root_read were unused by the actual regression test, and never would be: EnableSharedMountNamespace() validates the policy before the sandboxee is ever executed, so the test never needed a purpose-built binary that writes or reads a marker file -- that was left over from confirming the original report end to end (one instance's write directly readable by a second instance), not something the regression test itself requires. Removed both source files and their BUILD entries; SharedMountNamespaceRejectsWritableRoot now reuses the existing testcases:minimal binary, already a dependency of this test target and already used by the SharedMountNamespaceWorks test directly above it. 2. Removed UseForkServerSharedNetNs() from the test's PolicyBuilder chain. It isn't exercised by anything this test checks -- the test only needs EnableSharedMountNamespace() to reject a writable root, and this option was unrelated leftover from the same broader investigation as point 1. 3. Added a comment on the new check in VerifySharedMountNamespace() explaining why a writable root is rejected (the same reason tmpfs already is: it looks like per-instance state, but a shared mount namespace silently turns it into cross-instance state -- confirmed directly in the original report) and naming a concrete alternative: PolicyBuilder::AddDirectoryAt() with a distinct per-instance outside path and is_ro=false, which bind-mounts a real directory rather than a tmpfs or the root node, so it isn't caught by either check. Verified AddTmpfs() is not a substitute worth suggesting instead: VerifySharedMountNamespace() 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 already is. Not changed: the actual validation logic in VerifySharedMountNamespace() (the if (node.has_root_node() && node.root_node().writable()) check itself is byte-for-byte the same as before this commit) -- only comments, the test, and the two now-removed testcase files. --- sandboxed_api/sandbox2/BUILD | 2 -- sandboxed_api/sandbox2/mounts.cc | 22 +++++++++++++ sandboxed_api/sandbox2/sandbox2_test.cc | 9 +++--- sandboxed_api/sandbox2/testcases/BUILD | 16 ---------- .../sandbox2/testcases/shared_root_read.cc | 31 ------------------- .../sandbox2/testcases/shared_root_write.cc | 28 ----------------- 6 files changed, 27 insertions(+), 81 deletions(-) delete mode 100644 sandboxed_api/sandbox2/testcases/shared_root_read.cc delete mode 100644 sandboxed_api/sandbox2/testcases/shared_root_write.cc diff --git a/sandboxed_api/sandbox2/BUILD b/sandboxed_api/sandbox2/BUILD index c3f0e7d1..6ead61cb 100644 --- a/sandboxed_api/sandbox2/BUILD +++ b/sandboxed_api/sandbox2/BUILD @@ -1136,8 +1136,6 @@ cc_test( "//sandboxed_api/sandbox2/testcases:minimal", "//sandboxed_api/sandbox2/testcases:pthreads", "//sandboxed_api/sandbox2/testcases:shared_memory", - "//sandboxed_api/sandbox2/testcases:shared_root_read", - "//sandboxed_api/sandbox2/testcases:shared_root_write", "//sandboxed_api/sandbox2/testcases:sleep", "//sandboxed_api/sandbox2/testcases:starve", "//sandboxed_api/sandbox2/testcases:terminate_process_group", diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc index b282e49e..c425123e 100644 --- a/sandboxed_api/sandbox2/mounts.cc +++ b/sandboxed_api/sandbox2/mounts.cc @@ -100,6 +100,28 @@ absl::Status VerifySharedMountNamespace(const MountTree& mount_tree) { 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."); diff --git a/sandboxed_api/sandbox2/sandbox2_test.cc b/sandboxed_api/sandbox2/sandbox2_test.cc index 0372d8f0..59ea5a3c 100644 --- a/sandboxed_api/sandbox2/sandbox2_test.cc +++ b/sandboxed_api/sandbox2/sandbox2_test.cc @@ -394,15 +394,16 @@ TEST(Sandbox2Test, SharedMountNamespaceRejectsWritableRoot) { // 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. - const std::string path = - GetTestSourcePath("sandbox2/testcases/shared_root_write"); + // 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() - .UseForkServerSharedNetNs() .TryBuild()); Sandbox2 sandbox(std::move(executor), std::move(policy)); EXPECT_THAT(sandbox.EnableSharedMountNamespace(), diff --git a/sandboxed_api/sandbox2/testcases/BUILD b/sandboxed_api/sandbox2/testcases/BUILD index c5811c5d..54ac3be1 100644 --- a/sandboxed_api/sandbox2/testcases/BUILD +++ b/sandboxed_api/sandbox2/testcases/BUILD @@ -100,22 +100,6 @@ cc_binary( features = ["fully_static_link"], ) -cc_binary( - name = "shared_root_write", - testonly = True, - srcs = ["shared_root_write.cc"], - copts = sapi_platform_copts(), - features = ["fully_static_link"], -) - -cc_binary( - name = "shared_root_read", - testonly = True, - srcs = ["shared_root_read.cc"], - copts = sapi_platform_copts(), - features = ["fully_static_link"], -) - cc_binary( name = "personality", testonly = True, diff --git a/sandboxed_api/sandbox2/testcases/shared_root_read.cc b/sandboxed_api/sandbox2/testcases/shared_root_read.cc deleted file mode 100644 index f9a116ae..00000000 --- a/sandboxed_api/sandbox2/testcases/shared_root_read.cc +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2026 The Sandboxed API Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Testcase for SharedMountNamespaceLeaksWritableRoot: checks whether the -// marker file written by shared_root_write is visible from a separate -// Sandbox2 instance. - -#include -#include - -int main() { - FILE* f = fopen("/shared_root_marker", "r"); - if (f == nullptr) { - return 1; // Not found -- root is properly isolated. - } - char buf[128] = {0}; - fgets(buf, sizeof(buf), f); - fclose(f); - return strcmp(buf, "written-by-instance-A\n") == 0 ? 0 : 3; -} diff --git a/sandboxed_api/sandbox2/testcases/shared_root_write.cc b/sandboxed_api/sandbox2/testcases/shared_root_write.cc deleted file mode 100644 index cbc44754..00000000 --- a/sandboxed_api/sandbox2/testcases/shared_root_write.cc +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2026 The Sandboxed API Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Testcase for SharedMountNamespaceLeaksWritableRoot: writes a marker file -// to the sandboxee's own writable root filesystem. - -#include - -int main() { - FILE* f = fopen("/shared_root_marker", "w"); - if (f == nullptr) { - return 2; - } - fputs("written-by-instance-A\n", f); - fclose(f); - return 0; -}