Implement SharedMemory::ShareReadOnlyToProcess().

This avoids potential security holes where the renderer could be exploited and
then write into space shared by other renderers or even the browser.

I've done this on Posix by opening both a read/write and read-only file descriptor to the same file. Then ShareReadOnlyToProcess dup()s the read-only descriptor instead of the read/write one. It's an error to try to ShareReadOnly from a SharedMemory that was created from a single SharedMemoryHandle.

The test checks that operations strictly through the file handle can't get
write access to the memory. On Linux there's still a hole through /dev/fd
in the filesystem, but jln@ assures me that the sandbox prevents the
filesystem-based attack. We should eventually write an explicit test for this.

Android needs http://crbug.com/320865 figured out.

BUG=302724,320865

Review URL: https://codereview.chromium.org/27265002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236347 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 229 insertions, 38 deletions

diff --git a/base/memory/shared_memory.h b/base/memory/shared_memory.h
index 23f6973..9007aed 100644
--- a/base/memory/shared_memory.h
+++ b/base/memory/shared_memory.h
@@ -21,6 +21,7 @@
 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
+#include "base/file_util.h"
 #endif
 
 namespace base {
@@ -80,6 +81,11 @@ class BASE_EXPORT SharedMemory {
 
   // Create a new SharedMemory object from an existing, open
   // shared memory file.
+  //
+  // WARNING: This does not reduce the OS-level permissions on the handle; it
+  // only affects how the SharedMemory will be mmapped.  Use
+  // ShareReadOnlyToProcess to drop permissions.  TODO(jln,jyasskin): DCHECK
+  // that |read_only| matches the permissions of the handle.
   SharedMemory(SharedMemoryHandle handle, bool read_only);
 
   // Create a new SharedMemory object from an existing, open
@@ -189,15 +195,41 @@ class BASE_EXPORT SharedMemory {
   // It is safe to call Close repeatedly.
   void Close();
 
+  // Shares the shared memory to another process.  Attempts to create a
+  // platform-specific new_handle which can be used in a remote process to read
+  // the shared memory file.  new_handle is an output parameter to receive the
+  // handle for use in the remote process.
+  //
+  // |*this| must have been initialized using one of the Create*() or Open()
+  // methods.  If it was constructed from a SharedMemoryHandle, this call will
+  // CHECK-fail.
+  //
+  // Returns true on success, false otherwise.
+  bool ShareReadOnlyToProcess(ProcessHandle process,
+                              SharedMemoryHandle* new_handle) {
+    return ShareToProcessCommon(process, new_handle, false, SHARE_READONLY);
+  }
+
+  // Logically equivalent to:
+  //   bool ok = ShareReadOnlyToProcess(process, new_handle);
+  //   Close();
+  //   return ok;
+  // Note that the memory is unmapped by calling this method, regardless of the
+  // return value.
+  bool GiveReadOnlyToProcess(ProcessHandle process,
+                             SharedMemoryHandle* new_handle) {
+    return ShareToProcessCommon(process, new_handle, true, SHARE_READONLY);
+  }
+
   // Shares the shared memory to another process.  Attempts
   // to create a platform-specific new_handle which can be
   // used in a remote process to access the shared memory
-  // file.  new_handle is an ouput parameter to receive
+  // file.  new_handle is an output parameter to receive
   // the handle for use in the remote process.
   // Returns true on success, false otherwise.
   bool ShareToProcess(ProcessHandle process,
                       SharedMemoryHandle* new_handle) {
-    return ShareToProcessCommon(process, new_handle, false);
+    return ShareToProcessCommon(process, new_handle, false, SHARE_CURRENT_MODE);
   }
 
   // Logically equivalent to:
@@ -208,7 +240,7 @@ class BASE_EXPORT SharedMemory {
   // return value.
   bool GiveToProcess(ProcessHandle process,
                      SharedMemoryHandle* new_handle) {
-    return ShareToProcessCommon(process, new_handle, true);
+    return ShareToProcessCommon(process, new_handle, true, SHARE_CURRENT_MODE);
   }
 
   // Locks the shared memory.
@@ -232,19 +264,25 @@ class BASE_EXPORT SharedMemory {
 
  private:
 #if defined(OS_POSIX) && !defined(OS_NACL)
-  bool PrepareMapFile(FILE *fp);
+  bool PrepareMapFile(file_util::ScopedFILE fp, file_util::ScopedFD readonly);
   bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
   void LockOrUnlockCommon(int function);
 #endif
+  enum ShareMode {
+    SHARE_READONLY,
+    SHARE_CURRENT_MODE,
+  };
   bool ShareToProcessCommon(ProcessHandle process,
                             SharedMemoryHandle* new_handle,
-                            bool close_self);
+                            bool close_self,
+                            ShareMode);
 
 #if defined(OS_WIN)
   std::wstring       name_;
   HANDLE             mapped_file_;
 #elif defined(OS_POSIX)
   int                mapped_file_;
+  int                readonly_mapped_file_;
   ino_t              inode_;
 #endif
   size_t             mapped_size_;
diff --git a/base/memory/shared_memory_android.cc b/base/memory/shared_memory_android.cc
index 3ca3c8f..25a6573 100644
--- a/base/memory/shared_memory_android.cc
+++ b/base/memory/shared_memory_android.cc
@@ -37,6 +37,15 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     DLOG(ERROR) << "Error " << err << " when setting protection of ashmem";
     return false;
   }
+
+  // Android doesn't appear to have a way to drop write access on an ashmem
+  // segment for a single descriptor.  http://crbug.com/320865
+  readonly_mapped_file_ = dup(mapped_file_);
+  if (-1 == readonly_mapped_file_) {
+    DPLOG(ERROR) << "dup() failed";
+    return false;
+  }
+
   requested_size_ = options.size;
 
   return true;
diff --git a/base/memory/shared_memory_nacl.cc b/base/memory/shared_memory_nacl.cc
index bc2a98d..93c1002 100644
--- a/base/memory/shared_memory_nacl.cc
+++ b/base/memory/shared_memory_nacl.cc
@@ -140,7 +140,13 @@ void SharedMemory::Unlock() {
 
 bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
                                         SharedMemoryHandle *new_handle,
-                                        bool close_self) {
+                                        bool close_self,
+                                        ShareMode share_mode) {
+  if (share_mode == SHARE_READONLY) {
+    // Untrusted code can't create descriptors or handles, which is needed to
+    // drop permissions.
+    return false;
+  }
   const int new_fd = dup(mapped_file_);
   if (new_fd < 0) {
     DPLOG(ERROR) << "dup() failed.";
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index efb0caf..bb3c280 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -30,6 +30,9 @@
 #include "third_party/ashmem/ashmem.h"
 #endif
 
+using file_util::ScopedFD;
+using file_util::ScopedFILE;
+
 namespace base {
 
 namespace {
@@ -40,6 +43,7 @@ LazyInstance<Lock>::Leaky g_thread_lock_ = LAZY_INSTANCE_INITIALIZER;
 
 SharedMemory::SharedMemory()
     : mapped_file_(-1),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -49,6 +53,7 @@ SharedMemory::SharedMemory()
 
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
     : mapped_file_(handle.fd),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -65,6 +70,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
                            ProcessHandle process)
     : mapped_file_(handle.fd),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -92,7 +98,7 @@ SharedMemoryHandle SharedMemory::NULLHandle() {
 // static
 void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
   DCHECK_GE(handle.fd, 0);
-  if (HANDLE_EINTR(close(handle.fd)) < 0)
+  if (close(handle.fd) < 0)
     DPLOG(ERROR) << "close";
 }
 
@@ -124,8 +130,10 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   // and be deleted before they ever make it out to disk.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
 
-  FILE *fp;
+  ScopedFILE fp;
   bool fix_size = true;
+  int readonly_fd_storage = -1;
+  ScopedFD readonly_fd(&readonly_fd_storage);
 
   FilePath path;
   if (options.name == NULL || options.name->empty()) {
@@ -133,12 +141,19 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     DCHECK(!options.open_existing);
     // Q: Why not use the shm_open() etc. APIs?
     // A: Because they're limited to 4mb on OS X.  FFFFFFFUUUUUUUUUUU
-    fp = file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable);
+    fp.reset(
+        file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable));
 
-    // Deleting the file prevents anyone else from mapping it in (making it
-    // private), and prevents the need for cleanup (once the last fd is closed,
-    // it is truly freed).
     if (fp) {
+      // Also open as readonly so that we can ShareReadOnlyToProcess.
+      *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+      if (*readonly_fd < 0) {
+        DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
+        fp.reset();
+      }
+      // Deleting the file prevents anyone else from mapping it in (making it
+      // private), and prevents the need for cleanup (once the last fd is
+      // closed, it is truly freed).
       if (unlink(path.value().c_str()))
         PLOG(WARNING) << "unlink";
     }
@@ -175,32 +190,35 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
            sb.st_uid != effective_uid)) {
         LOG(ERROR) <<
             "Invalid owner when opening existing shared memory file.";
-        HANDLE_EINTR(close(fd));
+        close(fd);
         return false;
       }
 
       // An existing file was opened, so its size should not be fixed.
       fix_size = false;
     }
-    fp = NULL;
+
+    // Also open as readonly so that we can ShareReadOnlyToProcess.
+    *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+    if (*readonly_fd < 0) {
+      DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
+      close(fd);
+      fd = -1;
+    }
     if (fd >= 0) {
       // "a+" is always appropriate: if it's a new file, a+ is similar to w+.
-      fp = fdopen(fd, "a+");
+      fp.reset(fdopen(fd, "a+"));
     }
   }
   if (fp && fix_size) {
     // Get current size.
     struct stat stat;
-    if (fstat(fileno(fp), &stat) != 0) {
-      file_util::CloseFile(fp);
+    if (fstat(fileno(fp.get()), &stat) != 0)
       return false;
-    }
     const size_t current_size = stat.st_size;
     if (current_size != options.size) {
-      if (HANDLE_EINTR(ftruncate(fileno(fp), options.size)) != 0) {
-        file_util::CloseFile(fp);
+      if (HANDLE_EINTR(ftruncate(fileno(fp.get()), options.size)) != 0)
         return false;
-      }
     }
     requested_size_ = options.size;
   }
@@ -221,7 +239,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     return false;
   }
 
