Bug 1380701 - Remove brokering for link, unlink, and rename. r=gcp draft
authorJed Davis <jld@mozilla.com>
Thu, 20 Jul 2017 13:43:59 -0600
changeset 647730 d4cd86718b87cf8241ff29725cc7d0e5cdf4ce1c
parent 647460 6ebc251bd288c268b020815025b05854ccde5c08
child 647731 f7b2873c74d34eecaa6eae2990d403bec43102bb
push id74533
push userbmo:jld@mozilla.com
push dateWed, 16 Aug 2017 22:03:39 +0000
reviewersgcp
bugs1380701
milestone57.0a1
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();
 }