win: Lower integrity level of connection pipe

util/win/exception_handler_server.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 #include "util/win/exception_handler_server.h"
 
+#include <aclapi.h>
+#include <sddl.h>
 #include <string.h>
 
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "minidump/minidump_file_writer.h"
 #include "snapshot/crashpad_info_client_options.h"
 #include "snapshot/win/process_snapshot_win.h"
 #include "util/file/file_writer.h"
 #include "util/misc/tri_state.h"
 #include "util/misc/uuid.h"
 #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 {
 
 namespace {
 
 // We create two pipe instances, so that there's one listening while the
 // PipeServiceProc is processing a registration.
 const size_t kPipeInstances = 2;
 
 // Wraps CreateNamedPipe() to create a single named pipe instance.
 //
 // 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() {
   static const auto get_named_pipe_client_process_id =
       GET_FUNCTION(L"kernel32.dll", ::GetNamedPipeClientProcessId);
   return get_named_pipe_client_process_id;
 }
 
 HANDLE DuplicateEvent(HANDLE process, HANDLE event) {
   HANDLE handle;
@@ skipping 213 matching lines @@
   int tries = 5;
   std::string pipe_name_base =
       base::StringPrintf("\\\\.\\pipe\\crashpad_%d_", GetCurrentProcessId());
   std::wstring pipe_name;
   do {
     pipe_name = base::UTF8ToUTF16(pipe_name_base);
     for (int index = 0; index < 16; ++index) {
       pipe_name.append(1, static_cast<wchar_t>(base::RandInt('A', 'Z')));
     }
 
     first_pipe_instance_.reset(CreateNamedPipeInstance(pipe_name, true));

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

The logging needs to be rationalized a bit. Now you have
CreateNamedPipeInstance() always logging on failure, where previously, it never
did. Since this is retried in a loop and does its own logging only after a few
retries, having CreateNamedPipeInstance() log on its own on each iteration is
undesirable.

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

(no lowering function now)

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

I think we're back to previous logging behaviour now, with the addition that the
lowering function will log on failure.

 
     // CreateNamedPipe() is documented as setting the error to
     // ERROR_ACCESS_DENIED if FILE_FLAG_FIRST_PIPE_INSTANCE is specified and the
     // pipe name is already in use. However it may set the error to other codes
     // such as ERROR_PIPE_BUSY (if the pipe already exists and has reached its
     // maximum instance count) or ERROR_INVALID_PARAMETER (if the pipe already
     // exists and its attributes differ from those specified to
     // CreateNamedPipe()). Some of these errors may be ambiguous: for example,
     // ERROR_INVALID_PARAMETER may also occur if CreateNamedPipe() is called
     // incorrectly even in the absence of an existing pipe by the same name.
     //
     // Rather than chasing down all of the possible errors that might indicate
     // that a pipe name is already in use, retry up to a few times on any error.
   } while (!first_pipe_instance_.is_valid() && --tries);
 
   PCHECK(first_pipe_instance_.is_valid()) << "CreateNamedPipe";
 
   SetPipeName(pipe_name);
   return pipe_name;
 }
 
 void ExceptionHandlerServer::Run(Delegate* delegate) {
   uint64_t shutdown_token = base::RandUint64();
   ScopedKernelHANDLE thread_handles[kPipeInstances];
   for (int i = 0; i < arraysize(thread_handles); ++i) {
     HANDLE pipe;
     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.

     // Ownership of this object (and the pipe instance) is given to the new
     // thread. We close the thread handles at the end of the scope. They clean
     // up the context object and the pipe instance on termination.
     internal::PipeServiceContext* context =
         new internal::PipeServiceContext(port_.get(),
                                          pipe,
                                          delegate,
                                          &clients_lock_,
                                          &clients_,
                                          shutdown_token);
@@ skipping 224 matching lines @@
 void __stdcall ExceptionHandlerServer::OnProcessEnd(void* ctx, BOOLEAN) {
   // This function is executed on the thread pool.
   internal::ClientData* client = reinterpret_cast<internal::ClientData*>(ctx);
   base::AutoLock lock(*client->lock());
 
   // Post back to the main thread to have it delete this client record.
   PostQueuedCompletionStatus(client->port(), 0, ULONG_PTR(client), nullptr);
 }
 
 }  // namespace crashpad