-  return PrepareMapFile(fp);
+  return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
 }
 
 // Our current implementation of shmem is with mmap()ing of files.
@@ -247,8 +265,14 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
   read_only_ = read_only;
 
   const char *mode = read_only ? "r" : "r+";
-  FILE *fp = file_util::OpenFile(path, mode);
-  return PrepareMapFile(fp);
+  ScopedFILE fp(file_util::OpenFile(path, mode));
+  int readonly_fd_storage = -1;
+  ScopedFD readonly_fd(&readonly_fd_storage);
+  *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+  if (*readonly_fd < 0) {
+    DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
+  }
+  return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
 }
 
 #endif  // !defined(OS_ANDROID)
@@ -305,10 +329,15 @@ void SharedMemory::Close() {
   Unmap();
 
   if (mapped_file_ > 0) {
-    if (HANDLE_EINTR(close(mapped_file_)) < 0)
+    if (close(mapped_file_) < 0)
       PLOG(ERROR) << "close";
     mapped_file_ = -1;
   }
+  if (readonly_mapped_file_ > 0) {
+    if (close(readonly_mapped_file_) < 0)
+      PLOG(ERROR) << "close";
+    readonly_mapped_file_ = -1;
+  }
 }
 
 void SharedMemory::Lock() {
@@ -322,18 +351,28 @@ void SharedMemory::Unlock() {
 }
 
 #if !defined(OS_ANDROID)
-bool SharedMemory::PrepareMapFile(FILE *fp) {
+bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
   DCHECK_EQ(-1, mapped_file_);
-  if (fp == NULL) return false;
+  DCHECK_EQ(-1, readonly_mapped_file_);
+  if (fp == NULL || *readonly_fd < 0) return false;
 
   // This function theoretically can block on the disk, but realistically
   // the temporary files we create will just go into the buffer cache
   // and be deleted before they ever make it out to disk.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
 
-  file_util::ScopedFILE file_closer(fp);
+  struct stat st = {};
+  struct stat readonly_st = {};
+  if (fstat(fileno(fp.get()), &st))
+    NOTREACHED();
+  if (fstat(*readonly_fd, &readonly_st))
+    NOTREACHED();
+  if (st.st_dev != readonly_st.st_dev || st.st_ino != readonly_st.st_ino) {
+    LOG(ERROR) << "writable and read-only inodes don't match; bailing";
+    return false;
+  }
 
-  mapped_file_ = dup(fileno(fp));
+  mapped_file_ = dup(fileno(fp.get()));
   if (mapped_file_ == -1) {
     if (errno == EMFILE) {
       LOG(WARNING) << "Shared memory creation failed; out of file descriptors";
@@ -342,11 +381,8 @@ bool SharedMemory::PrepareMapFile(FILE *fp) {
       NOTREACHED() << "Call to dup failed, errno=" << errno;
     }
   }
-
-  struct stat st;
-  if (fstat(mapped_file_, &st))
-    NOTREACHED();
   inode_ = st.st_ino;
+  readonly_mapped_file_ = *readonly_fd.release();
 
   return true;
 }
@@ -399,9 +435,23 @@ void SharedMemory::LockOrUnlockCommon(int function) {
 }
 
 bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
-                                        SharedMemoryHandle *new_handle,
-                                        bool close_self) {
-  const int new_fd = dup(mapped_file_);
+                                        SharedMemoryHandle* new_handle,
+                                        bool close_self,
+                                        ShareMode share_mode) {
+  int handle_to_dup = -1;
+  switch(share_mode) {
+    case SHARE_CURRENT_MODE:
+      handle_to_dup = mapped_file_;
+      break;
+    case SHARE_READONLY:
+      // We could imagine re-opening the file from /dev/fd, but that can't make
+      // it readonly on Mac: https://codereview.chromium.org/27265002/#msg10
+      CHECK(readonly_mapped_file_ >= 0);
+      handle_to_dup = readonly_mapped_file_;
+      break;
+  }
+
+  const int new_fd = dup(handle_to_dup);
   if (new_fd < 0) {
     DPLOG(ERROR) << "dup() failed.";
     return false;
diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
index a8dea8d..2b3ad7a 100644
--- a/base/memory/shared_memory_unittest.cc
+++ b/base/memory/shared_memory_unittest.cc
@@ -20,12 +20,18 @@
 #endif
 
 #if defined(OS_POSIX)
+#include <errno.h>
+#include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 #endif
 
+#if defined(OS_WIN)
+#include "base/win/scoped_handle.h"
+#endif
+
 static const int kNumThreads = 5;
 static const int kNumTasks = 5;
 
@@ -361,6 +367,79 @@ TEST(SharedMemoryTest, AnonymousPrivate) {
   }
 }
 
+TEST(SharedMemoryTest, ShareReadOnly) {
+  StringPiece contents = "Hello World";
+
+  SharedMemory writable_shmem;
+  ASSERT_TRUE(writable_shmem.CreateAndMapAnonymous(contents.size()));
+  memcpy(writable_shmem.memory(), contents.data(), contents.size());
+  EXPECT_TRUE(writable_shmem.Unmap());
+
+  SharedMemoryHandle readonly_handle;
+  ASSERT_TRUE(writable_shmem.ShareReadOnlyToProcess(GetCurrentProcessHandle(),
+                                                    &readonly_handle));
+  SharedMemory readonly_shmem(readonly_handle, /*readonly=*/true);
+
+  ASSERT_TRUE(readonly_shmem.Map(contents.size()));
+  EXPECT_EQ(contents,
+            StringPiece(static_cast<const char*>(readonly_shmem.memory()),
+                        contents.size()));
+  EXPECT_TRUE(readonly_shmem.Unmap());
+
+  // Make sure the writable instance is still writable.
+  ASSERT_TRUE(writable_shmem.Map(contents.size()));
+  StringPiece new_contents = "Goodbye";
+  memcpy(writable_shmem.memory(), new_contents.data(), new_contents.size());
+  EXPECT_EQ(new_contents,
+            StringPiece(static_cast<const char*>(writable_shmem.memory()),
+                        new_contents.size()));
+
+  // We'd like to check that if we send the read-only segment to another
+  // process, then that other process can't reopen it read/write.  (Since that
+  // would be a security hole.)  Setting up multiple processes is hard in a
+  // unittest, so this test checks that the *current* process can't reopen the
+  // segment read/write.  I think the test here is stronger than we actually
+  // care about, but there's a remote possibility that sending a file over a
+  // pipe would transform it into read/write.
+  SharedMemoryHandle handle = readonly_shmem.handle();
+
+#if defined(OS_ANDROID)
+  // The "read-only" handle is still writable on Android:
+  // http://crbug.com/320865
+  (void)handle;
+#elif defined(OS_POSIX)
+  EXPECT_EQ(O_RDONLY, fcntl(handle.fd, F_GETFL) & O_ACCMODE)
+      << "The descriptor itself should be read-only.";
+
+  errno = 0;
+  void* writable = mmap(
+      NULL, contents.size(), PROT_READ | PROT_WRITE, MAP_SHARED, handle.fd, 0);
+  int mmap_errno = errno;
+  EXPECT_EQ(MAP_FAILED, writable)
+      << "It shouldn't be possible to re-mmap the descriptor writable.";
+  EXPECT_EQ(EACCES, mmap_errno) << strerror(mmap_errno);
+  if (writable != MAP_FAILED)
+    EXPECT_EQ(0, munmap(writable, readonly_shmem.mapped_size()));
+
+#elif defined(OS_WIN)
+  EXPECT_EQ(NULL, MapViewOfFile(handle, FILE_MAP_WRITE, 0, 0, 0))
+      << "Shouldn't be able to map memory writable.";
+
+  base::win::ScopedHandle writable_handle;
+  EXPECT_EQ(0,
+            ::DuplicateHandle(GetCurrentProcess(),
+                              handle,
+                              GetCurrentProcess,
+                              writable_handle.Receive(),
+                              FILE_MAP_ALL_ACCESS,
+                              false,
+                              0))
+      << "Shouldn't be able to duplicate the handle into a writable one.";
+#else
+#error Unexpected platform; write a test that tries to make 'handle' writable.
+#endif  // defined(OS_POSIX) || defined(OS_WIN)
+}
+
 TEST(SharedMemoryTest, ShareToSelf) {
   StringPiece contents = "Hello World";
 
@@ -377,6 +456,14 @@ TEST(SharedMemoryTest, ShareToSelf) {
   EXPECT_EQ(
       contents,
       StringPiece(static_cast<const char*>(shared.memory()), contents.size()));
+
+  ASSERT_TRUE(shmem.ShareToProcess(GetCurrentProcessHandle(), &shared_handle));
+  SharedMemory readonly(shared_handle, /*readonly=*/true);
+
+  ASSERT_TRUE(readonly.Map(contents.size()));
+  EXPECT_EQ(contents,
+            StringPiece(static_cast<const char*>(readonly.memory()),
+                        contents.size()));
 }
 
 TEST(SharedMemoryTest, MapAt) {
diff --git a/base/memory/shared_memory_win.cc b/base/memory/shared_memory_win.cc
index d27ebc1..77741bc 100644
--- a/base/memory/shared_memory_win.cc
+++ b/base/memory/shared_memory_win.cc
@@ -189,13 +189,14 @@ bool SharedMemory::Unmap() {
 
 bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
                                         SharedMemoryHandle *new_handle,
-                                        bool close_self) {
+                                        bool close_self,
+                                        ShareMode share_mode) {
   *new_handle = 0;
   DWORD access = FILE_MAP_READ;
   DWORD options = 0;
   HANDLE mapped_file = mapped_file_;
   HANDLE result;
-  if (!read_only_)
+  if (share_mode == SHARE_CURRENT_MODE && !read_only_)
     access |= FILE_MAP_WRITE;
   if (close_self) {
     // DUPLICATE_CLOSE_SOURCE causes DuplicateHandle to close mapped_file.