From 4cf2637ddd76da9e43fdc4f545c4f1202bad2b24 Mon Sep 17 00:00:00 2001
From: "alexeypa@chromium.org"
 <alexeypa@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Tue, 18 Jun 2013 16:32:34 +0000
Subject: Configure security of the elevated controller in runtime.

This CL makes the elevated controller to configure its security (including the security descriptor of the server) in runtime by calling CoInitializeSecurity() instead of using declarative registry key (AccessPermissions). The problem is that when over-the-shoulder (OTS) COM elevation is used COM runtime uses the executable name to lookup the corresponding AppID\{xxx}\AccessPermissions value. We use the same binary to run different kind of processes so this mapping does not work for us very well.

Collateral changes:
 - CoInitializeSecurity() wrapper was moved to a separate file.

BUG=129477
R=jamiewalch@chromium.org

Review URL: https://codereview.chromium.org/17089004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207014 0039d316-1c4b-4281-b951-d872f2087c98
---
 remoting/host/installer/win/chromoting.wxs | 14 +-----
 remoting/host/win/chromoting_module.cc     | 24 +++++++++-
 remoting/host/win/com_security.cc          | 67 ++++++++++++++++++++++++++
 remoting/host/win/com_security.h           | 38 +++++++++++++++
 remoting/host/win/host_service.cc          | 75 +++---------------------------
 5 files changed, 136 insertions(+), 82 deletions(-)
 create mode 100644 remoting/host/win/com_security.cc
 create mode 100644 remoting/host/win/com_security.h

(limited to 'remoting/host')

diff --git a/remoting/host/installer/win/chromoting.wxs b/remoting/host/installer/win/chromoting.wxs
index 077e373..ce329c4 100644
--- a/remoting/host/installer/win/chromoting.wxs
+++ b/remoting/host/installer/win/chromoting.wxs
@@ -41,7 +41,7 @@
       "{987bca97-9d40-42fc-a00d-e6a701261af5}" ?>
 
   <!--
-    The long hex values below are security descriptors generated from SDDL
+    The long hex value(s) below are security descriptors generated from SDDL
     definition using the PowerShell script below:
 
     $sddl = "<SDDL definition goes here>"
@@ -50,15 +50,6 @@
   -->
 
   <!--
-    A security descriptor that allows SYSTEM, built-in administrators and
-    interactive users COM_RIGHTS_EXECUTE and COM_RIGHTS_EXECUTE_LOCAL rights.
-    The SDDL definition:
-
-    $sddl = "O:BAG:BAD:(A;;0x3;;;IU)(A;;0x3;;;SY)(A;;0x3;;;BA)"
-  -->
-  <?define ControllerSd = "010004805C0000006C00000000000000140000000200480003000000000014000300000001010000000000050400000000001400030000000101000000000005120000000000180003000000010200000000000520000000200200000102000000000005200000002002000001020000000000052000000020020000" ?>
-
-  <!--
     A security descriptor that gives SYSTEM, built-in administrators and
     LocalService accounts COM_RIGHTS_EXECUTE, COM_RIGHTS_EXECUTE_LOCAL, and
     COM_RIGHTS_ACTIVATE_LOCAL rights. It specifies a mandatory label that
@@ -259,9 +250,6 @@
             <RegistryKey Key="$(var.ControllerAppid)" Action="create">
               <RegistryValue Type="string"
                              Value="ChromotingElevatedController"/>
-              <RegistryValue Name="AccessPermission"
-                             Type="binary"
-                             Value="$(var.ControllerSd)"/>
             </RegistryKey>
 
             <RegistryKey Key="$(var.RdpAppid)" Action="create">
diff --git a/remoting/host/win/chromoting_module.cc b/remoting/host/win/chromoting_module.cc
index 47c7691..eb27fc9 100644
--- a/remoting/host/win/chromoting_module.cc
+++ b/remoting/host/win/chromoting_module.cc
@@ -4,15 +4,19 @@
 
 #include "remoting/host/win/chromoting_module.h"
 
