Bug 1380701 - Remove the file broker protocol support for two-path operations. r=gcp draft
authorJed Davis <jld@mozilla.com>
Wed, 16 Aug 2017 15:09:56 -0600
changeset 647731 f7b2873c74d34eecaa6eae2990d403bec43102bb
parent 647730 d4cd86718b87cf8241ff29725cc7d0e5cdf4ce1c
child 726627 e809fbba95780ce13b9c6db6d8d6c089c847facb
push id74533
push userbmo:jld@mozilla.com
push dateWed, 16 Aug 2017 22:03:39 +0000
reviewersgcp
bugs1380701
milestone57.0a1
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
security/sandbox/linux/SandboxBrokerClient.cpp
security/sandbox/linux/SandboxBrokerClient.h
security/sandbox/linux/broker/SandboxBroker.cpp
--- a/security/sandbox/linux/SandboxBrokerClient.cpp
+++ b/security/sandbox/linux/SandboxBrokerClient.cpp
@@ -30,18 +30,17 @@ SandboxBrokerClient::SandboxBrokerClient
 
 SandboxBrokerClient::~SandboxBrokerClient()
 {
   close(mFileDesc);
 }
 
 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
   // principle of least privilege and keep the broker as simple as
   // possible.  (Note: when pid namespaces happen, this will also need
   // to remap the inner pid to the outer pid.)
   // We only remap the first path.
   static const char kProcSelf[] = "/proc/self/";
@@ -59,48 +58,38 @@ SandboxBrokerClient::DoCall(const Reques
         SANDBOX_LOG_ERROR("rewriting %s -> %s", aPath, rewrittenPath);
       }
       path = rewrittenPath;
     } else {
       SANDBOX_LOG_ERROR("not rewriting unexpectedly long path %s", 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]);
     return -sendErrno;
   }
 
   // Set up iovecs for response.
   Response resp;
@@ -151,76 +140,76 @@ SandboxBrokerClient::DoCall(const Reques
   }
   return resp.mError;
 }
 
 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) {
       fcntl(maybeFd, F_SETFD, 0);
     }
   }
   return maybeFd;
 }
 
 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
 
--- a/security/sandbox/linux/SandboxBrokerClient.h
+++ b/security/sandbox/linux/SandboxBrokerClient.h
@@ -40,16 +40,15 @@ class SandboxBrokerClient final : privat
   int Rmdir(const char* aPath);
   int Readlink(const char* aPath, void* aBuf, size_t aBufSize);
 
  private:
   int mFileDesc;
 
   int DoCall(const Request* aReq,
              const char* aPath,
-             const char* aPath2,
              void *aReponseBuff,
              bool expectFd);
 };
 
 } // namespace mozilla
 
 #endif // mozilla_SandboxBrokerClient_h
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -496,37 +496,29 @@ SandboxBroker::ThreadMain(void)
 
   // Permissive mode can only be enabled through an environment variable,
   // therefore it is sufficient to fetch the value once
   // before the main thread loop starts
   bool permissive = SandboxInfo::Get().Test(SandboxInfo::kPermissive);
 
   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;
     int respfd;
 
     // 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) {
       if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
         SANDBOX_LOG_ERROR("EOF from pid %d", mChildPid);
       }
       break;
     }
@@ -548,80 +540,45 @@ SandboxBroker::ThreadMain(void)
     if (respfd == -1) {
       SANDBOX_LOG_ERROR("no response fd from pid %d", mChildPid);
       shutdown(mFileDesc, SHUT_RD);
       break;
     }
 
     // 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);
     ios[1].iov_base = nullptr;
     ios[1].iov_len = 0;
     int openedFd = -1;
 
-    // Clear permissions
-    int perms;
-
-    // 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;
+    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';
 
-      // 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?
-      // Then try to figure out the original path and see if that is readable.
-      if (!(perms & MAY_READ)) {
-          // Work on the original path,
-          // this reverses ConvertToRealPath above.
-          int symlinkPerms = SymlinkPermissions(recvBuf, first_len);
-          if (symlinkPerms > 0) {
-            perms = symlinkPerms;
-          }
+    // We don't have read permissions on the requested dir.
+    // Did we arrive from a symlink in a path that is not writable?
+    // Then try to figure out the original path and see if that is readable.
+    if (!(perms & MAY_READ)) {
+      // Work on the original path,
+      // this reverses ConvertToRealPath above.
+      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.
       resp.mError = -ENOSYS;
     } else if (permissive || perms & MAY_ACCESS) {
       // If the operation was only allowed because of permissive mode, log it.
@@ -721,19 +678,22 @@ SandboxBroker::ThreadMain(void)
           }
         } else {
           AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
         }
         break;
 
       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) {
+            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);
               nsDependentCString xlat(respBuf, respSize);
               if (!orig.Equals(xlat) && xlat[0] == '/') {
                 if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
                   SANDBOX_LOG_ERROR("Recording mapping %s -> %s",
                                     xlat.get(), orig.get());