Bug 2040515 - Rip out the symlink brokering. r=Thinker

This is almost certainly unused.  Bug 1380701 comment #23 mentioned
PulseAudio in content processes, which is no longer supported.  (If
you flip `media.cubeb.sandbox` for testing, you'll need to turn off
sandboxing; you should not flip that pref for production use.)

In general there are very few places left where sandboxed processes can
create files, and none of them should need symlinks.  However, there
might be libraries which were calling symlink and silently failing.

Given all of that, this patch does not return symlink/symlinkat to being
unexpected syscalls (crash on Nightly, ENOSYS otherwise) but instead just
makes them quietly fail with EPERM.  (This happens to match macOS, it's a
reasonable error code, and tests can distinguish it from the EACCES of a
broker rejection.)

Differential Revision: https://phabricator.services.mozilla.com/D301463

Author: jld

security/sandbox/common/test/SandboxTestingChildTests.h

@@ -627,6 +627,12 @@ void RunTestsContent(SandboxTestingChild* child) {
     return fd;
   });
 
+  child->ErrnoValueTest("symlink"_ns, EPERM,
+                        [] { return symlink("something", "/tmp/testlink"); });
+  child->ErrnoValueTest("symlinkat"_ns, EPERM, [] {
+    return symlinkat("something", AT_FDCWD, "/tmp/testlink");
+  });
+
 #  endif  // XP_LINUX
 
 #  ifdef XP_MACOSX

security/sandbox/linux/SandboxBrokerClient.cpp

@@ -205,11 +205,6 @@ int SandboxBrokerClient::Link(const char* aOldPath, const char* aNewPath) {
   return DoCall(&req, aOldPath, aNewPath, nullptr, false);
 }
 
