Bug 1380701 - Remove brokering for link, unlink, and rename. r=gcp
authorJed Davis <jld@mozilla.com>
Thu, 20 Jul 2017 13:43:59 -0600
changeset 424674 0d56979a6efae4883ab2bd015dae79271b832725
parent 424673 1770d65a34281a5a8ee16b2febc9b017a5d1ed6b
child 424675 9fb892c41a9e6cc0d22f4f8fd14e0c0ae92a432f
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgcp
bugs1380701
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1380701 - Remove brokering for link, unlink, and rename. r=gcp In testing (local and CI) these seem to no longer be used. MozReview-Commit-ID: 2D3C8eWoIsB
security/sandbox/linux/SandboxBrokerClient.cpp
security/sandbox/linux/SandboxBrokerClient.h
security/sandbox/linux/SandboxFilter.cpp
security/sandbox/linux/broker/SandboxBroker.cpp
security/sandbox/linux/broker/SandboxBrokerCommon.h
security/sandbox/linux/gtest/TestBroker.cpp
--- a/security/sandbox/linux/SandboxBrokerClient.cpp
+++ b/security/sandbox/linux/SandboxBrokerClient.cpp
@@ -190,37 +190,16 @@ SandboxBrokerClient::LStat(const char* a
 int
 SandboxBrokerClient::Chmod(const char* aPath, int aMode)
 {
   Request req = {SANDBOX_FILE_CHMOD, aMode, 0};
   return DoCall(&req, aPath, nullptr, nullptr, false);
 }
 
 int
-SandboxBrokerClient::Link(const char* aOldPath, const char* aNewPath)
-{
-  Request req = {SANDBOX_FILE_LINK, 0, 0};
-  return DoCall(&req, aOldPath, aNewPath, nullptr, false);
-}
-
-int
-SandboxBrokerClient::Symlink(const char* aOldPath, const char* aNewPath)
-{
-  Request req = {SANDBOX_FILE_SYMLINK, 0, 0};
-  return DoCall(&req, aOldPath, aNewPath, nullptr, false);
-}
-
-int
-SandboxBrokerClient::Rename(const char* aOldPath, const char* aNewPath)
-{
-  Request req = {SANDBOX_FILE_RENAME, 0, 0};
-  return DoCall(&req, aOldPath, aNewPath, nullptr, false);
-}
-
-int
 SandboxBrokerClient::Mkdir(const char* aPath, int aMode)
 {
   Request req = {SANDBOX_FILE_MKDIR, aMode, 0};
   return DoCall(&req, aPath, nullptr, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Unlink(const char* aPath)
--- a/security/sandbox/linux/SandboxBrokerClient.h
+++ b/security/sandbox/linux/SandboxBrokerClient.h
@@ -30,20 +30,17 @@ class SandboxBrokerClient final : privat
   explicit SandboxBrokerClient(int aFd);
   ~SandboxBrokerClient();
 
   int Open(const char* aPath, int aFlags);
   int Access(const char* aPath, int aMode);
   int Stat(const char* aPath, statstruct* aStat);
   int LStat(const char* aPath, statstruct* aStat);
   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);
   int Readlink(const char* aPath, void* aBuf, size_t aBufSize);
 
  private:
   int mFileDesc;
 
   int DoCall(const Request* aReq,
--- a/security/sandbox/linux/SandboxFilter.cpp
+++ b/security/sandbox/linux/SandboxFilter.cpp
@@ -441,37 +441,16 @@ private:
 
   static intptr_t ChmodTrap(ArgsRef aArgs, void* aux) {
     auto broker = static_cast<SandboxBrokerClient*>(aux);
     auto path = reinterpret_cast<const char*>(aArgs.args[0]);
     auto mode = static_cast<mode_t>(aArgs.args[1]);
     return broker->Chmod(path, mode);
   }
 
-  static intptr_t LinkTrap(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->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]);
-    auto path2 = reinterpret_cast<const char*>(aArgs.args[1]);
-    return broker->Rename(path, path2);
-  }
-
   static intptr_t MkdirTrap(ArgsRef aArgs, void* aux) {
     auto broker = static_cast<SandboxBrokerClient*>(aux);
     auto path = reinterpret_cast<const char*>(aArgs.args[0]);
     auto mode = static_cast<mode_t>(aArgs.args[1]);
     return broker->Mkdir(path, mode);
   }
 
   static intptr_t RmdirTrap(ArgsRef aArgs, void* aux) {
@@ -610,24 +589,18 @@ public:
       CASES_FOR_stat:
         return Trap(StatTrap, mBroker);
       CASES_FOR_lstat:
         return Trap(LStatTrap, mBroker);
       CASES_FOR_fstatat:
         return Trap(StatAtTrap, mBroker);
       case __NR_chmod:
         return Trap(ChmodTrap, mBroker);
-      case __NR_link:
-        return Trap(LinkTrap, mBroker);
       case __NR_mkdir:
         return Trap(MkdirTrap, mBroker);
-      case __NR_symlink:
-        return Trap(SymlinkTrap, mBroker);
-      case __NR_rename:
-        return Trap(RenameTrap, mBroker);
       case __NR_rmdir:
         return Trap(RmdirTrap, mBroker);
       case __NR_unlink:
         return Trap(UnlinkTrap, mBroker);
       case __NR_readlink:
         return Trap(ReadlinkTrap, mBroker);
       }
     } else {
@@ -636,20 +609,17 @@ public:
       case __NR_open:
       case __NR_openat:
       case __NR_access:
       case __NR_faccessat:
       CASES_FOR_stat:
       CASES_FOR_lstat:
       CASES_FOR_fstatat:
       case __NR_chmod:
-      case __NR_link:
       case __NR_mkdir:
-      case __NR_symlink:
-      case __NR_rename:
       case __NR_rmdir:
       case __NR_unlink:
       case __NR_readlink:
         return Allow();
       }
     }
 
     switch (sysno) {
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -408,29 +408,16 @@ static int
 DoStat(const char* aPath, void* aBuff, int aFlags)
 {
  if (aFlags & O_NOFOLLOW) {
     return lstatsyscall(aPath, (statstruct*)aBuff);
   }
   return statsyscall(aPath, (statstruct*)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");
-}
-
 size_t
 SandboxBroker::ConvertToRealPath(char* aPath, size_t aBufSize, size_t aPathLen)
 {
   if (strstr(aPath, "..") != nullptr) {
     char* result = realpath(aPath, nullptr);
     if (result != nullptr) {
       base::strlcpy(aPath, result, aBufSize);
       free(result);
@@ -689,41 +676,16 @@ SandboxBroker::ThreadMain(void)
           } else {
             resp.mError = -errno;
           }
         } else {
           AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
         }
         break;
 
-      case SANDBOX_FILE_LINK:
-      case SANDBOX_FILE_SYMLINK:
-        if (permissive || AllowOperation(W_OK, perms)) {
-          if (DoLink(pathBuf, pathBuf2, req.mOp) == 0) {
-            resp.mError = 0;
-          } else {
-            resp.mError = -errno;
-          }
-        } else {
-          AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
-        }
-        break;
-
-      case SANDBOX_FILE_RENAME:
-        if (permissive || AllowOperation(W_OK, perms)) {
-          if (rename(pathBuf, pathBuf2) == 0) {
-            resp.mError = 0;
-          } else {
-            resp.mError = -errno;
-          }
-        } else {
-          AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
-        }
-        break;
-
       case SANDBOX_FILE_MKDIR:
         if (permissive || AllowOperation(W_OK | X_OK, perms)) {
           if (mkdir(pathBuf, req.mFlags) == 0) {
             resp.mError = 0;
           } else {
             resp.mError = -errno;
           }
         } else {
--- a/security/sandbox/linux/broker/SandboxBrokerCommon.h
+++ b/security/sandbox/linux/broker/SandboxBrokerCommon.h
@@ -25,20 +25,17 @@ namespace mozilla {
 
 class SandboxBrokerCommon {
 public:
   enum 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,
   };
   // String versions of the above
   static const char* OperationDescription[];
 
   struct Request {
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -59,28 +59,19 @@ protected:
     return mClient->Stat(aPath, aStat);
   }
   int LStat(const char* aPath, statstruct* aStat) {
     return mClient->LStat(aPath, aStat);
   }
   int Chmod(const char* aPath, int aMode) {
     return mClient->Chmod(aPath, aMode);
   }
-  int Link(const char* aPath, const char* bPath) {
-    return mClient->Link(aPath, bPath);
-  }
   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);
-  }
   int Rmdir(const char* aPath) {
     return mClient->Rmdir(aPath);
   }
   int Unlink(const char* aPath) {
     return mClient->Unlink(aPath);
   }
   ssize_t Readlink(const char* aPath, char* aBuff, size_t aSize) {
     return mClient->Readlink(aPath, aBuff, aSize);
@@ -275,53 +266,16 @@ TEST_F(SandboxBrokerTest, Chmod)
   ASSERT_EQ(0, Chmod("/tmp/blublu", S_IRUSR | S_IWUSR));
   EXPECT_EQ(0, statsyscall("/tmp/blublu", &realStat));
   EXPECT_EQ((mode_t)(S_IRUSR | S_IWUSR), realStat.st_mode & 0777);
   EXPECT_EQ(0, unlink("/tmp/blublu"));
 
   PrePostTestCleanup();
 }
 
-TEST_F(SandboxBrokerTest, Link)
-{
-  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, Link("/tmp/blublu", "/tmp/blublublu"));
-  EXPECT_EQ(0, Access("/tmp/blublublu", F_OK));
-  // Not whitelisted target path
-  EXPECT_EQ(-EACCES, Link("/tmp/blublu", "/tmp/nope"));
-  EXPECT_EQ(0, unlink("/tmp/blublublu"));
-  EXPECT_EQ(0, unlink("/tmp/blublu"));
-
-  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();
 
   ASSERT_EQ(0, mkdir("/tmp/blublu", 0600))
     << "Creating dir /tmp/blublu failed.";
   EXPECT_EQ(0, Access("/tmp/blublu", F_OK));
   // Not whitelisted target path
@@ -331,34 +285,16 @@ TEST_F(SandboxBrokerTest, Mkdir)
   EXPECT_EQ(-EEXIST, Mkdir("/proc/self", 0600))
     << "Creating uncreatable dir that already exists didn't fail correctly.";
   EXPECT_EQ(-EEXIST, Mkdir("/dev/zero", 0600))
     << "Creating uncreatable dir over preexisting file didn't fail correctly.";
 
   PrePostTestCleanup();
 }
 
-TEST_F(SandboxBrokerTest, Rename)
-{
-  PrePostTestCleanup();
-
-  ASSERT_EQ(0, mkdir("/tmp/blublu", 0600))
-    << "Creating dir /tmp/blublu failed.";
-  EXPECT_EQ(0, Access("/tmp/blublu", F_OK));
-  ASSERT_EQ(0, Rename("/tmp/blublu", "/tmp/blublublu"));
-  EXPECT_EQ(0, Access("/tmp/blublublu", F_OK));
-  EXPECT_EQ(-ENOENT , Access("/tmp/blublu", F_OK));
-  // Not whitelisted target path
-  EXPECT_EQ(-EACCES, Rename("/tmp/blublublu", "/tmp/nope"))
-    << "Renaming dir without write access succeed.";
-  EXPECT_EQ(0, rmdir("/tmp/blublublu"));
-
-  PrePostTestCleanup();
-}
-
 TEST_F(SandboxBrokerTest, Rmdir)
 {
   PrePostTestCleanup();
 
   ASSERT_EQ(0, mkdir("/tmp/blublu", 0600))
     << "Creating dir /tmp/blublu failed.";
   EXPECT_EQ(0, Access("/tmp/blublu", F_OK));
   ASSERT_EQ(0, Rmdir("/tmp/blublu"));
@@ -391,17 +327,17 @@ TEST_F(SandboxBrokerTest, Unlink)
 
 TEST_F(SandboxBrokerTest, Readlink)
 {
   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"));
+  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)));
   linkBuff[11] = '\0';
   EXPECT_EQ(0, strcmp(linkBuff, "/tmp/blublu"));
 
   PrePostTestCleanup();
 }