Bug 1289718 - Implement mkdir. draft
authorGian-Carlo Pascutto <gcp@mozilla.com>
Wed, 31 Aug 2016 19:24:28 +0200
changeset 409936 c6db11730ee842db0167eabea4d2c132100ed3de
parent 409935 5df5fcf01590a9138c3ca72371e5e1d3498cf85b
child 409937 0694a1b511323346af25c9c743f34436386d024f
push id28613
push usergpascutto@mozilla.com
push dateMon, 05 Sep 2016 18:00:21 +0000
bugs1289718
milestone51.0a1
Bug 1289718 - Implement mkdir. MozReview-Commit-ID: 6r6sqOVekVu
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
@@ -200,10 +200,17 @@ SandboxBrokerClient::Chmod(const char* a
 
 int
 SandboxBrokerClient::Link(const char* aOldPath, const char* aNewPath)
 {
   Request req = {SANDBOX_FILE_LINK, 0};
   return DoCall(&req, aOldPath, aNewPath, nullptr, false);
 }
 
+int
+SandboxBrokerClient::Mkdir(const char* aPath, int aMode)
+{
+  Request req = {SANDBOX_FILE_MKDIR, aMode};
+  return DoCall(&req, aPath, nullptr, nullptr, false);
+}
+
 } // namespace mozilla
 
--- a/security/sandbox/linux/SandboxBrokerClient.h
+++ b/security/sandbox/linux/SandboxBrokerClient.h
@@ -30,16 +30,17 @@ class SandboxBrokerClient final : privat
   ~SandboxBrokerClient();
 
   int Open(const char* aPath, int aFlags);
   int Access(const char* aPath, int aMode);
   int Stat(const char* aPath, struct stat* aStat);
   int LStat(const char* aPath, struct stat* aStat);
   int Chmod(const char* aPath, int aMode);
   int Link(const char* aPath, const char* aPath2);
