base: Create file mappings with reduced access control permissions.

base/memory/shared_memory_win.cc

 // Copyright (c) 2011 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 <aclapi.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 44 matching lines @@
   // The handle must have SECTION_QUERY access for this to succeed.
   SECTION_BASIC_INFORMATION basic_information = {};
   ULONG status =
       nt_query_section_func(handle, SectionBasicInformation, &basic_information,
                             sizeof(basic_information), nullptr);
   if (status)
     return false;
   return (basic_information.Attributes & SEC_IMAGE) != SEC_IMAGE;
 }
 
+// Returns a HANDLE on success and |nullptr| on failure.
+// This function is similar to CreateFileMapping, but removes the permissions
+// WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE.
+//
+// A newly created file mapping has two sets of permissions. It has access
+// control permissions (WRITE_DAC, WRITE_OWNER, READ_CONTROL, and DELETE) and
+// file permissions (FILE_MAP_READ, FILE_MAP_WRITE, etc.). ::DuplicateHandle()
+// with the parameter DUPLICATE_SAME_ACCESS copies both sets of permissions.
+//
+// The Chrome sandbox prevents HANDLEs with the WRITE_DAC permission from being
+// duplicated into unprivileged processes. But the only way to copy file
+// permissions is with the parameter DUPLICATE_SAME_ACCESS. This means that
+// there is no way for a privileged process to duplicate a file mapping into an
+// unprivileged process while maintaining the previous file permissions.
+//
+// By removing all access control permissions of a file mapping immediately
+// after creation, ::DuplicateHandle() effectively only copies the file
+// permissions.
+HANDLE CreateFileMappingWithReducedPermissions(SECURITY_ATTRIBUTES* sa,
+                                               size_t rounded_size,
+                                               LPCWSTR name) {
+  HANDLE h = CreateFileMapping(INVALID_HANDLE_VALUE, sa, PAGE_READWRITE, 0,
+                               static_cast<DWORD>(rounded_size), name);
+  if (!h)
+    return nullptr;
+
+  HANDLE h2;
+  BOOL success = ::DuplicateHandle(
+      GetCurrentProcess(), h, GetCurrentProcess(), &h2,
+      FILE_MAP_READ | FILE_MAP_WRITE | SECTION_QUERY, FALSE, 0);
+  ::CloseHandle(h);

[Mark Mentovai, 2016/02/17 19:03:14]

BOOL rv = ::CloseHandle(h);
DCHECK(rv);

?

[erikchen, 2016/02/17 19:22:26]

Done.

+  return success ? h2 : nullptr;
+}
+
 }  // namespace.
 
 namespace base {
 
 SharedMemoryCreateOptions::SharedMemoryCreateOptions()
     : name_deprecated(nullptr),
       open_existing_deprecated(false),
       size(0),
       executable(false),
       share_read_only(false) {}
@@ skipping 54 matching lines @@
 
 // static
 SharedMemoryHandle SharedMemory::DuplicateHandle(
     const SharedMemoryHandle& handle) {
   DCHECK(handle.BelongsToCurrentProcess());
   HANDLE duped_handle;
   ProcessHandle process = GetCurrentProcess();
   BOOL success =
       ::DuplicateHandle(process, handle.GetHandle(), process, &duped_handle, 0,
                         FALSE, DUPLICATE_SAME_ACCESS);
-  if (success)
-    return SharedMemoryHandle(duped_handle, GetCurrentProcId());
+  if (success) {
+    base::SharedMemoryHandle handle(duped_handle, GetCurrentProcId());
+    handle.SetOwnershipPassesToIPC(true);
+    return handle;
+  }
   return SharedMemoryHandle();
 }
 
 bool SharedMemory::CreateAndMapAnonymous(size_t size) {
   return CreateAnonymous(size) && Map(size);
 }
 
 bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   // TODO(bsy,sehr): crbug.com/210609 NaCl forces us to round up 64k here,
   // wasting 32k per mapping on average.
   static const size_t kSectionMask = 65536 - 1;
   DCHECK(!options.executable);
   DCHECK(!mapped_file_);
   if (options.size == 0)
     return false;
 
   // Check maximum accounting for overflow.
   if (options.size >
       static_cast<size_t>(std::numeric_limits<int>::max()) - kSectionMask)
     return false;
 
   size_t rounded_size = (options.size + kSectionMask) & ~kSectionMask;
   name_ = options.name_deprecated ?
       ASCIIToUTF16(*options.name_deprecated) : L"";
   SECURITY_ATTRIBUTES sa = { sizeof(sa), NULL, FALSE };
   SECURITY_DESCRIPTOR sd;
   ACL dacl;
 
-  if (options.share_read_only && name_.empty()) {
+  if (name_.empty()) {

[penny, 2016/02/17 00:32:24]

So, we're no longer looking at whether |share_read_only| is true or false in the
SharedMemoryCreateOptions passed in.  (We want to always name the mapping now,
regardless.)

1) It appears the caller has always been expected to set the access controls to
read-only when duplicating the handle later.  In which case, |share_read_only|
flag was only ever used to tell us to make sure that the caller COULD adjust the
access controls later.  So it no longer has a use.  I can't immediately see
anywhere else that this is looked at.

If it's not referenced in other places, we could potentially remove that flag
from source now.

2) SharedMemory::DuplicateHandle function does not seem to adjust anything for
read-only either.  Should we be enforcing read-only sharing somewhere in this
file... or do callers need to do their own ::DuplicateHandle() system call with
the appropriate accesses?  I wonder why SharedMemory::DuplicateHandle doesn't
support different access permissions for the new handle.


jschuh@, thoughts?

[erikchen, 2016/02/17 00:43:22]

(1) is a correct assessment. I would prefer to remove |share_read_only| in a
follow-up CL. And yes, the current system is very fragile, since a caller could
forget to set up the handle correctly, and then attempt to duplicate read_only,
and *think* that it had succeeded in making a read-only handle.

(2) There are too many ways of duplicating SharedMemoryHandles on Windows. This
is something I am working on fixing.

(a) In this file, the method SharedMemory::ShareToProcessCommon() will duplicate
a handle with read_only permissions, depending on the parameters it has been
called with. 

(b) Windows sandbox code uses it's own logic where it calls ::DuplicateHandle()
and manually sets the new permissions:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san...

(c) There's also SharedMemory::DuplicateHandle, as you noticed, as well as other
callers of ::DuplicateHandle (AttachmentBroker comes to mind, but there are
probably others). 

     // Add an empty DACL to enforce anonymous read-only sections.
     sa.lpSecurityDescriptor = &sd;
     if (!InitializeAcl(&dacl, sizeof(dacl), ACL_REVISION))
       return false;
     if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION))
       return false;
     if (!SetSecurityDescriptorDacl(&sd, TRUE, &dacl, FALSE))
       return false;
 
     // Windows ignores DACLs on certain unnamed objects (like shared sections).
     // So, we generate a random name when we need to enforce read-only.
     uint64_t rand_values[4];
     RandBytes(&rand_values, sizeof(rand_values));
     name_ = StringPrintf(L"CrSharedMem_%016llx%016llx%016llx%016llx",
                          rand_values[0], rand_values[1],
                          rand_values[2], rand_values[3]);
   }
-  mapped_file_ = CreateFileMapping(INVALID_HANDLE_VALUE, &sa,
-      PAGE_READWRITE, 0, static_cast<DWORD>(rounded_size),
-      name_.empty() ? nullptr : name_.c_str());
+  mapped_file_ = CreateFileMappingWithReducedPermissions(
+      &sa, rounded_size, name_.empty() ? nullptr : name_.c_str());
   if (!mapped_file_)
     return false;
 
   requested_size_ = options.size;
 
   // Check if the shared memory pre-exists.
   if (GetLastError() == ERROR_ALREADY_EXISTS) {
     // If the file already existed, set requested_size_ to 0 to show that
     // we don't know the size.
     requested_size_ = 0;
@@ skipping 102 matching lines @@
     ::CloseHandle(mapped_file_);
     mapped_file_ = NULL;
   }
 }
 
 SharedMemoryHandle SharedMemory::handle() const {
   return SharedMemoryHandle(mapped_file_, base::GetCurrentProcId());
 }
 
 }  // namespace base