base: Create file mappings with reduced access control permissions.

base/memory/shared_memory_win.cc

Index: base/memory/shared_memory_win.cc
diff --git a/base/memory/shared_memory_win.cc b/base/memory/shared_memory_win.cc
index 791db3f5b38e17ca74543d7422ac1de49181677e..f105418976a3f1f90f353d469973689d05615b8a 100644
--- a/base/memory/shared_memory_win.cc
+++ b/base/memory/shared_memory_win.cc
@@ -62,6 +62,40 @@ bool IsSectionSafeToMap(HANDLE handle) {
   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 {
@@ -136,8 +170,11 @@ SharedMemoryHandle SharedMemory::DuplicateHandle(
   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();
 }
 
@@ -166,7 +203,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   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))
@@ -184,9 +221,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
                          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;