Posix: fix named SHM mappings permissions.

base/memory/shared_memory_posix.cc

Index: base/memory/shared_memory_posix.cc
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index 5d580d08c38c0294485e69a28082094fdcbd211d..4a4fb09b6eaa14c370256ae7a5aaa5313799d033 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -8,6 +8,7 @@
 #include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 #include "base/file_util.h"
@@ -149,12 +150,39 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     if (!FilePathForMemoryName(*options.name, &path))
       return false;
 
-    fp = file_util::OpenFile(path, "w+x");
-    if (fp == NULL && options.open_existing) {
-      // "w+" will truncate if it already exists.
-      fp = file_util::OpenFile(path, "a+");
+    // Make sure that the file is opened without any permission
+    // to other users on the system.
+    const mode_t kOwnerOnly = S_IRUSR | S_IWUSR;
+    // First, try to create the file.

[Mark Mentovai, 2013/07/02 19:52:56]

Blank line before this.

[jln (very slow on Chromium), 2013/07/02 21:20:02]

Done.

+    int fd = HANDLE_EINTR(
+        open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, kOwnerOnly));
+    if (fd == -1 && options.open_existing) {
+      // If this doesn't work, try and open an existing file in append mode.
+      // Opening an existing file in a world writable directory has two main
+      // security implications:
+      // - Attackers could plant a file under their control, so ownership of
+      //   the file is checked below.
+      // - Attackers could plant a symbolic link so that an unexpected file
+      //   is opened, so O_NOFOLLOW is passed to open().
+      // O_NOFOLLOW makes sure that the latter doesn't happen.
+      // Checking the former happens below.
+      fd = HANDLE_EINTR(
+          open(path.value().c_str(), O_RDWR | O_APPEND | O_NOFOLLOW));
+      struct stat sb;
+      // Check that the current user owns the file.

[Mark Mentovai, 2013/07/02 19:52:56]

And this too. When you have comments interspersed with code like this, you’re
breaking things up into conceptual blocks, and the whitespace in between really
helps a reader jump to the comment, see what’s going on, and find the breaks
between those blocks.

But I’d actually declare sb after the comment, since sb’s there for the fstat,
and the comment applies to both of them.

[jln (very slow on Chromium), 2013/07/02 21:20:02]

Done.

+      if (fd >= 0 &&
+          (fstat(fd, &sb) != 0 || sb.st_uid != getuid())) {
+        HANDLE_EINTR(close(fd));
+        return false;
+      }
+      // An existing file was opened, so its size should not be fixed.
       fix_size = false;
     }
+    fp = NULL;
+    if (fd >= 0) {
+      // "a+" is always appropriate: if it's a new file, a+ is similar to w+.
+      fp = fdopen(fd, "a+");
+    }
   }
   if (fp && fix_size) {
     // Get current size.