diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc index 5b3d6ca..5ff9530 100644 --- a/sandboxed_api/sandbox2/mounts.cc +++ b/sandboxed_api/sandbox2/mounts.cc @@ -145,24 +145,54 @@ bool IsSameFile(const std::string& path1, const std::string& path2) { return stat1.st_dev == stat2.st_dev && stat1.st_ino == stat2.st_ino; } -bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2) { +bool IsWritable(const MountTree::Node& node) { + switch (node.node_case()) { + case MountTree::Node::kFileNode: + return node.file_node().writable(); + case MountTree::Node::kDirNode: + return node.dir_node().writable(); + case MountTree::Node::kRootNode: + return node.root_node().writable(); + default: + return false; + } +} + +bool HasSameTarget(const MountTree::Node& n1, const MountTree::Node& n2) { // Return early when node types are different if (n1.node_case() != n2.node_case()) { return false; } + // Compare proto fileds + switch (n1.node_case()) { + case MountTree::Node::kFileNode: + // Check whether files are the same (e.g. symlinks / hardlinks) + return IsSameFile(n1.file_node().outside(), n2.file_node().outside()); + case MountTree::Node::kDirNode: + // Check whether dirs are the same (e.g. symlinks / hardlinks) + return IsSameFile(n1.dir_node().outside(), n2.dir_node().outside()); + case MountTree::Node::kTmpfsNode: + return n1.tmpfs_node().tmpfs_options() == n2.tmpfs_node().tmpfs_options(); + case MountTree::Node::kRootNode: + return true; + default: + return false; + } +} + +bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2) { + if (!HasSameTarget(n1, n2)) { + return false; + } // Compare proto fileds switch (n1.node_case()) { case MountTree::Node::kFileNode: - // Check whether files are the same (e.g. symlinks / hardlinks) - return n1.file_node().writable() == n2.file_node().writable() && - IsSameFile(n1.file_node().outside(), n2.file_node().outside()); + return n1.file_node().writable() == n2.file_node().writable(); case MountTree::Node::kDirNode: - // Check whether dirs are the same (e.g. symlinks / hardlinks) - return n1.dir_node().writable() == n2.dir_node().writable() && - IsSameFile(n1.dir_node().outside(), n2.dir_node().outside()); + return n1.dir_node().writable() == n2.dir_node().writable(); case MountTree::Node::kTmpfsNode: - return n1.tmpfs_node().tmpfs_options() == n2.tmpfs_node().tmpfs_options(); + return true; case MountTree::Node::kRootNode: return n1.root_node().writable() == n2.root_node().writable(); default: @@ -275,6 +305,24 @@ absl::Status Mounts::Insert(absl::string_view path, std::string(path).c_str()); return absl::OkStatus(); } + if (internal::HasSameTarget(curtree->node(), new_node)) { + if (!internal::IsWritable(curtree->node()) && + internal::IsWritable(new_node)) { + SAPI_RAW_LOG(INFO, + "Chaning %s to writable, was insterted read-only before", + std::string(path).c_str()); + *curtree->mutable_node() = new_node; + return absl::OkStatus(); + } + if (internal::IsWritable(curtree->node()) && + !internal::IsWritable(new_node)) { + SAPI_RAW_LOG(INFO, + "Inserting %s read-only is a nop, as it was insterted " + "writable before", + std::string(path).c_str()); + return absl::OkStatus(); + } + } return absl::FailedPreconditionError(absl::StrCat( "Inserting ", path, " twice with conflicting values ", curtree->node().DebugString(), " vs. ", new_node.DebugString())); @@ -289,10 +337,6 @@ absl::Status Mounts::Insert(absl::string_view path, return absl::OkStatus(); } -absl::Status Mounts::AddFile(absl::string_view path, bool is_ro) { - return AddFileAt(path, path, is_ro); -} - absl::Status Mounts::AddFileAt(absl::string_view outside, absl::string_view inside, bool is_ro) { MountTree::Node node; diff --git a/sandboxed_api/sandbox2/mounts.h b/sandboxed_api/sandbox2/mounts.h index 99431f3..0b2e5ca 100644 --- a/sandboxed_api/sandbox2/mounts.h +++ b/sandboxed_api/sandbox2/mounts.h @@ -29,6 +29,8 @@ namespace sandbox2 { namespace internal { bool IsSameFile(const std::string& path1, const std::string& path2); +bool IsWritable(const MountTree::Node& node); +bool HasSameTarget(const MountTree::Node& n1, const MountTree::Node& n2); bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2); } // namespace internal @@ -47,11 +49,17 @@ class Mounts { Mounts& operator=(const Mounts&) = default; Mounts& operator=(Mounts&&) = default; - absl::Status AddFile(absl::string_view path, bool is_ro = true); + absl::Status AddFile(absl::string_view path, bool is_ro = true) { + return AddFileAt(path, path, is_ro); + } absl::Status AddFileAt(absl::string_view outside, absl::string_view inside, bool is_ro = true); + absl::Status AddDirectory(absl::string_view path, bool is_ro = true) { + return AddDirectoryAt(path, path, is_ro); + } + absl::Status AddDirectoryAt(absl::string_view outside, absl::string_view inside, bool is_ro = true); diff --git a/sandboxed_api/sandbox2/mounts_test.cc b/sandboxed_api/sandbox2/mounts_test.cc index 21fc38b..5b41319 100644 --- a/sandboxed_api/sandbox2/mounts_test.cc +++ b/sandboxed_api/sandbox2/mounts_test.cc @@ -128,6 +128,18 @@ TEST(MountTreeTest, TestMultipleInsertionFileSymlink) { EXPECT_THAT(mounts.AddFileAt(symlink_path, "/a"), IsOk()); } +TEST(MountTreeTest, TestMultipleInsertionUpgradeToWritable) { + Mounts mounts; + EXPECT_THAT(mounts.AddFile("/a"), IsOk()); + EXPECT_THAT(mounts.AddFile("/a", /*is_ro=*/false), IsOk()); + EXPECT_THAT(mounts.AddDirectory("/b"), IsOk()); + EXPECT_THAT(mounts.AddDirectory("/b", /*is_ro=*/false), IsOk()); + EXPECT_THAT(mounts.AddFile("/c", /*is_ro=*/false), IsOk()); + EXPECT_THAT(mounts.AddFile("/c"), IsOk()); + EXPECT_THAT(mounts.AddDirectory("/d", /*is_ro=*/false), IsOk()); + EXPECT_THAT(mounts.AddDirectory("/d"), IsOk()); +} + TEST(MountTreeTest, TestMultipleInsertionDirSymlink) { Mounts mounts; @@ -265,6 +277,96 @@ TEST(MountTreeTest, TestList) { // clang-format on } +TEST(MountTreeTest, TestIsWritable) { + MountTree::Node nodes[7]; + MountTree::FileNode* fn0 = nodes[0].mutable_file_node(); + fn0->set_writable(false); + fn0->set_outside("foo"); + MountTree::FileNode* fn1 = nodes[1].mutable_file_node(); + fn1->set_writable(true); + fn1->set_outside("bar"); + MountTree::DirNode* dn0 = nodes[2].mutable_dir_node(); + dn0->set_writable(false); + dn0->set_outside("foo"); + MountTree::DirNode* dn1 = nodes[3].mutable_dir_node(); + dn1->set_writable(true); + dn1->set_outside("bar"); + MountTree::RootNode* rn0 = nodes[4].mutable_root_node(); + rn0->set_writable(false); + MountTree::RootNode* rn1 = nodes[5].mutable_root_node(); + rn1->set_writable(true); + MountTree::TmpfsNode* tn0 = nodes[6].mutable_tmpfs_node(); + tn0->set_tmpfs_options("option1"); + + EXPECT_FALSE(internal::IsWritable(nodes[0])); + EXPECT_TRUE(internal::IsWritable(nodes[1])); + EXPECT_FALSE(internal::IsWritable(nodes[2])); + EXPECT_TRUE(internal::IsWritable(nodes[3])); + EXPECT_FALSE(internal::IsWritable(nodes[4])); + EXPECT_TRUE(internal::IsWritable(nodes[5])); + EXPECT_FALSE(internal::IsWritable(nodes[6])); +} + +TEST(MountTreeTest, TestHasSameTarget) { + MountTree::Node nodes[10]; + MountTree::FileNode* fn0 = nodes[0].mutable_file_node(); + fn0->set_writable(false); + fn0->set_outside("foo"); + MountTree::FileNode* fn1 = nodes[1].mutable_file_node(); + fn1->set_writable(true); + fn1->set_outside("foo"); + MountTree::FileNode* fn2 = nodes[2].mutable_file_node(); + fn2->set_writable(false); + fn2->set_outside("bar"); + MountTree::DirNode* dn0 = nodes[3].mutable_dir_node(); + dn0->set_writable(false); + dn0->set_outside("foo"); + MountTree::DirNode* dn1 = nodes[4].mutable_dir_node(); + dn1->set_writable(true); + dn1->set_outside("foo"); + MountTree::DirNode* dn2 = nodes[5].mutable_dir_node(); + dn2->set_writable(false); + dn2->set_outside("bar"); + MountTree::TmpfsNode* tn0 = nodes[6].mutable_tmpfs_node(); + tn0->set_tmpfs_options("option1"); + MountTree::TmpfsNode* tn1 = nodes[7].mutable_tmpfs_node(); + tn1->set_tmpfs_options("option2"); + MountTree::RootNode* rn0 = nodes[8].mutable_root_node(); + rn0->set_writable(false); + MountTree::RootNode* rn1 = nodes[9].mutable_root_node(); + rn1->set_writable(true); + + // Compare same file nodes + EXPECT_TRUE(internal::HasSameTarget(nodes[0], nodes[0])); + // Compare almost same file nodes (ro vs rw) + EXPECT_TRUE(internal::HasSameTarget(nodes[0], nodes[1])); + // Compare different file nodes + EXPECT_FALSE(internal::HasSameTarget(nodes[0], nodes[2])); + // Compare file node with dir node + EXPECT_FALSE(internal::HasSameTarget(nodes[0], nodes[3])); + + // Compare same dir nodes + EXPECT_TRUE(internal::HasSameTarget(nodes[3], nodes[3])); + // Compare almost same dir nodes (ro vs rw) + EXPECT_TRUE(internal::HasSameTarget(nodes[3], nodes[4])); + // Compare different dir nodes + EXPECT_FALSE(internal::HasSameTarget(nodes[3], nodes[5])); + // Compare dir node with tmpfs node + EXPECT_FALSE(internal::HasSameTarget(nodes[3], nodes[6])); + + // Compare same tmpfs nodes + EXPECT_TRUE(internal::HasSameTarget(nodes[6], nodes[6])); + // Compare different tmpfs nodes + EXPECT_FALSE(internal::HasSameTarget(nodes[6], nodes[7])); + // Compare dir node with root node + EXPECT_FALSE(internal::HasSameTarget(nodes[6], nodes[8])); + + // Compare same root nodes + EXPECT_TRUE(internal::HasSameTarget(nodes[8], nodes[8])); + // Compare almost same root nodes (ro vs rw) + EXPECT_TRUE(internal::HasSameTarget(nodes[8], nodes[9])); +} + TEST(MountTreeTest, TestNodeEquivalence) { MountTree::Node nodes[8]; MountTree::FileNode* fn0 = nodes[0].mutable_file_node();