Allow network service to query remote_security_key process for its session ID

remoting/host/security_key/remote_security_key_main.cc

 // Copyright 2016 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 "remoting/host/security_key/remote_security_key_main.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
 #include "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "remoting/host/host_exit_codes.h"
 #include "remoting/host/logging.h"
 #include "remoting/host/security_key/remote_security_key_ipc_client.h"
 #include "remoting/host/security_key/remote_security_key_message_handler.h"
 
+#if defined(OS_WIN)
+#include <aclapi.h>
+#include <windows.h>
+
+#include "base/win/scoped_handle.h"
+#endif  // defined(OS_WIN)
+
+#if defined(OS_WIN)
+namespace {

[Hzj_jie, 2016/06/23 22:35:58]

I believe we usually add a blank line after "namespace {", except for forwarding
declaration.

[joedow, 2016/06/23 23:07:18]

Done.

+bool AddAccessRightForWellKnownSid(WELL_KNOWN_SID_TYPE type, DWORD new_right) {

[Hzj_jie, 2016/06/23 22:35:58]

I saw this function has only been used with WinLocalServiceSid and
PROCESS_QUERY_LIMITED_INFORMATION combination. So do we really need the
parameters? A bool ApplyQueryLimitedInformationPermission() function should be
enough.

[joedow, 2016/06/23 23:07:17]

There are two reasons:
- When someone reads the call site it is obvious what is being applied (this is
true with your example but it doesn't state who the permission is being granted
to, ApplyQueryLimitedInformationPermissionForLocalServiceAccount() seemed too
long of a name :)

- I can imagine other scenarios where we would want to add a specific ACL for a
well-known SID.  By writing the function this way up front, it would be easy to
pull this code out into a separate header/cc and use it in multiple locations.

+  // Open a handle for the current process, read the current DACL, update it,
+  // and write it back.  This will add |new_right| to the current process.
+  base::win::ScopedHandle process_handle(OpenProcess(READ_CONTROL | WRITE_DAC,
+                                                     /*bInheritHandle=*/FALSE,
+                                                     GetCurrentProcessId()));
+  if (!process_handle.IsValid()) {
+    PLOG(ERROR) << "OpenProcess() failed!";
+    return false;
+  }
+
+  PSECURITY_DESCRIPTOR descriptor = nullptr;
+  // |old_dacl| is a pointer into the opaque |descriptor| struct, don't free it.
+  PACL old_dacl = nullptr;
+  PACL new_dacl = nullptr;

[Sergey Ulanov, 2016/06/23 22:28:41]

move this below, next to the SetEntriesInAcl() call.

[joedow, 2016/06/23 23:07:18]

Done.

+  if (GetSecurityInfo(process_handle.Get(),
+                      SE_KERNEL_OBJECT,
+                      DACL_SECURITY_INFORMATION,
+                      /*ppsidOwner=*/nullptr,
+                      /*ppsidGroup=*/nullptr,
+                      &old_dacl,
+                      /*ppSacl=*/nullptr,
+                      &descriptor) != ERROR_SUCCESS) {
+    PLOG(ERROR) << "GetSecurityInfo() failed!";
+    return false;
+  }
+
+  BYTE buffer[SECURITY_MAX_SID_SIZE] = {0};
+  DWORD buffer_size = SECURITY_MAX_SID_SIZE;
+  if (!CreateWellKnownSid(type, /*DomainSid=*/nullptr, buffer, &buffer_size)) {
+    PLOG(ERROR) << "CreateWellKnownSid() failed!";
+    LocalFree(descriptor);

[Sergey Ulanov, 2016/06/23 22:28:41]

do you need to free old_nacl here and below?

[Hzj_jie, 2016/06/23 22:35:58]

You can use unique_ptr with a custom deleter to avoid calling LocalFree before
return.

[Sergey Ulanov, 2016/06/23 22:43:40]

+1, that would be the best

[joedow, 2016/06/23 23:07:17]

Acknowledged.  I looked into this and it was more than I wanted to do for this
CL.  My plan is to implement a ScopedLocal type (or similar approach) with the
other Windows scoped ptrs and update the other areas of the codebase with it. 
I'll add a TODO and a bug for now.

[joedow, 2016/06/23 23:07:18]

old_dacl is a pointer into an opaque struct (|descriptor|).  I have a comment
that describes why it shouldn't be freed.  I think it will be more obvious that
it is intentional after I add a custom deleter.

+    return false;
+  }
+
+  SID* sid = reinterpret_cast<SID*>(buffer);
+  EXPLICIT_ACCESS new_access = {0};
+  new_access.grfAccessMode = GRANT_ACCESS;
+  new_access.grfAccessPermissions = new_right;
+  new_access.grfInheritance = NO_INHERITANCE;
+
+  new_access.Trustee.pMultipleTrustee = nullptr;
+  new_access.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
+  new_access.Trustee.TrusteeForm = TRUSTEE_IS_SID;
+  new_access.Trustee.ptstrName = reinterpret_cast<LPWSTR>(sid);
+
+  if (ERROR_SUCCESS != SetEntriesInAcl(1, &new_access, old_dacl, &new_dacl)) {

[Sergey Ulanov, 2016/06/23 22:28:41]

nit: move ERROR_SUCCESS to the end, i.e. (SetEntriesInAcl() != ERROR_SUCCESS).
That way it would be more readable and more consistent with the rest of the
code.

[joedow, 2016/06/23 23:07:17]

Done.

+    PLOG(ERROR) << "SetEntriesInAcl() failed!";
+    LocalFree(descriptor);
+    return false;
+  }
+
+  bool right_added = true;
+  if (SetSecurityInfo(process_handle.Get(),
+                      SE_KERNEL_OBJECT,
+                      DACL_SECURITY_INFORMATION,
+                      /*ppsidOwner=*/nullptr,
+                      /*ppsidGroup=*/nullptr,
+                      new_dacl,
+                      /*ppSacl=*/nullptr) != ERROR_SUCCESS) {
+    PLOG(ERROR) << "SetSecurityInfo() failed!";
+    right_added = false;
+  }
+
+  LocalFree(new_dacl);
+  LocalFree(descriptor);
+
+  return right_added;
+}
+}  // namespace
+#endif  // defined(OS_WIN)
+
 namespace remoting {
 
 int StartRemoteSecurityKey() {
 #if defined(OS_WIN)
+  if (!AddAccessRightForWellKnownSid(WinLocalServiceSid,
+                                     PROCESS_QUERY_LIMITED_INFORMATION)) {
+    return false;

[Hzj_jie, 2016/06/23 22:35:58]

The return type is int, not bool.

[joedow, 2016/06/23 23:07:17]

Ha, good catch :)

+  }
+
   // GetStdHandle() returns pseudo-handles for stdin and stdout even if
   // the hosting executable specifies "Windows" subsystem. However the returned
   // handles are invalid in that case unless standard input and output are
   // redirected to a pipe or file.
   base::File read_file(GetStdHandle(STD_INPUT_HANDLE));
   base::File write_file(GetStdHandle(STD_OUTPUT_HANDLE));
 
   // After the message handler starts, the remote security key message reader
   // will keep doing blocking read operations on the input named pipe.
   // If any other thread tries to perform any operation on STDIN, it will also
@@ skipping 31 matching lines @@
   base::AtExitManager exit_manager;
   base::MessageLoopForIO message_loop;
 
   base::CommandLine::Init(argc, argv);
   remoting::InitHostLogging();
 
   return StartRemoteSecurityKey();
 }
 
 }  // namespace remoting