Chromoting: Implemented security attention sequence (SAS) emulation on Windows.

Chromoting: Implemented security attention sequence (SAS) emulation on Windows.

The default SAS is Ctrl+Alt+Delete. It can neither be intercepted by the client (at least on Windows) nor it can be injected the same way other key strokes are injected. This CL makes the Windows host to interpret a double Scroll Lock as a SAS and invoke the Chromoting service to issue the actual SAS.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126186

[alexeypa (please no reviews), 2012-03-06 23:52:24]

Please, take a look.

darin -> ipc/ipc_message_utils.h
wez -> everything else.

garykac -> FYI: see the double Scroll Lock handling on Windows.

[darin (slow to review), 2012-03-07 00:38:32]

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:103: ChromotingServiceMsgStart,
i'd prefer to have jam@ sign off on this.  he should be aware of all IPC
consumers in chrome land.

[jam, 2012-03-07 00:51:26]

ipc lgtm

http://codereview.chromium.org/9617027/diff/1/remoting/host/chromoting_servic...
File remoting/host/chromoting_service_messages.h (right):

http://codereview.chromium.org/9617027/diff/1/remoting/host/chromoting_servic...
remoting/host/chromoting_service_messages.h:10: #ifndef
REMOTING_HOST_CHROMOTING_SERVICE_MESSAGES_H_
you don't need this block since it's empty