-int SandboxBrokerClient::Symlink(const char* aOldPath, const char* aNewPath) {
-  Request req = MakeRequest(SANDBOX_FILE_SYMLINK, 0, 0);
-  return DoCall(&req, aOldPath, aNewPath, nullptr, false);
-}
-
 int SandboxBrokerClient::Rename(const char* aOldPath, const char* aNewPath) {
   Request req = MakeRequest(SANDBOX_FILE_RENAME, 0, 0);
   return DoCall(&req, aOldPath, aNewPath, nullptr, false);

security/sandbox/linux/SandboxBrokerClient.h

@@ -34,7 +34,6 @@ class SandboxBrokerClient final : private SandboxBrokerCommon {
   int Chmod(const char* aPath, int aMode);
   int Link(const char* aPath, const char* aPath2);
   int Mkdir(const char* aPath, int aMode);
-  int Symlink(const char* aOldPath, const char* aNewPath);
   int Rename(const char* aOldPath, const char* aNewPath);
   int Unlink(const char* aPath);
   int Rmdir(const char* aPath);

security/sandbox/linux/SandboxFilter.cpp

@@ -299,13 +299,6 @@ class SandboxPolicyCommon : public SandboxPolicyBase {
     return broker->Link(path, path2);
   }
 
-  static intptr_t SymlinkTrap(ArgsRef aArgs, void* aux) {
-    auto broker = static_cast<SandboxBrokerClient*>(aux);
-    auto path = reinterpret_cast<const char*>(aArgs.args[0]);
-    auto path2 = reinterpret_cast<const char*>(aArgs.args[1]);
-    return broker->Symlink(path, path2);
-  }
-
   static intptr_t RenameTrap(ArgsRef aArgs, void* aux) {
     auto broker = static_cast<SandboxBrokerClient*>(aux);
     auto path = reinterpret_cast<const char*>(aArgs.args[0]);
@@ -472,19 +465,6 @@ class SandboxPolicyCommon : public SandboxPolicyBase {
     return broker->Link(path, path2);
   }
 
-  static intptr_t SymlinkAtTrap(ArgsRef aArgs, void* aux) {
-    auto broker = static_cast<SandboxBrokerClient*>(aux);
-    auto path = reinterpret_cast<const char*>(aArgs.args[0]);
-    auto fd2 = static_cast<int>(aArgs.args[1]);
-    auto path2 = reinterpret_cast<const char*>(aArgs.args[2]);
-    if (fd2 != AT_FDCWD && path2[0] != '/') {
-      SANDBOX_LOG("unsupported fd-relative symlinkat(\"%s\", %d, \"%s\")", path,
-                  fd2, path2);
-      return BlockedSyscallTrap(aArgs, nullptr);
-    }
-    return broker->Symlink(path, path2);
-  }
-
   static intptr_t RenameAtTrap(ArgsRef aArgs, void* aux) {
     auto broker = static_cast<SandboxBrokerClient*>(aux);
     auto fd = static_cast<int>(aArgs.args[0]);
@@ -966,7 +946,7 @@ class SandboxPolicyCommon : public SandboxPolicyBase {
         case __NR_mkdir:
           return Trap(MkdirTrap, mBroker);
         case __NR_symlink:
-          return Trap(SymlinkTrap, mBroker);
+          return Error(EPERM);
         case __NR_rename:
           return Trap(RenameTrap, mBroker);
         case __NR_rmdir:
@@ -995,7 +975,7 @@ class SandboxPolicyCommon : public SandboxPolicyBase {
         case __NR_mkdirat:
           return Trap(MkdirAtTrap, mBroker);
         case __NR_symlinkat:
-          return Trap(SymlinkAtTrap, mBroker);
+          return Error(EPERM);
         case __NR_renameat:
           return Trap(RenameAtTrap, mBroker);
         case __NR_unlinkat:

security/sandbox/linux/broker/SandboxBroker.cpp

@@ -529,17 +529,6 @@ static int DoStat(const char* aPath, statstruct* aBuff, int aFlags) {
   return statsyscall(aPath, aBuff);
 }
 
-static int DoLink(const char* aPath, const char* aPath2,
-                  SandboxBrokerCommon::Operation aOper) {
-  if (aOper == SandboxBrokerCommon::Operation::SANDBOX_FILE_LINK) {
-    return link(aPath, aPath2);
-  }
-  if (aOper == SandboxBrokerCommon::Operation::SANDBOX_FILE_SYMLINK) {
-    return symlink(aPath, aPath2);
-  }
-  MOZ_CRASH("SandboxBroker: Unknown link operation");
-}
-
 static int DoConnect(const char* aPath, size_t aLen, int aType,
                      bool aIsAbstract) {
   // Deny SOCK_DGRAM for the same reason it's denied for socketpair.
@@ -910,9 +899,8 @@ void SandboxBroker::ThreadMain(void) {
           break;
 
         case SANDBOX_FILE_LINK:
-        case SANDBOX_FILE_SYMLINK:
           if (permissive || AllowOperation(W_OK | X_OK, perms)) {
-            if (DoLink(pathBuf, pathBuf2, req.mOp) == 0) {
+            if (link(pathBuf, pathBuf2) == 0) {
               resp.mError = 0;
             } else {
               resp.mError = -errno;

security/sandbox/linux/broker/SandboxBrokerCommon.cpp

@@ -46,19 +46,8 @@ unsigned SandboxBrokerCommon::OperationToInt(Operation aOp) {
 // static
 const char* SandboxBrokerCommon::OperationDescription(Operation aOp) {
   static constexpr const char* kNames[] = {
-      "open",
-      "access",
-      "stat",
-      "chmod",
-      "link",
-      "symlink",
-      "mkdir",
-      "rename",
-      "rmdir",
-      "unlink",
-      "readlink",
-      "connect",
-      "connect-abstract",
+      "open",   "access", "stat",   "chmod",    "link",    "mkdir",
+      "rename", "rmdir",  "unlink", "readlink", "connect", "connect-abstract",
   };
 
   static_assert(

security/sandbox/linux/broker/SandboxBrokerCommon.h

@@ -32,7 +32,6 @@ class SandboxBrokerCommon {
     SANDBOX_FILE_STAT,
     SANDBOX_FILE_CHMOD,
     SANDBOX_FILE_LINK,
-    SANDBOX_FILE_SYMLINK,
     SANDBOX_FILE_MKDIR,
     SANDBOX_FILE_RENAME,
     SANDBOX_FILE_RMDIR,
@@ -44,11 +43,11 @@ class SandboxBrokerCommon {
   };
   MOZ_USING_ENUM_STATIC(Operation, SANDBOX_FILE_OPEN, SANDBOX_FILE_ACCESS,
                         SANDBOX_FILE_STAT, SANDBOX_FILE_CHMOD,
-                        SANDBOX_FILE_LINK, SANDBOX_FILE_SYMLINK,
-                        SANDBOX_FILE_MKDIR, SANDBOX_FILE_RENAME,
-                        SANDBOX_FILE_RMDIR, SANDBOX_FILE_UNLINK,
-                        SANDBOX_FILE_READLINK, SANDBOX_SOCKET_CONNECT,
-                        SANDBOX_SOCKET_CONNECT_ABSTRACT, SANDBOX_OP_MAX_VALUE);
+                        SANDBOX_FILE_LINK, SANDBOX_FILE_MKDIR,
+                        SANDBOX_FILE_RENAME, SANDBOX_FILE_RMDIR,
+                        SANDBOX_FILE_UNLINK, SANDBOX_FILE_READLINK,
+                        SANDBOX_SOCKET_CONNECT, SANDBOX_SOCKET_CONNECT_ABSTRACT,
+                        SANDBOX_OP_MAX_VALUE);
 
   static bool OperationIsValid(Operation aOp) {
     return static_cast<unsigned>(aOp) <=

security/sandbox/linux/gtest/TestBroker.cpp

@@ -68,9 +68,6 @@ class SandboxBrokerTest : public ::testing::Test {
   int Mkdir(const char* aPath, int aMode) {
     return mClient->Mkdir(aPath, aMode);
   }
-  int Symlink(const char* aPath, const char* bPath) {
-    return mClient->Symlink(aPath, bPath);
-  }
   int Rename(const char* aPath, const char* bPath) {
     return mClient->Rename(aPath, bPath);
   }
@@ -306,25 +303,6 @@ TEST_F(SandboxBrokerTest, Link) {
   PrePostTestCleanup();
 }
 
-TEST_F(SandboxBrokerTest, Symlink) {
-  PrePostTestCleanup();
-
-  int fd = Open("/tmp/blublu", O_WRONLY | O_CREAT);
-  ASSERT_GE(fd, 0) << "Opening /tmp/blublu for writing failed.";
-  close(fd);
-  ASSERT_EQ(0, Symlink("/tmp/blublu", "/tmp/blublublu"));
-  EXPECT_EQ(0, Access("/tmp/blublublu", F_OK));
-  statstruct aStat;
-  ASSERT_EQ(0, lstatsyscall("/tmp/blublublu", &aStat));
-  EXPECT_EQ((mode_t)S_IFLNK, aStat.st_mode & S_IFMT);
-  // Not whitelisted target path
-  EXPECT_EQ(-EACCES, Symlink("/tmp/blublu", "/tmp/nope"));
-  EXPECT_EQ(0, unlink("/tmp/blublublu"));
-  EXPECT_EQ(0, unlink("/tmp/blublu"));
-
-  PrePostTestCleanup();
-}
-
 TEST_F(SandboxBrokerTest, Mkdir) {
   PrePostTestCleanup();
 
@@ -400,7 +378,8 @@ TEST_F(SandboxBrokerTest, Readlink) {
   int fd = Open("/tmp/blublu", O_WRONLY | O_CREAT);
   ASSERT_GE(fd, 0) << "Opening /tmp/blublu for writing failed.";
   close(fd);
-  ASSERT_EQ(0, Symlink("/tmp/blublu", "/tmp/blublublu"));
+  // This is the real symlink() now that there's no broker support.
+  ASSERT_EQ(0, symlink("/tmp/blublu", "/tmp/blublublu"));
   EXPECT_EQ(0, Access("/tmp/blublublu", F_OK));
   char linkBuff[256];
   EXPECT_EQ(11, Readlink("/tmp/blublublu", linkBuff, sizeof(linkBuff)));

security/sandbox/test/browser_content_sandbox_fs_tests.js

@@ -75,10 +75,9 @@ async function createTempFile() {
       createSymlink
     );
     ok(!symlinkCreated.ok, "created a symlink in temp failed");
-    const expectedError = isLinux() ? lazy.LIBC.EACCES : lazy.LIBC.EPERM;
     is(
       symlinkCreated.code,
-      expectedError,
+      lazy.LIBC.EPERM,
       "created a symlink in temp failed with access denied"
     );
   }