Backed out 2 changesets (bug 1380701) for bustage in SandboxBroker a=backout

Backed out changeset 6cef83dd4d11 (bug 1380701) Backed out changeset 4456ebfe5657 (bug 1380701) MozReview-Commit-ID: Cnfj7TZvCbv
2017-08-16 09:46:48 -07:00 · 2017-08-16 09:46:48 -07:00 · 9a955fd0b3
--- a/security/sandbox/linux/SandboxBrokerClient.cpp
+++ b/security/sandbox/linux/SandboxBrokerClient.cpp
@ -35,7 +35,8 @@ SandboxBrokerClient::~SandboxBrokerClient()

 int
 SandboxBrokerClient::DoCall(const Request* aReq, const char* aPath,
-                            void* aResponseBuff, bool expectFd)
+                            const char* aPath2, void* aResponseBuff,
+                            bool expectFd)
 {
  // Remap /proc/self to the actual pid, so that the broker can open
  // it.  This happens here instead of in the broker to follow the
@ -63,28 +64,38 @@ SandboxBrokerClient::DoCall(const Request* aReq, const char* aPath,
    }
  }

-  struct iovec ios[2];
+  struct iovec ios[3];
  int respFds[2];

  // Set up iovecs for request + path.
  ios[0].iov_base = const_cast<Request*>(aReq);
  ios[0].iov_len = sizeof(*aReq);
  ios[1].iov_base = const_cast<char*>(path);
-  ios[1].iov_len = strlen(path);
+  ios[1].iov_len = strlen(path) + 1;
+  if (aPath2 != nullptr) {
+    ios[2].iov_base = const_cast<char*>(aPath2);
+    ios[2].iov_len = strlen(aPath2) + 1;
+  } else {
+    ios[2].iov_base = nullptr;
+    ios[2].iov_len = 0;
+  }
  if (ios[1].iov_len > kMaxPathLen) {
    return -ENAMETOOLONG;
  }
+  if (ios[2].iov_len > kMaxPathLen) {
+    return -ENAMETOOLONG;
+  }

  // Create response socket and send request.
  if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, respFds) < 0) {
    return -errno;
  }
-  const ssize_t sent = SendWithFd(mFileDesc, ios, 2, respFds[1]);
+  const ssize_t sent = SendWithFd(mFileDesc, ios, 3, respFds[1]);
  const int sendErrno = errno;
  MOZ_ASSERT(sent < 0 ||
             static_cast<size_t>(sent) == ios[0].iov_len
-                                        + ios[1].iov_len);
-
+                                        + ios[1].iov_len
+                                        + ios[2].iov_len);
  close(respFds[1]);
  if (sent < 0) {
    close(respFds[0]);
@ -145,7 +156,7 @@ int
 SandboxBrokerClient::Open(const char* aPath, int aFlags)
 {
  Request req = { SANDBOX_FILE_OPEN, aFlags, 0 };
-  int maybeFd = DoCall(&req, aPath, nullptr, true);
+  int maybeFd = DoCall(&req, aPath, nullptr, nullptr, true);
  if (maybeFd >= 0) {
    // NSPR has opinions about file flags.  Fix O_CLOEXEC.
    if ((aFlags & O_CLOEXEC) == 0) {
@ -159,56 +170,77 @@ int
 SandboxBrokerClient::Access(const char* aPath, int aMode)
 {
  Request req = { SANDBOX_FILE_ACCESS, aMode, 0 };
-  return DoCall(&req, aPath, nullptr, false);
+  return DoCall(&req, aPath, nullptr, nullptr, false);
 }

 int
 SandboxBrokerClient::Stat(const char* aPath, statstruct* aStat)
 {
  Request req = { SANDBOX_FILE_STAT, 0, sizeof(statstruct) };
-  return DoCall(&req, aPath, (void*)aStat, false);
+  return DoCall(&req, aPath, nullptr, (void*)aStat, false);
 }

 int
 SandboxBrokerClient::LStat(const char* aPath, statstruct* aStat)
 {
  Request req = { SANDBOX_FILE_STAT, O_NOFOLLOW, sizeof(statstruct) };
-  return DoCall(&req, aPath, (void*)aStat, false);
+  return DoCall(&req, aPath, nullptr, (void*)aStat, false);
 }

 int
 SandboxBrokerClient::Chmod(const char* aPath, int aMode)
 {
  Request req = {SANDBOX_FILE_CHMOD, aMode, 0};
-  return DoCall(&req, aPath, nullptr, false);
+  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, false);
+  return DoCall(&req, aPath, nullptr, nullptr, false);
 }

 int
 SandboxBrokerClient::Unlink(const char* aPath)
 {
  Request req = {SANDBOX_FILE_UNLINK, 0, 0};
-  return DoCall(&req, aPath, nullptr, false);
+  return DoCall(&req, aPath, nullptr, nullptr, false);
 }

 int
 SandboxBrokerClient::Rmdir(const char* aPath)
 {
  Request req = {SANDBOX_FILE_RMDIR, 0, 0};
-  return DoCall(&req, aPath, nullptr, false);
+  return DoCall(&req, aPath, nullptr, nullptr, false);
 }

 int
 SandboxBrokerClient::Readlink(const char* aPath, void* aBuff, size_t aSize)
 {
  Request req = {SANDBOX_FILE_READLINK, 0, aSize};
-  return DoCall(&req, aPath, aBuff, false);
+  return DoCall(&req, aPath, nullptr, aBuff, false);
 }

 } // namespace mozilla
--- a/security/sandbox/linux/SandboxBrokerClient.h
+++ b/security/sandbox/linux/SandboxBrokerClient.h
@ -35,7 +35,10 @@ class SandboxBrokerClient final : private SandboxBrokerCommon {
  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);
@ -45,6 +48,7 @@ class SandboxBrokerClient final : private SandboxBrokerCommon {

  int DoCall(const Request* aReq,
             const char* aPath,
+             const char* aPath2,
             void *aReponseBuff,
             bool expectFd);
 };
--- a/security/sandbox/linux/SandboxFilter.cpp
+++ b/security/sandbox/linux/SandboxFilter.cpp
@ -446,6 +446,27 @@ private:
    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]);