[Wez, 2012-03-07 01:56:12]

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:103: ChromotingServiceMsgStart,
nit: The term "service" used for the process type that runs Cloud Print, etc, so
we've been referring to the Chromoting host process as a "daemon" to avoid
confusion.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
File remoting/host/chromoting_service_messages.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
remoting/host/chromoting_service_messages.h:5: // Defines messages between the
Chrmoting host and service.
typo: Chromoting

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
remoting/host/chromoting_service_messages.h:22:
IPC_MESSAGE_CONTROL0(ChromotingServiceMsg_SendSas)
nit: SendSasToConsole?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
File remoting/host/event_executor_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
remoting/host/event_executor_win.cc:35: Capturer* capturer);
EventExecutorWin is responsible just for injecting the events; the SendSAS and
desktop-switching features feel like they belong in a separate
EventExecutorServiceWin class that would call on to EventExecutorWin to do the
actual injection after handling the service-specific work?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:114: class SasSenderVista : public SasSender {
Why is this SasSanderVista rather than SasSenderWin?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:138: // the same folder as this binary.
nit: Premature line-wrap.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:168: // if the policy does not applow services
to generate software SAS.
typo: allow

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:171: (*send_sas_)(FALSE);
Is it a problem for us to call SendSAS if the policy tweaking didn't work?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:174: return true;
Do you mean to return true whether or not the policy tweaking worked?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:15: // sequence is Ctrl+Alt+Delete.
nit: What does it mean for the default sequence to be C-A-D?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:16: class SasSender {
nit: For other input we have EventInjector, so perhaps this would better be
named SasInjector?  (Also SasSender sounds like it might be the IPC send end of
the SAS event)

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:21: virtual bool Send() = 0;
nit: Send -> SendSAS / InjectSAS?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:23: static scoped_ptr<SasSender> Create();
nit: Clarify that Create can return NULL if SendSAS() is not supported on the
platform?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:89: const int kDefaultMaxPortNumber =
12409;
nit: Not part of this CL?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.cc:201: //      Everyone group
and the anonymous account.
nit: Clarify why read access is not an issue?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.cc:205: //      order to connect.
nit: How is this comment relevant to "the result", below?  Are you saying that
clients only permitted read acess can't connect to DUPLEX pipes?

[alexeypa (please no reviews), 2012-03-07 19:59:08]

Wez, PTAL. This is a rebase + a bunch of renamings.

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:103: ChromotingServiceMsgStart,
On 2012/03/07 01:56:13, Wez wrote:
> nit: The term "service" used for the process type that runs Cloud Print, etc,
so
> we've been referring to the Chromoting host process as a "daemon" to avoid
> confusion.

It is "Chromoting Session" now.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
File remoting/host/chromoting_service_messages.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
remoting/host/chromoting_service_messages.h:5: // Defines messages between the
Chrmoting host and service.
On 2012/03/07 01:56:13, Wez wrote:
> typo: Chromoting

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
remoting/host/chromoting_service_messages.h:10: #ifndef
REMOTING_HOST_CHROMOTING_SERVICE_MESSAGES_H_
On 2012/03/07 00:51:26, John Abd-El-Malek wrote:
> you don't need this block since it's empty

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/chromotin...
remoting/host/chromoting_service_messages.h:22:
IPC_MESSAGE_CONTROL0(ChromotingServiceMsg_SendSas)
On 2012/03/07 01:56:13, Wez wrote:
> nit: SendSasToConsole?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
File remoting/host/event_executor_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
remoting/host/event_executor_win.cc:35: Capturer* capturer);
On 2012/03/07 01:56:13, Wez wrote:
> EventExecutorWin is responsible just for injecting the events; the SendSAS and
> desktop-switching features feel like they belong in a separate
> EventExecutorServiceWin class that would call on to EventExecutorWin to do the
> actual injection after handling the service-specific work?

There is DesktopEnvironment class that looks like a better choice for hosting
the IPC channel.
It will give a reference to the channel to the executor and capturer. The
executor will use the channel to inject SAS. I think it is aligned with the
purpose of the executor perfectly. After all it is just another even to inject.
Once the captures will be moved out to a separate component it will use the IPC
channel to talk to encoder.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:114: class SasSenderVista : public SasSender {
On 2012/03/07 01:56:13, Wez wrote:
> Why is this SasSanderVista rather than SasSenderWin?

Because, potentially, we can have a different implementation for Windows XP.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:138: // the same folder as this binary.
On 2012/03/07 01:56:13, Wez wrote:
> nit: Premature line-wrap.

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:168: // if the policy does not applow services
to generate software SAS.
On 2012/03/07 01:56:13, Wez wrote:
> typo: allow

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:171: (*send_sas_)(FALSE);
On 2012/03/07 01:56:13, Wez wrote:
> Is it a problem for us to call SendSAS if the policy tweaking didn't work?

Yes. It does nothing if the policy is not enabled. We have to call SendSAS()
regardless what we think the current state of the policy is. Winlogon is the
only one who can make the final policy decision.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:174: return true;
On 2012/03/07 01:56:13, Wez wrote:
> Do you mean to return true whether or not the policy tweaking worked?

Yes. False is returned only if any of the operations failed.  A SAS blocked by
the policy is considered a successful operation since nothing is broken.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:15: // sequence is Ctrl+Alt+Delete.
On 2012/03/07 01:56:13, Wez wrote:
> nit: What does it mean for the default sequence to be C-A-D?

It implies that there are other SASs such as inserting a smart card or a
notification from a biometric device.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:16: class SasSender {
On 2012/03/07 01:56:13, Wez wrote:
> nit: For other input we have EventInjector, so perhaps this would better be
> named SasInjector?  (Also SasSender sounds like it might be the IPC send end
of
> the SAS event)

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:21: virtual bool Send() = 0;
On 2012/03/07 01:56:13, Wez wrote:
> nit: Send -> SendSAS / InjectSAS?

InjectSAS

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:23: static scoped_ptr<SasSender> Create();
On 2012/03/07 01:56:13, Wez wrote:
> nit: Clarify that Create can return NULL if SendSAS() is not supported on the
> platform?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:89: const int kDefaultMaxPortNumber =
12409;
On 2012/03/07 01:56:13, Wez wrote:
> nit: Not part of this CL?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.cc:201: //      Everyone group
and the anonymous account.
On 2012/03/07 01:56:13, Wez wrote:
> nit: Clarify why read access is not an issue?

See PIPE_ACCESS_DUPLEX below

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.cc:205: //      order to connect.
On 2012/03/07 01:56:13, Wez wrote:
> Are you saying that
> clients only permitted read acess can't connect to DUPLEX pipes?


Yes. I rephrased the comment to articulate it better.

[Wez, 2012-03-08 00:01:33]

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:103: ChromotingServiceMsgStart,
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > nit: The term "service" used for the process type that runs Cloud Print,
etc,
> so
> > we've been referring to the Chromoting host process as a "daemon" to avoid
> > confusion.
> 
> It is "Chromoting Session" now.

When we discussed this, I thought you were referring to the channel name; the
message-Id space should probably stay as ChromotingService/DaemonMsgStart - it
could be re-used for non-session-related messages in future.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
File remoting/host/event_executor_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
remoting/host/event_executor_win.cc:35: Capturer* capturer);
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > EventExecutorWin is responsible just for injecting the events; the SendSAS
and
> > desktop-switching features feel like they belong in a separate
> > EventExecutorServiceWin class that would call on to EventExecutorWin to do
the
> > actual injection after handling the service-specific work?
> 
> There is DesktopEnvironment class that looks like a better choice for hosting
> the IPC channel.
> It will give a reference to the channel to the executor and capturer. The
> executor will use the channel to inject SAS. I think it is aligned with the
> purpose of the executor perfectly. After all it is just another even to
inject.
> Once the captures will be moved out to a separate component it will use the
IPC
> channel to talk to encoder. 

DesktopEnvironment is a cross-platform interface for exposing the cross-platform
interfaces to the platform-specific integration implementations to the
ChromotingHost, so it shouldn't know anything about the IPC channel.

The EventExecutorWin is designed just to inject input events; it's used in IT2Me
and so should not have any knowledge of issues specific to running in the
context of a service.

You're right that it's just another event to inject - this is why I suggested
having an EventExecutorServiceWin that takes care of the service-specific work,
i.e. IPC and desktop switching, and uses an EventExecutorWin to do normal key
event injection.  EventExecutorServiceWin would expose the same InputStub
interface.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:114: class SasSenderVista : public SasSender {
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > Why is this SasSanderVista rather than SasSenderWin?
> 
> Because, potentially, we can have a different implementation for Windows XP.

That's fine - this version is SasSenderWin, and one we add for older systems can
be SasSenderXP.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:171: (*send_sas_)(FALSE);
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > Is it a problem for us to call SendSAS if the policy tweaking didn't work?
> 
> Yes. It does nothing if the policy is not enabled. We have to call SendSAS()
> regardless what we think the current state of the policy is. 

So we shouldn't be testing the result of |enable_sas.Apply()| in that case.

> Winlogon is the
> only one who can make the final policy decision.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:174: return true;
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > Do you mean to return true whether or not the policy tweaking worked?
> 
> Yes. False is returned only if any of the operations failed.  A SAS blocked by
> the policy is considered a successful operation since nothing is broken.

Is it worth returning false, in that case, since there's nothing we can do about
it?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:15: // sequence is Ctrl+Alt+Delete.
On 2012/03/07 19:59:08, alexeypa wrote:
> On 2012/03/07 01:56:13, Wez wrote:
> > nit: What does it mean for the default sequence to be C-A-D?
> 
> It implies that there are other SASs such as inserting a smart card or a
> notification from a biometric device.

I'm not sure what you mean - regardless of whether it's generated by a biometric
device or C-A-D it's a Secure Attention Sequence, so in what sense are we
generating a C-A-D?

Or are you just trying to clarify what is meant by Secure Attention Sequence?

https://chromiumcodereview.appspot.com/9617027/diff/8001/remoting/host/wts_se...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/8001/remoting/host/wts_se...
remoting/host/wts_session_process_launcher_win.cc:206: //      (Everyone and
anonymous) will not be able to connect.
nit: ... to connect, which means ...

The comment makes more sense, but as a security guarantee it seems a little
weak.

[alexeypa (please no reviews), 2012-03-08 01:52:54]

FYI.

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:103: ChromotingServiceMsgStart,
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > nit: The term "service" used for the process type that runs Cloud Print,
> etc,
> > so
> > > we've been referring to the Chromoting host process as a "daemon" to avoid
> > > confusion.
> > 
> > It is "Chromoting Session" now.
> 
> When we discussed this, I thought you were referring to the channel name; the
> message-Id space should probably stay as ChromotingService/DaemonMsgStart - it
> could be re-used for non-session-related messages in future.

If this is going to ab an umbrella for all Chromoting IPC it makes sense to call
it "ChromotingMsgStart" and define messages like 
"ChromotingSessionMsg_SendSas", "ChromotingFooBarMsg_Bla"

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
File remoting/host/event_executor_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/event_exe...
remoting/host/event_executor_win.cc:35: Capturer* capturer);
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > EventExecutorWin is responsible just for injecting the events; the SendSAS
> and
> > > desktop-switching features feel like they belong in a separate
> > > EventExecutorServiceWin class that would call on to EventExecutorWin to do
> the
> > > actual injection after handling the service-specific work?
> > 
> > There is DesktopEnvironment class that looks like a better choice for
hosting
> > the IPC channel.
> > It will give a reference to the channel to the executor and capturer. The
> > executor will use the channel to inject SAS. I think it is aligned with the
> > purpose of the executor perfectly. After all it is just another even to
> inject.
> > Once the captures will be moved out to a separate component it will use the
> IPC
> > channel to talk to encoder. 
> 
> DesktopEnvironment is a cross-platform interface for exposing the
cross-platform
> interfaces to the platform-specific integration implementations to the
> ChromotingHost, so it shouldn't know anything about the IPC channel.
> 
> The EventExecutorWin is designed just to inject input events; it's used in
IT2Me
> and so should not have any knowledge of issues specific to running in the
> context of a service.
> 
> You're right that it's just another event to inject - this is why I suggested
> having an EventExecutorServiceWin that takes care of the service-specific
work,
> i.e. IPC and desktop switching, and uses an EventExecutorWin to do normal key
> event injection.  EventExecutorServiceWin would expose the same InputStub
> interface.

OK, makes sense.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:114: class SasSenderVista : public SasSender {
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > Why is this SasSanderVista rather than SasSenderWin?
> > 
> > Because, potentially, we can have a different implementation for Windows XP.
> 
> That's fine - this version is SasSenderWin, and one we add for older systems
can
> be SasSenderXP.

Once we do that the naming will become odd: why is XP not Windows? Or why is
Windows does not include XP?

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:171: (*send_sas_)(FALSE);
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > Is it a problem for us to call SendSAS if the policy tweaking didn't work?
> > 
> > Yes. It does nothing if the policy is not enabled. We have to call SendSAS()
> > regardless what we think the current state of the policy is. 
> 
> So we shouldn't be testing the result of |enable_sas.Apply()| in that case.

No, we should. False means 'something is badly wrong, let's not touch anything
and return'. True means we didn't see any error, so we can try sending SAS.

Note I said nothing about tweaking the policy by us above. It does not matter,
really. The policy could be in default state and we enabled it. It could be in
non default state (and we didn't touch it), but can still allows services to
generate SAS. IN either case we need to call SendSAS().

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.cc:174: return true;
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > Do you mean to return true whether or not the policy tweaking worked?
> > 
> > Yes. False is returned only if any of the operations failed.  A SAS blocked
by
> > the policy is considered a successful operation since nothing is broken.
> 
> Is it worth returning false, in that case, since there's nothing we can do
about
> it?

We should (and do) behave differently. See above.

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
File remoting/host/sas_sender_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/1/remoting/host/sas_sende...
remoting/host/sas_sender_win.h:15: // sequence is Ctrl+Alt+Delete.
On 2012/03/08 00:01:33, Wez wrote:
> On 2012/03/07 19:59:08, alexeypa wrote:
> > On 2012/03/07 01:56:13, Wez wrote:
> > > nit: What does it mean for the default sequence to be C-A-D?
> > 
> > It implies that there are other SASs such as inserting a smart card or a
> > notification from a biometric device.
> 
> I'm not sure what you mean - regardless of whether it's generated by a
biometric
> device or C-A-D it's a Secure Attention Sequence, so in what sense are we
> generating a C-A-D?
> 
> Or are you just trying to clarify what is meant by Secure Attention Sequence?

The latter.

https://chromiumcodereview.appspot.com/9617027/diff/8001/remoting/host/wts_se...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/8001/remoting/host/wts_se...
remoting/host/wts_session_process_launcher_win.cc:206: //      (Everyone and
anonymous) will not be able to connect.
On 2012/03/08 00:01:33, Wez wrote:
> nit: ... to connect, which means ...
> 
> The comment makes more sense, but as a security guarantee it seems a little
> weak.

Actually, there is much more serious problem with the client end of the pipe. In
it's current implementation it can leak LocalSystem context to a low-privileged
process. I'll need to harden this a little bit.

[alexeypa (please no reviews), 2012-03-08 19:08:10]

I've updated the CL. Added:

- SessionEventExecutor for SAS
- Strong security descriptor for the pipe.
- Random (chrome-style) pipe name.
- Minor tweaks.

[alexeypa (please no reviews), 2012-03-08 20:06:10]

FYI: another update finalizing the names and removing spotted debug-only code.

[Wez, 2012-03-08 22:58:55]

https://chromiumcodereview.appspot.com/9617027/diff/25001/ipc/ipc_message_uti...
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/25001/ipc/ipc_message_uti...
ipc/ipc_message_utils.h:103: ChromotingSessionMsgStart,
nit: Did you want to re-name this ChromotingMsgStart, as per our discussion?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
File remoting/host/desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:23: scoped_ptr<EventExecutor>
event_executor(
nit: Since the EventExecutor interface doesn't add anything over InputStub, I
think the DesktopEnvironment should return an InputStub here - my suggested
re-parenting of SessionEventExecutor from EventExecutor to InputStub would
require that change - the EventExecutor interface could actually go away
entirely....

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:35: event_executor.Pass()));
nit: This is a shame; we really only want to use this implementation when
running in the daemon, so we should be able to choose the impl to use.

DesktopEnvironment was supposed to be an interface that lets the implementations
be chosen on a per-application basis, for this reason. :-/

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.cc:114: class SasInjectorVista : public
SasInjector {
We discussed removing Vista from this name in favour of Win (or WinImpl, as in
the header).

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.h:5: #ifndef REMOTING_HOST_SAS_INJECTOR_WIN_H_
The guard should really match the class name.  Probably the best way to do that,
since you want the files to be _win.*, is to name the interface SasInjectorWin
and the implementation SasInjectorWinImpl.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
File remoting/host/session_event_executor_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:20: class MessageLoopProxy;
nit: Does the style guide require that there be blank lines around these
forward-decls? If not then I'd suggest removing them to aid readability.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
nit: SessionEventExecutorWin, to match the guard & filename?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
public EventExecutor -> public InputStub

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
I'm not convinced by the name; what is "session" about this class?  It's really
more a "daemon" event-injector, surely?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:35: SessionEventExecutor(MessageLoop*
message_loop,
It's not clear what this class does with |message_loop|, or what happens to
events that have been injected, posted to |message_loop|, but not yet processed
by the SessionEventExecutor, if it gets deleted?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:37: scoped_ptr<EventExecutor>
nested_executor);
scoped_ptr<EventExecutor> -> scoped_ptr<InputStub>

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:43: const char
kChromePipeNamePrefix[] = "\\\\.\\pipe\\chrome.";
nit: Comment this.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:45: // Generatesthe command
line of the host process.
typo: Generates the ...

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:147: // Creates the server end
of the Chromoting IPC channel.
nit: Similar comment "based on IPC::Channel"?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:176: // IPC::Channel with
exception of passing a non-default security descriptor.
Not for this CL, but would adding support for this usage to IPC::Channel make
sense?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:275:
IPC::ChannelHandle(pipe.Get()),
nit: This indentation looks weird; can you put the items 4 bytes in from the
start of |chromoting_channel_|?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:70: // Sends the security
attention sequence to the session represented by
nit: Secure Attention Sequence

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:86: // Pointer to the I/O
thread.
nit: I can see that from the naming.  What's it used for, though? ;)

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:114: // The server endpoint of
the chromoting IPC channel.
Is it relevant that it's the server endpoint? Would it be valid to describe it
as the IPC channel to the per-session process?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.g...
remoting/remoting.gyp:419: [ 'OS=="win"', {
nit: Put this between OS=="mac" and OS!="win"?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.g...
remoting/remoting.gyp:421: '../ipc/ipc.gyp:ipc'
nit: It's a shame to have the plugin depend on IPC, since it will (currently)
never use it. :(

[alexeypa (please no reviews), 2012-03-09 01:13:54]

PTAL. Another round of renamings.

https://chromiumcodereview.appspot.com/9617027/diff/25001/ipc/ipc_message_uti...
File ipc/ipc_message_utils.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/25001/ipc/ipc_message_uti...
ipc/ipc_message_utils.h:103: ChromotingSessionMsgStart,
On 2012/03/08 22:58:55, Wez wrote:
> nit: Did you want to re-name this ChromotingMsgStart, as per our discussion?

It is renamed in the patch set #5.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
File remoting/host/desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:23: scoped_ptr<EventExecutor>
event_executor(
On 2012/03/08 22:58:55, Wez wrote:
> nit: Since the EventExecutor interface doesn't add anything over InputStub, I
> think the DesktopEnvironment should return an InputStub here - my suggested
> re-parenting of SessionEventExecutor from EventExecutor to InputStub would
> require that change - the EventExecutor interface could actually go away
> entirely....

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:35: event_executor.Pass()));
On 2012/03/08 22:58:55, Wez wrote:
> nit: This is a shame; we really only want to use this implementation when
> running in the daemon, so we should be able to choose the impl to use.

Not true. We need SessionEventExecutor to switch between desktops to handle UAC.

> DesktopEnvironment was supposed to be an interface that lets the
implementations
> be chosen on a per-application basis, for this reason. :-/

I could drag this into event_executor_win.cc at the cost of passing
io_meesage_loop to each EventExecutor::Create instance. I don't think you want
that.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.cc:114: class SasInjectorVista : public
SasInjector {
On 2012/03/08 22:58:55, Wez wrote:
> We discussed removing Vista from this name in favour of Win (or WinImpl, as in
> the header).

You apparently missed my comment on the matter. I'm renaming it now to avoid
turning this into discussion about the color of the bike shed.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.h:5: #ifndef REMOTING_HOST_SAS_INJECTOR_WIN_H_
On 2012/03/08 22:58:55, Wez wrote:
> WinImpl

Well, this would be pretty ugly. I removed _win from the header's name.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
File remoting/host/session_event_executor_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:20: class MessageLoopProxy;
On 2012/03/08 22:58:55, Wez wrote:
> nit: Does the style guide require that there be blank lines around these
> forward-decls? If not then I'd suggest removing them to aid readability.

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
On 2012/03/08 22:58:55, Wez wrote:
> nit: SessionEventExecutorWin, to match the guard & filename?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
On 2012/03/08 22:58:55, Wez wrote:
> public EventExecutor -> public InputStub

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
On 2012/03/08 22:58:55, Wez wrote:
> I'm not convinced by the name; what is "session" about this class?  It's
really
> more a "daemon" event-injector, surely?

This class's responsibility is handling of session-specific events (and other
session-specific stuff such as switching between desktops).

The fact the "daemon" is involved is only an implementation detail.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:35: SessionEventExecutor(MessageLoop*
message_loop,
On 2012/03/08 22:58:55, Wez wrote:
> It's not clear what this class does with |message_loop|, or what happens to
> events that have been injected, posted to |message_loop|, but not yet
processed
> by the SessionEventExecutor, if it gets deleted?

It does the same thing as other EventExecutor classes.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:37: scoped_ptr<EventExecutor>
nested_executor);
On 2012/03/08 22:58:55, Wez wrote:
> scoped_ptr<EventExecutor> -> scoped_ptr<InputStub>

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:43: const char
kChromePipeNamePrefix[] = "\\\\.\\pipe\\chrome.";
On 2012/03/08 22:58:55, Wez wrote:
> nit: Comment this.

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:45: // Generatesthe command
line of the host process.
On 2012/03/08 22:58:55, Wez wrote:
> typo: Generates the ...

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:147: // Creates the server end
of the Chromoting IPC channel.
On 2012/03/08 22:58:55, Wez wrote:
> nit: Similar comment "based on IPC::Channel"?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:176: // IPC::Channel with
exception of passing a non-default security descriptor.
On 2012/03/08 22:58:55, Wez wrote:
> Not for this CL, but would adding support for this usage to IPC::Channel make
> sense?

Probably.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:275:
IPC::ChannelHandle(pipe.Get()),
On 2012/03/08 22:58:55, Wez wrote:
> nit: This indentation looks weird; can you put the items 4 bytes in from the
> start of |chromoting_channel_|?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:70: // Sends the security
attention sequence to the session represented by
On 2012/03/08 22:58:55, Wez wrote:
> nit: Secure Attention Sequence

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:86: // Pointer to the I/O
thread.
On 2012/03/08 22:58:55, Wez wrote:
> nit: I can see that from the naming.  What's it used for, though? ;)

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:114: // The server endpoint of
the chromoting IPC channel.
On 2012/03/08 22:58:55, Wez wrote:
> Is it relevant that it's the server endpoint? Would it be valid to describe it
> as the IPC channel to the per-session process?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.g...
remoting/remoting.gyp:419: [ 'OS=="win"', {
On 2012/03/08 22:58:55, Wez wrote:
> nit: Put this between OS=="mac" and OS!="win"?

Done.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.g...
remoting/remoting.gyp:421: '../ipc/ipc.gyp:ipc'
On 2012/03/08 22:58:55, Wez wrote:
> nit: It's a shame to have the plugin depend on IPC, since it will (currently)
> never use it. :(

I don't see a way to avoid it without major surgery. Life is tough.

[Wez, 2012-03-09 22:24:34]

LGTM but please see comments below.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
File remoting/host/desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:35: event_executor.Pass()));
On 2012/03/09 01:13:54, alexeypa wrote:
> On 2012/03/08 22:58:55, Wez wrote:
> > nit: This is a shame; we really only want to use this implementation when
> > running in the daemon, so we should be able to choose the impl to use.
> 
> Not true. We need SessionEventExecutor to switch between desktops to handle
UAC.

No we don't - we won't be running with sufficient privileges to switch desktops
to see UAC prompts, since they're privileged.

> > DesktopEnvironment was supposed to be an interface that lets the
> implementations
> > be chosen on a per-application basis, for this reason. :-/
> 
> I could drag this into event_executor_win.cc at the cost of passing
> io_meesage_loop to each EventExecutor::Create instance. I don't think you want
> that.

No, definitely not.  The idea is that DesktopEnvironment is just an interface
that each application running a ChromotingHost implements, with an
IT2MeDesktopEnvironment, and a Me2MeDesktopEnvironment, for example, that know
how to create the relevant application- and platform-specific classes.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.cc:114: class SasInjectorVista : public
SasInjector {
On 2012/03/09 01:13:54, alexeypa wrote:
> On 2012/03/08 22:58:55, Wez wrote:
> > We discussed removing Vista from this name in favour of Win (or WinImpl, as
in
> > the header).
> 
> You apparently missed my comment on the matter. I'm renaming it now to avoid
> turning this into discussion about the color of the bike shed.

No, I saw the comment, and replied roughly this:

"This implementation works for current and future versions of Windows, while the
XP-specific one is XP-specific, so naming then Win and XP makes perfect sense. 
It makes still more sense right now, when we have no XP implementation..."

I'm not sure why Rietveld doesn't have it. :(

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
File remoting/host/session_event_executor_win.h (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:32: class SessionEventExecutor :
public EventExecutor,
On 2012/03/09 01:13:54, alexeypa wrote:
> On 2012/03/08 22:58:55, Wez wrote:
> > I'm not convinced by the name; what is "session" about this class?  It's
> really
> > more a "daemon" event-injector, surely?
> 
> This class's responsibility is handling of session-specific events (and other
> session-specific stuff such as switching between desktops).
> 
> The fact the "daemon" is involved is only an implementation detail.

Except that this implementation only makes sense in the context of the daemon. 
That said, if UIAccess grants us rights to switch into the UAC desktop and/or
call SendSAS, then it that makes more sense.  The problem with "session" is that
it's used elsewhere in the remoting/ code - "WinSession" might be clearer?

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sessi...
remoting/host/session_event_executor_win.h:35: SessionEventExecutor(MessageLoop*
message_loop,
On 2012/03/09 01:13:54, alexeypa wrote:
> On 2012/03/08 22:58:55, Wez wrote:
> > It's not clear what this class does with |message_loop|, or what happens to
> > events that have been injected, posted to |message_loop|, but not yet
> processed
> > by the SessionEventExecutor, if it gets deleted?
> 
> It does the same thing as other EventExecutor classes.

Yes, I know that.  My point is that it's not at all obvious what that is,
without a comment on the constructor to explain, or even to refer the reader to
EventExecutor::Create() (although that doesn't document the behaviour either).

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/remoting.g...
remoting/remoting.gyp:421: '../ipc/ipc.gyp:ipc'
On 2012/03/09 01:13:54, alexeypa wrote:
> On 2012/03/08 22:58:55, Wez wrote:
> > nit: It's a shame to have the plugin depend on IPC, since it will
(currently)
> > never use it. :(
> 
> I don't see a way to avoid it without major surgery. Life is tough.

Yeah, I think we'd need to fix up DesktopEnvironment to be a pure interface, so
that the plugin no longer uses the SessionEventExecutor on Windows.

[alexeypa (please no reviews), 2012-03-10 18:08:52]

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
File remoting/host/desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/deskt...
remoting/host/desktop_environment.cc:35: event_executor.Pass()));
On 2012/03/09 22:24:34, Wez wrote:
> No we don't - we won't be running with sufficient privileges to switch
desktops
> to see UAC prompts, since they're privileged.

Yes, you are right. we should think about that scenario then. It is It2Me so UAC
prompts will happen. The good thing is that there will be a person to click on
them, but it would be nice just to specify level of access when sharing the
screen. Something like: a user shares the screen and optionally click on the
elevation button. It shows UAC prompt and if the user agrees the host will be
elevated and the remote user will be able to see other desktops.

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
File remoting/host/sas_injector_win.cc (right):

https://chromiumcodereview.appspot.com/9617027/diff/31001/remoting/host/sas_i...
remoting/host/sas_injector_win.cc:114: class SasInjectorVista : public
SasInjector {
> I'm not sure why Rietveld doesn't have it. :(

Strange. Anyway, it is done now.

[commit-bot: I haz the power, 2012-03-12 16:36:27]

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

[commit-bot: I haz the power, 2012-03-12 17:31:23]

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

[commit-bot: I haz the power, 2012-03-12 19:23:09]

Change committed as 126186