Posix: fix named SHM mappings permissions.

base/memory/shared_memory_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/shared_memory.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/safe_strerror_posix.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
@@ skipping 121 matching lines @@
     // private), and prevents the need for cleanup (once the last fd is closed,
     // it is truly freed).
     if (fp) {
       if (unlink(path.value().c_str()))
         PLOG(WARNING) << "unlink";
     }
   } else {
     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.
     struct stat stat;
     if (fstat(fileno(fp), &stat) != 0) {
       file_util::CloseFile(fp);
       return false;
     }
     const size_t current_size = stat.st_size;
     if (current_size != options.size) {
@@ skipping 211 matching lines @@
   new_handle->fd = new_fd;
   new_handle->auto_close = true;
 
   if (close_self)
     Close();
 
   return true;
 }
 
 }  // namespace base