+#include <sddl.h>
+
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/base/typed_buffer.h"
 #include "remoting/host/host_exit_codes.h"
+#include "remoting/host/win/com_security.h"
 #include "remoting/host/win/elevated_controller.h"
 #include "remoting/host/win/rdp_desktop_session.h"
 
@@ -20,6 +24,17 @@ namespace remoting {
 
 namespace {
 
+// A security descriptor allowing local processes running under SYSTEM, built-in
+// administrators and interactive users to call COM methods.
+const wchar_t kElevatedControllerSd[] =
+    SDDL_OWNER L":" SDDL_BUILTIN_ADMINISTRATORS
+    SDDL_GROUP L":" SDDL_BUILTIN_ADMINISTRATORS
+    SDDL_DACL L":"
+    SDDL_ACE(SDDL_ACCESS_ALLOWED, SDDL_COM_EXECUTE_LOCAL, SDDL_LOCAL_SYSTEM)
+    SDDL_ACE(SDDL_ACCESS_ALLOWED, SDDL_COM_EXECUTE_LOCAL,
+             SDDL_BUILTIN_ADMINISTRATORS)
+    SDDL_ACE(SDDL_ACCESS_ALLOWED, SDDL_COM_EXECUTE_LOCAL, SDDL_INTERACTIVE);
+
 // Holds a reference to the task runner used by the module.
 base::LazyInstance<scoped_refptr<AutoThreadTaskRunner> > g_module_task_runner =
     LAZY_INSTANCE_INITIALIZER;
@@ -187,7 +202,14 @@ int ElevatedControllerMain() {
 
   ChromotingModule module(elevated_controller_entry,
                           elevated_controller_entry + 1);
-  return module.Run() ? kSuccessExitCode : kInitializationFailed;
+
+  if (!InitializeComSecurity(WideToUTF8(kElevatedControllerSd), "", true))
+    return kInitializationFailed;
+
+  if (!module.Run())
+    return kInitializationFailed;
+
+  return kSuccessExitCode;
 }
 
 // RdpClient entry point.
diff --git a/remoting/host/win/com_security.cc b/remoting/host/win/com_security.cc
new file mode 100644
index 0000000..34b3dc4
--- /dev/null
+++ b/remoting/host/win/com_security.cc
@@ -0,0 +1,67 @@
+// Copyright 2013 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/win/com_security.h"
+
+#include <objidl.h>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/win/windows_version.h"
+#include "remoting/host/win/security_descriptor.h"
+
+namespace remoting {
+
+bool InitializeComSecurity(const std::string& security_descriptor,
+                           const std::string& mandatory_label,
+                           bool activate_as_activator) {
+  std::string sddl = security_descriptor;
+  if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
+    sddl += mandatory_label;
+  }
+
+  // Convert the SDDL description into a security descriptor in absolute format.
+  ScopedSd relative_sd = ConvertSddlToSd(sddl);
+  if (!relative_sd) {
+    LOG_GETLASTERROR(ERROR) << "Failed to create a security descriptor";
+    return false;
+  }
+  ScopedSd absolute_sd;
+  ScopedAcl dacl;
+  ScopedSid group;
+  ScopedSid owner;
+  ScopedAcl sacl;
+  if (!MakeScopedAbsoluteSd(relative_sd, &absolute_sd, &dacl, &group, &owner,
+                            &sacl)) {
+    LOG_GETLASTERROR(ERROR) << "MakeScopedAbsoluteSd() failed";
+    return false;
+  }
+
+  DWORD capabilities = EOAC_DYNAMIC_CLOAKING;
+  if (!activate_as_activator)
+    capabilities |= EOAC_DISABLE_AAA;
+
+  // Apply the security descriptor and default security settings. See
+  // InitializeComSecurity's declaration for details.
+  HRESULT result = CoInitializeSecurity(
+      absolute_sd.get(),
+      -1,       // Let COM choose which authentication services to register.
+      NULL,     // See above.
+      NULL,     // Reserved, must be NULL.
+      RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
+      RPC_C_IMP_LEVEL_IDENTIFY,
+      NULL,     // Default authentication information is not provided.
+      capabilities,
+      NULL);    /// Reserved, must be NULL
+  if (FAILED(result)) {
+    LOG(ERROR) << "CoInitializeSecurity() failed, result=0x"
+               << std::hex << result << std::dec << ".";
+    return false;
+  }
+
+  return true;
+}
+
+} // namespace remoting
diff --git a/remoting/host/win/com_security.h b/remoting/host/win/com_security.h
new file mode 100644
index 0000000..846450c
--- /dev/null
+++ b/remoting/host/win/com_security.h
@@ -0,0 +1,38 @@
+// Copyright 2013 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.
+
+#ifndef REMOTING_HOST_WIN_COM_SECURITY_H_
+#define REMOTING_HOST_WIN_COM_SECURITY_H_
+
+#include <string>
+
+// Concatenates ACE type, permissions and sid given as SDDL strings into an ACE
+// definition in SDDL form.
+#define SDDL_ACE(type, permissions, sid) \
+    L"(" type L";;" permissions L";;;" sid L")"
+
+// Text representation of COM_RIGHTS_EXECUTE and COM_RIGHTS_EXECUTE_LOCAL
+// permission bits that is used in the SDDL definition below.
+#define SDDL_COM_EXECUTE_LOCAL L"0x3"
+
+namespace remoting {
+
+// Initializes COM security of the process applying the passed security
+// descriptor. The mandatory label is applied if mandatory integrity control is
+// supported by the OS (i.e. on Vista and above). The function configures
+// the following settings:
+//  - the server authenticates that all data received is from the expected
+//    client.
+//  - the server can impersonate clients to check their identity but cannot act
+//    on their behalf.
+//  - the caller's identity is verified on every call (Dynamic cloaking).
+//  - Unless |activate_as_activator| is true, activations where the server would
+//    run under this process's identity are prohibited.
+bool InitializeComSecurity(const std::string& security_descriptor,
+                           const std::string& mandatory_label,
+                           bool activate_as_activator);
+
+} // namespace remoting
+
+#endif  // REMOTING_HOST_WIN_COM_SECURITY_H_
diff --git a/remoting/host/win/host_service.cc b/remoting/host/win/host_service.cc
index a76eb05..6b95dec 100644
--- a/remoting/host/win/host_service.cc
+++ b/remoting/host/win/host_service.cc
@@ -22,15 +22,14 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
 #include "base/win/scoped_com_initializer.h"
-#include "base/win/windows_version.h"
 #include "remoting/base/auto_thread.h"
 #include "remoting/base/scoped_sc_handle_win.h"
 #include "remoting/host/branding.h"
 #include "remoting/host/daemon_process.h"
 #include "remoting/host/host_exit_codes.h"
 #include "remoting/host/logging.h"
+#include "remoting/host/win/com_security.h"
 #include "remoting/host/win/core_resource.h"
-#include "remoting/host/win/security_descriptor.h"
 #include "remoting/host/win/wts_terminal_observer.h"
 
 namespace remoting {
@@ -44,15 +43,6 @@ const char kIoThreadName[] = "I/O thread";
 // "--console" runs the service interactively for debugging purposes.
 const char kConsoleSwitchName[] = "console";
 
-// Concatenates ACE type, permissions and sid given as SDDL strings into an ACE
-// definition in SDDL form.
-#define SDDL_ACE(type, permissions, sid) \
-    L"(" type L";;" permissions L";;;" sid L")"
-
-// Text representation of COM_RIGHTS_EXECUTE and COM_RIGHTS_EXECUTE_LOCAL
-// permission bits that is used in the SDDL definition below.
-#define SDDL_COM_EXECUTE_LOCAL L"0x3"
-
 // Security descriptor allowing local processes running under SYSTEM or
 // LocalService accounts to call COM methods exposed by the daemon.
 const wchar_t kComProcessSd[] =
@@ -68,61 +58,6 @@ const wchar_t kComProcessMandatoryLabel[] =
     SDDL_SACL L":"
     SDDL_ACE(SDDL_MANDATORY_LABEL, SDDL_NO_EXECUTE_UP, SDDL_ML_MEDIUM);
 
-#undef SDDL_ACE
-#undef SDDL_COM_EXECUTE_LOCAL
-
-// Allows incoming calls from clients running under SYSTEM or LocalService at
-// medium integrity level.
-bool InitializeComSecurity() {
-  std::string sddl = WideToUTF8(kComProcessSd);
-  if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
-    sddl += WideToUTF8(kComProcessMandatoryLabel);
-  }
-
-  // Convert the SDDL description into a security descriptor in absolute format.
-  ScopedSd relative_sd = ConvertSddlToSd(sddl);
-  if (!relative_sd) {
-    LOG_GETLASTERROR(ERROR) << "Failed to create a security descriptor";
-    return false;
-  }
-  ScopedSd absolute_sd;
-  ScopedAcl dacl;
-  ScopedSid group;
-  ScopedSid owner;
-  ScopedAcl sacl;
-  if (!MakeScopedAbsoluteSd(relative_sd, &absolute_sd, &dacl, &group, &owner,
-                            &sacl)) {
-    LOG_GETLASTERROR(ERROR) << "MakeScopedAbsoluteSd() failed";
-    return false;
-  }
-
-  // Apply the security descriptor and the following settings:
-  //   - The daemon authenticates that all data received is from the expected
-  //     client.
-  //   - The daemon can impersonate clients to check their identity but cannot
-  //     act on their behalf.
-  //   - The caller's identity on every call (Dynamic cloaking).
-  //   - Activations where the activated COM server would run under the daemon's
-  //     identity are prohibited.
-  HRESULT result = CoInitializeSecurity(
-      absolute_sd.get(),
-      -1,       // Let COM choose which authentication services to register.
-      NULL,     // See above.
-      NULL,     // Reserved, must be NULL.
-      RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
-      RPC_C_IMP_LEVEL_IDENTIFY,
-      NULL,     // Default authentication information is not provided.
-      EOAC_DYNAMIC_CLOAKING | EOAC_DISABLE_AAA,
-      NULL);    /// Reserved, must be NULL
-  if (FAILED(result)) {
-    LOG(ERROR) << "CoInitializeSecurity() failed, result=0x"
-               << std::hex << result << std::dec << ".";
-    return false;
-  }
-
-  return true;
-}
-
 }  // namespace
 
 HostService* HostService::GetInstance() {
@@ -336,8 +271,10 @@ void HostService::RunAsServiceImpl() {
   if (!com_initializer.succeeded())
     return;
 
-  if (!InitializeComSecurity())
+  if (!InitializeComSecurity(WideToUTF8(kComProcessSd),
+                             WideToUTF8(kComProcessMandatoryLabel), false)) {
     return;
+  }
 
   CreateLauncher(scoped_refptr<AutoThreadTaskRunner>(
       new AutoThreadTaskRunner(main_task_runner_,
@@ -370,8 +307,10 @@ int HostService::RunInConsole() {
   if (!com_initializer.succeeded())
     return result;
 
-  if (!InitializeComSecurity())
+  if (!InitializeComSecurity(WideToUTF8(kComProcessSd),
+                             WideToUTF8(kComProcessMandatoryLabel), false)) {
     return result;
+  }
 
   // Subscribe to Ctrl-C and other console events.
   if (!SetConsoleCtrlHandler(&HostService::ConsoleControlHandler, TRUE)) {
-- 
cgit v1.1