@ -594,8 +615,14 @@ public:
        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:
@ -614,7 +641,10 @@ public:
      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:
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@ -413,6 +413,19 @@ DoStat(const char* aPath, void* aBuff, int aFlags)
  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)
 {
@ -501,7 +514,12 @@ SandboxBroker::ThreadMain(void)

  while (true) {
    struct iovec ios[2];
-    char recvBuf[kMaxPathLen + 1];
+    // We will receive the path strings in 1 buffer and split them back up.
+    char recvBuf[2 * (kMaxPathLen + 1)];
+    char pathBuf[kMaxPathLen + 1];
+    char pathBuf2[kMaxPathLen + 1];
+    size_t pathLen = 0;
+    size_t pathLen2 = 0;
    char respBuf[kMaxPathLen + 1]; // Also serves as struct stat
    Request req;
    Response resp;
@ -510,10 +528,13 @@ SandboxBroker::ThreadMain(void)
    // Make sure stat responses fit in the response buffer
    MOZ_ASSERT((kMaxPathLen + 1) > sizeof(struct stat));

+    // This makes our string handling below a bit less error prone.
+    memset(recvBuf, 0, sizeof(recvBuf));
+
    ios[0].iov_base = &req;
    ios[0].iov_len = sizeof(req);
    ios[1].iov_base = recvBuf;
-    ios[1].iov_len = sizeof(recvBuf) - 1;
+    ios[1].iov_len = sizeof(recvBuf);

    const ssize_t recvd = RecvWithFd(mFileDesc, ios, 2, &respfd);
    if (recvd == 0) {
@ -545,6 +566,7 @@ SandboxBroker::ThreadMain(void)

    // Initialize the response with the default failure.
    memset(&resp, 0, sizeof(resp));
+    memset(&respBuf, 0, sizeof(respBuf));
    resp.mError = -EACCES;
    ios[0].iov_base = &resp;
    ios[0].iov_len = sizeof(resp);
@ -552,17 +574,35 @@ SandboxBroker::ThreadMain(void)
    ios[1].iov_len = 0;
    int openedFd = -1;

-    size_t origPathLen = static_cast<size_t>(recvd) - sizeof(req);
-    // Null-terminate to get a C-style string.
-    MOZ_RELEASE_ASSERT(origPathLen < sizeof(recvBuf));
-    recvBuf[origPathLen] = '\0';
+    // Clear permissions
+    int perms;

-    // Look up the pathname but first translate relative paths.
-    // (Make a copy so we can get back the original path if needed.)
-    char pathBuf[kMaxPathLen + 1];
-    base::strlcpy(pathBuf, recvBuf, sizeof(pathBuf));
-    size_t pathLen = ConvertToRealPath(pathBuf, sizeof(pathBuf), origPathLen);
-    int perms = mPolicy->Lookup(nsDependentCString(pathBuf, pathLen));
+    // Find end of first string, make sure the buffer is still
+    // 0 terminated.
+    size_t recvBufLen = static_cast<size_t>(recvd) - sizeof(req);
+    if (recvBufLen > 0 && recvBuf[recvBufLen - 1] != 0) {
+      SANDBOX_LOG_ERROR("corrupted path buffer from pid %d", mChildPid);
+      shutdown(mFileDesc, SHUT_RD);
+      break;
+    }
+
+    // First path should fit in maximum path length buffer.
+    size_t first_len = strlen(recvBuf);
+    if (first_len <= kMaxPathLen) {
+      strcpy(pathBuf, recvBuf);
+      // Skip right over the terminating 0, and try to copy in the
+      // second path, if any. If there's no path, this will hit a
+      // 0 immediately (we nulled the buffer before receiving).
+      // We do not assume the second path is 0-terminated, this is
+      // enforced below.
+      strncpy(pathBuf2, recvBuf + first_len + 1, kMaxPathLen + 1);
+
+      // First string is guaranteed to be 0-terminated.
+      pathLen = first_len;
+
+      // Look up the first pathname but first translate relative paths.
+      pathLen = ConvertToRealPath(pathBuf, sizeof(pathBuf), pathLen);
+      perms = mPolicy->Lookup(nsDependentCString(pathBuf, pathLen));

      // We don't have read permissions on the requested dir.
      // Did we arrive from a symlink in a path that is not writable?
@ -570,12 +610,28 @@ SandboxBroker::ThreadMain(void)
      if (!(perms & MAY_READ)) {
          // Work on the original path,
          // this reverses ConvertToRealPath above.
-      int symlinkPerms = SymlinkPermissions(recvBuf, origPathLen);
+          int symlinkPerms = SymlinkPermissions(recvBuf, first_len);
          if (symlinkPerms > 0) {
            perms = symlinkPerms;
          }
      }

+      // Same for the second path.
+      pathLen2 = strnlen(pathBuf2, kMaxPathLen);
+      if (pathLen2 > 0) {
+        // Force 0 termination.
+        pathBuf2[pathLen2] = '\0';
+        pathLen2 = ConvertToRealPath(pathBuf2, sizeof(pathBuf2), pathLen2);
+        int perms2 = mPolicy->Lookup(nsDependentCString(pathBuf2, pathLen2));
+
+        // Take the intersection of the permissions for both paths.
+        perms &= perms2;
+      }
+    } else {
+      // Failed to receive intelligible paths.
+      perms = 0;
+    }
+
    // And now perform the operation if allowed.
    if (perms & CRASH_INSTEAD) {
      // This is somewhat nonmodular, but it works.
@ -638,6 +694,31 @@ SandboxBroker::ThreadMain(void)
        }
        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) {
@ -683,12 +764,9 @@ SandboxBroker::ThreadMain(void)

      case SANDBOX_FILE_READLINK:
        if (permissive || AllowOperation(R_OK, perms)) {
-          ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf) - 1);
+          ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf));
          if (respSize >= 0) {
              if (respSize > 0) {
-              // Null-terminate for nsDependentCString.
-              MOZ_RELEASE_ASSERT(respSize < sizeof(respBuf));
-              respBuf[respSize] = '\0';
              // Record the mapping so we can invert the file to the original
              // symlink.
              nsDependentCString orig(pathBuf, pathLen);
--- a/security/sandbox/linux/broker/SandboxBrokerCommon.h
+++ b/security/sandbox/linux/broker/SandboxBrokerCommon.h
@ -30,7 +30,10 @@ public:
    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,
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@ -64,9 +64,18 @@ protected:
  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);
  }
@ -271,6 +280,43 @@ TEST_F(SandboxBrokerTest, Chmod)
  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();
@ -290,6 +336,24 @@ TEST_F(SandboxBrokerTest, Mkdir)
  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();
@ -332,7 +396,7 @@ 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"));
+  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)));