sandbox2: reject writable root in EnableSharedMountNamespace() - #183
prasanna8585 wants to merge 3 commits into
Conversation
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.
|
Hi Team, Thanks for your time! |
|
Hi! Thanks for the PR, and sorry for the late reply. We have a couple of major comments before we are ready to proceed:
|
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.
|
Thanks for the review @p13l13d13 , all three addressed. You're right — traced through it and EnableSharedMountNamespace() validates before the sandboxee ever runs, so neither test binary was actually needed by the regression test. That was leftover from confirming the original report end-to-end. Removed both, and the test now reuses testcases:minimal. No functional change to the actual check itself — happy to adjust further if any of this doesn't match what you had in mind. |
…Writable() Addresses p13l13d13's second review pass: 1. The comment on the writable-root check in VerifySharedMountNamespace() was too long. Trimmed to the why (silently becomes cross-instance shared state, like tmpfs), the workaround (PolicyBuilder::AddDirectoryAt() with is_ro=false), and why AddTmpfs() isn't a substitute (rejected at any path, not only root) -- in four lines instead of twenty-four. No logic changed; this is comment-only. 2. Added a NOTE to SetRootWritable()'s doc comment stating it does not work with EnableSharedMountNamespace(), matching the existing NOTE style already used there for the namespace-support requirement.
2 participants
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.