Bug 1380701 - Remove the file broker protocol support for two-path operations. r=gcp

Now that all of the operations that took two paths are removed, we can
have less string manipulation running on untrusted inputs in a trusted
context.

Note that the path isn't null-terminated in transit, because we know
the message length and there's no longer any need to delimit anything.
(This is how the protocol worked before the two-path operations were
added.)

MozReview-Commit-ID: 5VHkMoPlWmU

--HG--
extra : rebase_source : 2108a4f7c7bf5098f2ef63786c3675367bd56e19

security/sandbox/linux/SandboxBrokerClient.cpp

@@ -35,8 +35,7 @@ SandboxBrokerClient::~SandboxBrokerClient()
 
 int
 SandboxBrokerClient::DoCall(const Request* aReq, const char* aPath,
-                            const char* aPath2, void* aResponseBuff,
-                            bool expectFd)
+                            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
@@ -64,38 +63,28 @@ SandboxBrokerClient::DoCall(const Request* aReq, const char* aPath,
     }
   }
 
-  struct iovec ios[3];
+  struct iovec ios[2];
   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) + 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;
-  }
+  ios[1].iov_len = strlen(path);
   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, 3, respFds[1]);
+  const ssize_t sent = SendWithFd(mFileDesc, ios, 2, respFds[1]);
   const int sendErrno = errno;
   MOZ_ASSERT(sent < 0 ||
              static_cast<size_t>(sent) == ios[0].iov_len
-                                        + ios[1].iov_len
-                                        + ios[2].iov_len);
+                                        + ios[1].iov_len);
+
   close(respFds[1]);
   if (sent < 0) {
     close(respFds[0]);
@@ -156,7 +145,7 @@ int
 SandboxBrokerClient::Open(const char* aPath, int aFlags)
 {
   Request req = { SANDBOX_FILE_OPEN, aFlags, 0 };
-  int maybeFd = DoCall(&req, aPath, nullptr, nullptr, true);
+  int maybeFd = DoCall(&req, aPath, nullptr, true);
   if (maybeFd >= 0) {
     // NSPR has opinions about file flags.  Fix O_CLOEXEC.
     if ((aFlags & O_CLOEXEC) == 0) {
@@ -170,56 +159,56 @@ int
 SandboxBrokerClient::Access(const char* aPath, int aMode)
 {
   Request req = { SANDBOX_FILE_ACCESS, aMode, 0 };
-  return DoCall(&req, aPath, nullptr, nullptr, false);
+  return DoCall(&req, aPath, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Stat(const char* aPath, statstruct* aStat)
 {
   Request req = { SANDBOX_FILE_STAT, 0, sizeof(statstruct) };
-  return DoCall(&req, aPath, nullptr, (void*)aStat, false);
+  return DoCall(&req, aPath, (void*)aStat, false);
 }
 
 int
 SandboxBrokerClient::LStat(const char* aPath, statstruct* aStat)
 {
   Request req = { SANDBOX_FILE_STAT, O_NOFOLLOW, sizeof(statstruct) };
-  return DoCall(&req, aPath, nullptr, (void*)aStat, false);
+  return DoCall(&req, aPath, (void*)aStat, false);
 }
 
 int
 SandboxBrokerClient::Chmod(const char* aPath, int aMode)
 {
   Request req = {SANDBOX_FILE_CHMOD, aMode, 0};
-  return DoCall(&req, aPath, nullptr, nullptr, false);
+  return DoCall(&req, aPath, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Mkdir(const char* aPath, int aMode)
 {
   Request req = {SANDBOX_FILE_MKDIR, aMode, 0};
-  return DoCall(&req, aPath, nullptr, nullptr, false);
+  return DoCall(&req, aPath, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Unlink(const char* aPath)
 {
   Request req = {SANDBOX_FILE_UNLINK, 0, 0};
-  return DoCall(&req, aPath, nullptr, nullptr, false);
+  return DoCall(&req, aPath, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Rmdir(const char* aPath)
 {
   Request req = {SANDBOX_FILE_RMDIR, 0, 0};
-  return DoCall(&req, aPath, nullptr, nullptr, false);
+  return DoCall(&req, aPath, nullptr, false);
 }
 
 int
 SandboxBrokerClient::Readlink(const char* aPath, void* aBuff, size_t aSize)
 {
   Request req = {SANDBOX_FILE_READLINK, 0, aSize};
-  return DoCall(&req, aPath, nullptr, aBuff, false);
+  return DoCall(&req, aPath, aBuff, false);
 }
 
 } // namespace mozilla

security/sandbox/linux/SandboxBrokerClient.h

@@ -45,7 +45,6 @@ class SandboxBrokerClient final : private SandboxBrokerCommon {
 
   int DoCall(const Request* aReq,
              const char* aPath,
-             const char* aPath2,
              void *aReponseBuff,
              bool expectFd);
 };

security/sandbox/linux/broker/SandboxBroker.cpp

@@ -501,12 +501,7 @@ SandboxBroker::ThreadMain(void)
 
   while (true) {
     struct iovec ios[2];
-    // 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 recvBuf[kMaxPathLen + 1];
     char respBuf[kMaxPathLen + 1]; // Also serves as struct stat
     Request req;
     Response resp;
@@ -515,13 +510,10 @@ 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);
+    ios[1].iov_len = sizeof(recvBuf) - 1;
 
     const ssize_t recvd = RecvWithFd(mFileDesc, ios, 2, &respfd);
     if (recvd == 0) {
@@ -553,7 +545,6 @@ 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);
@@ -561,35 +552,17 @@ SandboxBroker::ThreadMain(void)
     ios[1].iov_len = 0;
     int openedFd = -1;
 
-    // Clear permissions
-    int perms;
+    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';
 
-    // 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));
+    // 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));
 
     // We don't have read permissions on the requested dir.
     // Did we arrive from a symlink in a path that is not writable?
@@ -597,28 +570,12 @@ SandboxBroker::ThreadMain(void)
     if (!(perms & MAY_READ)) {
       // Work on the original path,
       // this reverses ConvertToRealPath above.
-          int symlinkPerms = SymlinkPermissions(recvBuf, first_len);
+      int symlinkPerms = SymlinkPermissions(recvBuf, origPathLen);
       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.
@@ -726,9 +683,12 @@ SandboxBroker::ThreadMain(void)
 
       case SANDBOX_FILE_READLINK:
         if (permissive || AllowOperation(R_OK, perms)) {
-          ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf));
+          ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf) - 1);
           if (respSize >= 0) {
             if (respSize > 0) {
+              // Null-terminate for nsDependentCString.
+              MOZ_RELEASE_ASSERT(static_cast<size_t>(respSize) < sizeof(respBuf));
+              respBuf[respSize] = '\0';
               // Record the mapping so we can invert the file to the original
               // symlink.
               nsDependentCString orig(pathBuf, pathLen);