win: Lower integrity level of connection pipe

util/win/exception_handler_server.cc

Index: util/win/exception_handler_server.cc
diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc
index ce5687d74b7ba6f853708f68acf5e4f25390f5b0..6753f494b52b9cfc44c96540f8fab1a91951515d 100644
--- a/util/win/exception_handler_server.cc
+++ b/util/win/exception_handler_server.cc
@@ -14,6 +14,8 @@
 
 #include "util/win/exception_handler_server.h"
 
+#include <aclapi.h>
+#include <sddl.h>
 #include <string.h>
 
 #include "base/logging.h"
@@ -30,6 +32,7 @@
 #include "util/win/get_function.h"
 #include "util/win/handle.h"
 #include "util/win/registration_protocol_win.h"
+#include "util/win/scoped_local_free.h"
 #include "util/win/xp_compat.h"
 
 namespace crashpad {
@@ -45,18 +48,74 @@ const size_t kPipeInstances = 2;
 // If first_instance is true, the named pipe instance will be created with
 // FILE_FLAG_FIRST_PIPE_INSTANCE. This ensures that the the pipe name is not
 // already in use when created.
+//
+// The integrity level of the pipe is lowered so that it can be connected to by
+// processes at any integrity level.
 HANDLE CreateNamedPipeInstance(const std::wstring& pipe_name,
                                bool first_instance) {
-  return CreateNamedPipe(pipe_name.c_str(),
-                         PIPE_ACCESS_DUPLEX |
-                             (first_instance ? FILE_FLAG_FIRST_PIPE_INSTANCE
-                                             : 0),
-                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
-                         kPipeInstances,
-                         512,
-                         512,
-                         0,
-                         nullptr);
+  ScopedFileHandle pipe(CreateNamedPipe(
+      pipe_name.c_str(),
+      PIPE_ACCESS_DUPLEX | (first_instance ? FILE_FLAG_FIRST_PIPE_INSTANCE : 0),
+      PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+      kPipeInstances,
+      512,
+      512,
+      0,
+      nullptr));
+  if (!pipe.is_valid()) {
+    PLOG(ERROR) << "CreateNamedPipe";
+    return INVALID_HANDLE_VALUE;
+  }
+
+  // We only need to set the integrity level on the first instance of the pipe.
+  if (!first_instance)
+    return pipe.release();
+
+  // Lower the integrity of the pipe so that it can be connected to from
+  // processes at any integrity level. This only applies to Vista and later.
+  const DWORD version = GetVersion();
+  const DWORD major_version = LOBYTE(LOWORD(version));
+  const bool is_pre_vista = major_version < 6;
+  if (is_pre_vista)
+    return pipe.release();
+
+  // Mandatory Label, no ACE flags, no ObjectType, integrity level untrusted.
+  const wchar_t kSddl[] = L"S:(ML;;;;;S-1-16-0)";
+
+  SECURITY_DESCRIPTOR* sec_desc = nullptr;
+
+  PACL* sacl = nullptr;

[Mark Mentovai, 2015/11/05 21:36:15]

ACL**?

[scottmg, 2015/11/05 22:29:59]

Oops.

Sadly, now that I try, um, compiling, ACL* is fine, but the definitions are dumb
for SECURITY_DESCRIPTOR, so it has to be PSECURITY_DESCRIPTOR.

typedef PVOID PSECURITY_DESCRIPTOR;

typedef struct _SECURITY_DESCRIPTOR {
   BYTE  Revision;
   BYTE  Sbz1;
   SECURITY_DESCRIPTOR_CONTROL Control;
   PSID Owner;
   PSID Group;
   PACL Sacl;
   PACL Dacl;

   } SECURITY_DESCRIPTOR, *PISECURITY_DESCRIPTOR;

And the prototype is 

BOOL
WINAPI
ConvertStringSecurityDescriptorToSecurityDescriptorW(
    _In_ LPCWSTR StringSecurityDescriptor,
    _In_ DWORD StringSDRevision,
    _Outptr_ PSECURITY_DESCRIPTOR * SecurityDescriptor,
    _Out_opt_ PULONG SecurityDescriptorSize
    );

9_9

+  if (!ConvertStringSecurityDescriptorToSecurityDescriptor(
+          kSddl, SDDL_REVISION, &sec_desc, nullptr)) {

[Mark Mentovai, 2015/11/05 21:36:15]

Be explicit about the format of the string you’re providing, SDDL_REVISION_1?

[scottmg, 2015/11/05 22:29:59]

Done.

+    PLOG(ERROR) << "ConvertStringSecurityDescriptorToSecurityDescriptor";
+    return INVALID_HANDLE_VALUE;
+  }
+
+  // Take ownership of the allocated SECURITY_DESCRIPTOR.
+  ScopedLocalFree scoped_sec_desc(sec_desc);
+
+  BOOL sacl_present = FALSE;
+  BOOL sacl_defaulted = FALSE;
+  if (!GetSecurityDescriptorSacl(
+          sec_desc, &sacl_present, &sacl, &sacl_defaulted)) {

[Mark Mentovai, 2015/11/05 21:36:15]

sacl points to something in sec_desc, so no need to free it separately, right?

[scottmg, 2015/11/05 22:29:59]

Yes, that's my understanding from the docs. Maybe Justin can confirm.

... and now removed.

+    PLOG(ERROR) << "GetSecurityDescriptorSacl";
+    return INVALID_HANDLE_VALUE;
+  }
+
+  DWORD error = SetSecurityInfo(pipe.get(),
+                                SE_KERNEL_OBJECT,
+                                LABEL_SECURITY_INFORMATION,
+                                nullptr,
+                                nullptr,
+                                nullptr,
+                                sacl);
+  if (error != ERROR_SUCCESS) {
+    LOG(ERROR) << "SetSecurityInfo: "
+               << logging::SystemErrorCodeToString(error);
+    return INVALID_HANDLE_VALUE;
+  }
+
+  return pipe.release();
 }
 
 decltype(GetNamedPipeClientProcessId)* GetNamedPipeClientProcessIdFunction() {
@@ -320,7 +379,7 @@ void ExceptionHandlerServer::Run(Delegate* delegate) {
     if (first_pipe_instance_.is_valid()) {
       pipe = first_pipe_instance_.release();
     } else {
-      pipe = CreateNamedPipeInstance(pipe_name_, false);
+      pipe = CreateNamedPipeInstance(pipe_name_, i == 0);
       PCHECK(pipe != INVALID_HANDLE_VALUE) << "CreateNamedPipe";
     }
 

[Mark Mentovai, 2015/11/05 21:39:11]

Maybe this works out better from that perspective if you set the integrity level
from here if i == 0.

[scottmg, 2015/11/05 22:29:59]

Yeah, that seems better. Done.

...

Hmm, it would seem that SetSecurityInfo always fails if
FILE_FLAG_FIRST_PIPE_INSTANCE isn't set. So I need to set that flag all the same
on the first trip through the loop.

...

After a bit more discussion, I switched to passing in a SECURITY_ATTRIBUTES to
CreateNamedPipe instead. This still has to be done only on the first instance.