+  int Mkdir(const char* aPath, int aMode);
 
  private:
   int mFileDesc;
 
   int DoCall(const Request* aReq,
              const char* aPath,
              const char* aPath2,
              struct stat* aStat,
--- a/security/sandbox/linux/SandboxFilter.cpp
+++ b/security/sandbox/linux/SandboxFilter.cpp
@@ -418,16 +418,23 @@ class ContentSandboxPolicy : public Sand
 
   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 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 GetPPidTrap(ArgsRef aArgs, void* aux) {
     // In a pid namespace, getppid() will return 0. We will return 0 instead
     // of the real parent pid to see what breaks when we introduce the
     // pid namespace (Bug 1151624).
     return 0;
   }
 
 public:
@@ -519,16 +526,18 @@ public:
       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);
       }
     } else {
       // No broker; allow the syscalls directly.  )-:
       switch(sysno) {
       case __NR_open:
       case __NR_openat:
       case __NR_access:
       case __NR_faccessat:
@@ -541,17 +550,16 @@ public:
 
     switch (sysno) {
 #ifdef DESKTOP
     case __NR_getppid:
       return Trap(GetPPidTrap, nullptr);
 
       // Filesystem syscalls that need more work to determine who's
       // using them, if they need to be, and what we intend to about it.
-    case __NR_mkdir:
     case __NR_rmdir:
     case __NR_getcwd:
     CASES_FOR_statfs:
     CASES_FOR_fstatfs:
     case __NR_rename:
     case __NR_symlink:
     case __NR_quotactl:
     case __NR_unlink:
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -286,16 +286,22 @@ AllowOperation(int aReqFlags, int aPerms
 {
   int needed = 0;
   if (aReqFlags & R_OK) {
     needed |= SandboxBroker::MAY_READ;
   }
   if (aReqFlags & W_OK) {
     needed |= SandboxBroker::MAY_WRITE;
   }
+  // We don't really allow executing anything,
+  // so in true unix tradition we hijack this
+  // for directories.
+  if (aReqFlags & X_OK) {
+    needed |= SandboxBroker::MAY_CREATE;
+  }
   return (aPerms & needed) == needed;
 }
 
 static bool
 AllowAccess(int aReqFlags, int aPerms)
 {
   if (aReqFlags & ~(R_OK|W_OK|F_OK)) {
     return false;
@@ -370,16 +376,22 @@ DoChmod(const char* aPath, int aMode)
 }
 
 static int
 DoLink(const char* aPath, const char* aPath2)
 {
   return link(aPath, aPath2);
 }
 
+static int
+DoMkdir(const char* aPath, int aMode)
+{
+  return mkdir(aPath, aMode);
+}
+
 size_t
 SandboxBroker::ConvertToRealPath(char* aPath, size_t aBufSize, size_t aPathLen)
 {
   if (strstr(aPath, "..") != NULL) {
     char* result = realpath(aPath, NULL);
     if (result != NULL) {
       strncpy(aPath, result, aBufSize);
       aPath[aBufSize - 1] = '\0';
@@ -567,33 +579,43 @@ SandboxBroker::ThreadMain(void)
           ios[1].iov_len = sizeof(statBuf);
         } else {
           resp.mError = errno;
         }
         break;
 
       case SANDBOX_FILE_CHMOD:
         if (permissive || AllowOperation(W_OK, perms)) {
-          if (DoChmod(pathBuf,req.mFlags) == 0) {
+          if (DoChmod(pathBuf, req.mFlags) == 0) {
             resp.mError = 0;
           } else {
             resp.mError = errno;
           }
         }
         break;
 
       case SANDBOX_FILE_LINK:
         if (permissive || AllowOperation(W_OK, perms)) {
           if (DoLink(pathBuf, pathBuf2) == 0) {
             resp.mError = 0;
           } else {
             resp.mError = errno;
           }
         }
         break;
+
+      case SANDBOX_FILE_MKDIR:
+        if (permissive || AllowOperation(W_OK | X_OK, perms)) {
+          if (DoMkdir(pathBuf, req.mFlags) == 0) {
+            resp.mError = 0;
+          } else {
+            resp.mError = errno;
+          }
+        }
+        break;
       }
     } else {
       MOZ_ASSERT(perms == 0);
       AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
     }
 
     const size_t numIO = ios[1].iov_len > 0 ? 2 : 1;
     DebugOnly<const ssize_t> sent = SendWithFd(respfd, ios, numIO, openedFd);
--- a/security/sandbox/linux/broker/SandboxBrokerCommon.h
+++ b/security/sandbox/linux/broker/SandboxBrokerCommon.h
@@ -26,16 +26,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_MKDIR,
   };
 
   struct Request {
     Operation mOp;
     // For open, flags; for access, "mode"; for stat, O_NOFOLLOW for lstat.
     int mFlags;
     // The rest of the packet is the pathname.
     // SCM_RIGHTS for response socket attached.
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -61,16 +61,19 @@ protected:
     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);
+  }
 
   virtual void SetUp() {
     ipc::FileDescriptor fd;
 
     mServer = SandboxBroker::Create(GetPolicy(), getpid(), fd);
     ASSERT_NE(mServer, nullptr);
     ASSERT_TRUE(fd.IsValid());
     auto rawFD = fd.ClonePlatformHandle();
@@ -217,16 +220,17 @@ TEST_F(SandboxBrokerTest, LStat)
 
   EXPECT_EQ(0, LStat("/proc/self", &brokeredStat));
   EXPECT_TRUE(S_ISLNK(brokeredStat.st_mode));
 }
 
 TEST_F(SandboxBrokerTest, Chmod)
 {
   unlink("/tmp/blublu");
+  rmdir("/tmp/blublu");
   int fd = Open("/tmp/blublu", O_WRONLY | O_CREAT);
   ASSERT_GE(fd, 0) << "Opening /tmp/blublu for writing failed.";
   close(fd);
   // Set read only. SandboxBroker enforces 0600 mode flags.
   ASSERT_EQ(0, Chmod("/tmp/blublu", S_IRUSR));
   // SandboxBroker doesn't use real access(), it just checks against
   // the policy. So it can't see the change in permisions here.
   // This won't work:
@@ -239,28 +243,44 @@ TEST_F(SandboxBrokerTest, Chmod)
   EXPECT_EQ(0, stat("/tmp/blublu", &realStat));
   EXPECT_EQ((mode_t)(S_IRUSR | S_IWUSR), realStat.st_mode & 0777);
   EXPECT_EQ(0, unlink("/tmp/blublu"));
 }
 
 TEST_F(SandboxBrokerTest, Link)
 {
   unlink("/tmp/blublu");
+  rmdir("/tmp/blublu");
   unlink("/tmp/blublublu");
   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"));
 }
 
+TEST_F(SandboxBrokerTest, Mkdir)
+{
+  unlink("/tmp/blublu");
+  rmdir("/tmp/blublu");
+  unlink("/tmp/nope");
+  rmdir("/tmp/nope");
+  ASSERT_EQ(mkdir("/tmp/blublu", 0600), 0)
+    << "Creating dir /tmp/blublu failed.";
+  EXPECT_EQ(0, Access("/tmp/blublu", F_OK));
+  // Not whitelisted target path
+  EXPECT_EQ(-EACCES, Mkdir("/tmp/nope", 0600))
+    << "Creating dir without MAY_CREATE succeed.";
+  EXPECT_EQ(0, rmdir("/tmp/blublu"));
+}
+
 TEST_F(SandboxBrokerTest, MultiThreadOpen) {
   RunOnManyThreads<SandboxBrokerTest,
                    &SandboxBrokerTest::MultiThreadOpenWorker>();
 }
 void SandboxBrokerTest::MultiThreadOpenWorker() {
   static const int kNumLoops = 10000;
 
   for (int i = 1; i <= kNumLoops; ++i) {