Initialize COM and configure security settings in the daemon.

Initialize COM and configure security settings in the daemon.

This CL initializes a single-threaded apartment on the main thread in the daemon and configures COM security in the following way:
  - 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.

BUG=137696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186245

[alexeypa (please no reviews), 2013-03-04 17:50:01]

PTAL.

[alexeypa (please no reviews), 2013-03-04 19:18:46]

Adding Cris, because it is security related.

Some background:

The daemon (running under SYSTEM) will be activating an out-of-process COM
object configured to run under LocalService at medium integrity level.

The daemon will pass to it a COM interface pointer to be used as a callback. The
daemon is not hosting any other COM classes.

This CL tries to make sure that:
- the daemon does not let the object it activated to impersonate the daemon. 
- the activated object can call methods of the callback passed by the daemon.

[Cris Neckar, 2013-03-04 19:56:49]

Adding jschuh@ as he has a much better understanding of COM.

[jschuh, 2013-03-04 21:12:33]

Seems broadly okay, but with some points that would make the code a bit clearer.

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
File remoting/host/win/host_service.cc (right):

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
remoting/host/win/host_service.cc:72:
"O:SYG:SYD:(A;;0x3;;;SY)(A;;0x3;;;LS)S:(ML;;NX;;;ME)";
SDDL can be pretty impenetrable. Generally, I find it much clearer to break the
string into pieces with comments and build it using the macros from sddl.h, like
we do in restricted_token_utils.cc.

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
remoting/host/win/host_service.cc:99: //   - Dynamic cloacking is used. DCOM
verifies the caller's identify on every
nit: cloaking

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/security_de...
File remoting/host/win/security_descriptor.h (right):

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/security_de...
remoting/host/win/security_descriptor.h:32: bool MakeAbsoluteSd(const ScopedSd&
relative_sd,
This choice of naming is a bit evil, because it took me several minutes to
figure out what was going on since it differs from the system function only by
the case of the last letter. Please call it something like MakeScopedAbsoluteSD,
so it's clear how and why it differs from the system function.

[alexeypa (please no reviews), 2013-03-04 21:46:30]

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
File remoting/host/win/host_service.cc (right):

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
remoting/host/win/host_service.cc:72:
"O:SYG:SYD:(A;;0x3;;;SY)(A;;0x3;;;LS)S:(ML;;NX;;;ME)";
On 2013/03/04 21:12:33, Justin Schuh wrote:
> SDDL can be pretty impenetrable. Generally, I find it much clearer to break
the
> string into pieces with comments and build it using the macros from sddl.h,
like
> we do in restricted_token_utils.cc.

Done.

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/host_servic...
remoting/host/win/host_service.cc:99: //   - Dynamic cloacking is used. DCOM
verifies the caller's identify on every
On 2013/03/04 21:12:33, Justin Schuh wrote:
> nit: cloaking

Done.

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/security_de...
File remoting/host/win/security_descriptor.h (right):

https://codereview.chromium.org/12378078/diff/1/remoting/host/win/security_de...
remoting/host/win/security_descriptor.h:32: bool MakeAbsoluteSd(const ScopedSd&
relative_sd,
On 2013/03/04 21:12:33, Justin Schuh wrote:
> This choice of naming is a bit evil, because it took me several minutes to
> figure out what was going on since it differs from the system function only by
> the case of the last letter. Please call it something like
MakeScopedAbsoluteSD,
> so it's clear how and why it differs from the system function.

Done.

[jschuh, 2013-03-04 21:54:15]

Thanks. That's much easier for me to read. lgtm on the security side.

[Wez, 2013-03-05 02:25:27]

LGTM w/ nits:

There are a couple of typos in the CL description.

The CL description should also mention why we need the daemon to have COM
initialized at all.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
File remoting/host/win/host_service.cc (right):

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:69: // definition is SDDL form.
typo: is -> in

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:80: // level.
nit: Clarify why, e.g. "The descriptor has the "no execute up" mandatory label,
preventing lower integrity level processes using it."?

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:118: //   - Dynamic cloaking is used. DCOM
verifies the caller's identify on every
nit: Is the second sentence just a description of the first? In which case
suggest "DCOM ... every call (aka 'Dynamic cloaking')"

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:118: //   - Dynamic cloaking is used. DCOM
verifies the caller's identify on every
typo: identity

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:121: //     identify are prohibited.
typo: identify -> identity

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:124: -1,
nit: Comment the numeric/NULL parameters, e.g. -1, // i^2 or something.

[alexeypa (please no reviews), 2013-03-05 18:00:27]

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
File remoting/host/win/host_service.cc (right):

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:69: // definition is SDDL form.
On 2013/03/05 02:25:28, Wez wrote:
> typo: is -> in

Done.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:80: // level.
On 2013/03/05 02:25:28, Wez wrote:
> nit: Clarify why, e.g. "The descriptor has the "no execute up" mandatory
label,
> preventing lower integrity level processes using it."?

Done.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:118: //   - Dynamic cloaking is used. DCOM
verifies the caller's identify on every
On 2013/03/05 02:25:28, Wez wrote:
> typo: identity

Done.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:118: //   - Dynamic cloaking is used. DCOM
verifies the caller's identify on every
On 2013/03/05 02:25:28, Wez wrote:
> nit: Is the second sentence just a description of the first?

No.

> In which case
> suggest "DCOM ... every call (aka 'Dynamic cloaking')"

Done.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:121: //     identify are prohibited.
On 2013/03/05 02:25:28, Wez wrote:
> typo: identify -> identity

Done.

https://codereview.chromium.org/12378078/diff/9001/remoting/host/win/host_ser...
remoting/host/win/host_service.cc:124: -1,
On 2013/03/05 02:25:28, Wez wrote:
> nit: Comment the numeric/NULL parameters, e.g. -1, // i^2 or something.

Done.

[commit-bot: I haz the power, 2013-03-05 18:01:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12378078/10001

[alexeypa (please no reviews), 2013-03-05 20:33:01]

Committed patchset #3 manually as r186245 (presubmit successful).