Allow network service to query remote_security_key process for its session ID

remoting/host/security_key/remote_security_key_main.cc

Index: remoting/host/security_key/remote_security_key_main.cc
diff --git a/remoting/host/security_key/remote_security_key_main.cc b/remoting/host/security_key/remote_security_key_main.cc
index c68f2e6078453160fa9c18ea004020495f614e4f..1d5590dfd403c6f241e6ae9b5be968663929575e 100644
--- a/remoting/host/security_key/remote_security_key_main.cc
+++ b/remoting/host/security_key/remote_security_key_main.cc
@@ -17,10 +17,96 @@
 #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