Automated rollback of commit 2fffcba.

PiperOrigin-RevId: 718829287
Change-Id: Ia3814a76fa5087b9b9bef86417261045268ff89f

sandboxed_api/sandbox2/BUILD

@@ -625,6 +625,7 @@ cc_library(
         "@com_google_sandboxed_api//sandboxed_api/sandbox2/network_proxy:filtering",
         "@com_google_sandboxed_api//sandboxed_api/sandbox2/util:bpf_helper",
         "@com_google_sandboxed_api//sandboxed_api/util:file_base",
+        "@com_google_sandboxed_api//sandboxed_api/util:fileops",
         "@com_google_sandboxed_api//sandboxed_api/util:status",
     ],
 )

sandboxed_api/sandbox2/CMakeLists.txt

@@ -551,6 +551,7 @@ target_link_libraries(sandbox2_policybuilder
           sandbox2::namespace
           sandbox2::syscall
           sapi::file_base
+          sapi::fileops
           sapi::status
   PUBLIC absl::check
          absl::core_headers

sandboxed_api/sandbox2/policybuilder.cc

@@ -94,6 +94,27 @@ namespace {
 
 namespace file = ::sapi::file;
 
+// Validates that the path is absolute and canonical.
+absl::StatusOr<std::string> ValidatePath(absl::string_view path,
+                                         bool allow_relative_path = false) {
+  if (path.empty()) {
+    return absl::InvalidArgumentError("Path must not be empty");
+  }
+
+  if (!file::IsAbsolutePath(path) && !allow_relative_path) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("Path must be absolute: ", path));
+  }
+
+  std::string fixed_path = file::CleanPath(path);
+  if (fixed_path != path) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("Path is not canonical: ", path));
+  }
+
+  return fixed_path;
+}
+
 constexpr uint32_t kMmapSyscalls[] = {
 #ifdef __NR_mmap2
     __NR_mmap2,
@@ -1346,25 +1367,6 @@ PolicyBuilder& PolicyBuilder::DefaultAction(TraceAllSyscalls) {
   return *this;
 }
 
-absl::StatusOr<std::string> PolicyBuilder::ValidateAbsolutePath(
-    absl::string_view path) {
-  if (!file::IsAbsolutePath(path)) {
-    return absl::InvalidArgumentError(
-        absl::StrCat("Path is not absolute: '", path, "'"));
-  }
-  return ValidatePath(path);
-}
-
-absl::StatusOr<std::string> PolicyBuilder::ValidatePath(
-    absl::string_view path) {
-  std::string fixed_path = file::CleanPath(path);
-  if (fixed_path != path) {
-    return absl::InvalidArgumentError(absl::StrCat(
-        "Path was not normalized. '", path, "' != '", fixed_path, "'"));
-  }
-  return fixed_path;
-}
-
 std::vector<sock_filter> PolicyBuilder::ResolveBpfFunc(BpfFunc f) {
   bpf_labels l = {0};
 
@@ -1446,7 +1448,7 @@ PolicyBuilder& PolicyBuilder::AddFileIfNamespaced(absl::string_view path,
 PolicyBuilder& PolicyBuilder::AddFileAtIfNamespaced(absl::string_view outside,
                                                     absl::string_view inside,
                                                     bool is_ro) {
-  auto valid_outside = ValidateAbsolutePath(outside);
+  auto valid_outside = ValidatePath(outside);
   if (!valid_outside.ok()) {
     SetError(valid_outside.status());
     return *this;
@@ -1481,7 +1483,7 @@ PolicyBuilder& PolicyBuilder::AddLibrariesForBinary(
     absl::string_view path, absl::string_view ld_library_path) {
   EnableNamespaces();  // NOLINT(clang-diagnostic-deprecated-declarations)
 
-  auto valid_path = ValidatePath(path);
+  auto valid_path = ValidatePath(path, /*allow_relative_path=*/true);
   if (!valid_path.ok()) {
     SetError(valid_path.status());
     return *this;
@@ -1519,7 +1521,7 @@ PolicyBuilder& PolicyBuilder::AddDirectoryIfNamespaced(absl::string_view path,
 
 PolicyBuilder& PolicyBuilder::AddDirectoryAtIfNamespaced(
     absl::string_view outside, absl::string_view inside, bool is_ro) {
-  auto valid_outside = ValidateAbsolutePath(outside);
+  auto valid_outside = ValidatePath(outside);
   if (!valid_outside.ok()) {
     SetError(valid_outside.status());
     return *this;

sandboxed_api/sandbox2/policybuilder.h

@@ -915,14 +915,6 @@ class PolicyBuilder final {
   friend class PolicyBuilderPeer;  // For testing
   friend class StackTracePeer;
 
-  // Validates that the path is absolute and normalized.
-  static absl::StatusOr<std::string> ValidateAbsolutePath(
-      absl::string_view path);
-
-  // Validates that the path is normalized.
-  //
-  static absl::StatusOr<std::string> ValidatePath(absl::string_view path);
-
   // Similar to AddFile(At)/AddDirectory(At) but it won't force use of
   // namespaces - files will only be added to the namespace if it is not
   // disabled by the time of TryBuild().

sandboxed_api/sandbox2/policybuilder_test.cc

@@ -18,14 +18,15 @@
 #include <unistd.h>
 
 #include <cerrno>
+#include <initializer_list>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/status/status.h"
-#include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
 #include "sandboxed_api/sandbox2/allow_unrestricted_networking.h"
 #include "sandboxed_api/sandbox2/policy.h"
@@ -40,11 +41,6 @@ class PolicyBuilderPeer {
 
   int policy_size() const { return builder_->user_policy_.size(); }
 
-  static absl::StatusOr<std::string> ValidateAbsolutePath(
-      absl::string_view path) {
-    return PolicyBuilder::ValidateAbsolutePath(path);
-  }
-
  private:
   PolicyBuilder* builder_;
 };
@@ -56,7 +52,6 @@ using ::sapi::StatusIs;
 using ::testing::Eq;
 using ::testing::Lt;
 using ::testing::StartsWith;
-using ::testing::StrEq;
 
 TEST(PolicyBuilderTest, Testpolicy_size) {
   ssize_t last_size = 0;
@@ -113,28 +108,50 @@ TEST(PolicyBuilderTest, Testpolicy_size) {
   // clang-format on
 }
 
-TEST(PolicyBuilderTest, TestValidateAbsolutePath) {
-  for (auto const& bad_path : {
-           "..",
-           "a",
-           "a/b",
-           "a/b/c",
-           "/a/b/c/../d",
-           "/a/b/c/./d",
-           "/a/b/c//d",
-           "/a/b/c/d/",
-           "/a/bAAAAAAAAAAAAAAAAAAAAAA/c/d/",
-       }) {
-    EXPECT_THAT(PolicyBuilderPeer::ValidateAbsolutePath(bad_path),
-                StatusIs(absl::StatusCode::kInvalidArgument));
+TEST(PolicyBuilderTest, ApisWithPathValidation) {
+  const std::initializer_list<std::pair<absl::string_view, absl::StatusCode>>
+      kTestCases = {
+          {"/a", absl::StatusCode::kOk},
+          {"/a/b/c/d", absl::StatusCode::kOk},
+          {"/a/b/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", absl::StatusCode::kOk},
+          {"", absl::StatusCode::kInvalidArgument},
+          // Fails because we reject paths starting with '..'
+          {"..", absl::StatusCode::kInvalidArgument},
+          {"..a", absl::StatusCode::kInvalidArgument},
+          {"../a", absl::StatusCode::kInvalidArgument},
+          // Fails because is not absolute
+          {"a", absl::StatusCode::kInvalidArgument},
+          {"a/b", absl::StatusCode::kInvalidArgument},
+          {"a/b/c", absl::StatusCode::kInvalidArgument},
+          // Fails because '..' in path
+          {"/a/b/c/../d", absl::StatusCode::kInvalidArgument},
+          // Fails because '.' in path
+          {"/a/b/c/./d", absl::StatusCode::kInvalidArgument},
+          // Fails because '//' in path
+          {"/a/b/c//d", absl::StatusCode::kInvalidArgument},
+          // Fails because path ends with '/'
+          {"/a/b/c/d/", absl::StatusCode::kInvalidArgument},
+      };
+  for (auto const& [path, status] : kTestCases) {
+    EXPECT_THAT(PolicyBuilder().AddFile(path).TryBuild(), StatusIs(status));
+    EXPECT_THAT(PolicyBuilder().AddFileAt(path, "/input").TryBuild(),
+                StatusIs(status));
+    EXPECT_THAT(PolicyBuilder().AddDirectory(path).TryBuild(),
+                StatusIs(status));
+    EXPECT_THAT(PolicyBuilder().AddDirectoryAt(path, "/input").TryBuild(),
+                StatusIs(status));
   }
 
-  for (auto const& good_path :
-       {"/", "/a/b/c/d", "/a/b/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"}) {
-    SAPI_ASSERT_OK_AND_ASSIGN(
-        std::string path, PolicyBuilderPeer::ValidateAbsolutePath(good_path));
-    EXPECT_THAT(path, StrEq(good_path));
-  }
+  // Fails because it attempts to mount to '/' inside
+  EXPECT_THAT(PolicyBuilder().AddFile("/").TryBuild(),
+              StatusIs(absl::StatusCode::kInternal));
+  EXPECT_THAT(PolicyBuilder().AddDirectory("/").TryBuild(),
+              StatusIs(absl::StatusCode::kInternal));
+
+  // Succeeds because it attempts to mount to '/' inside
+  EXPECT_THAT(PolicyBuilder().AddFileAt("/a", "/input").TryBuild(), IsOk());
+  EXPECT_THAT(PolicyBuilder().AddDirectoryAt("/a", "/input").TryBuild(),
+              IsOk());
 }
 
 TEST(PolicyBuilderTest, TestCanOnlyBuildOnce) {