Handle Security Key requests from outside the remoted session correctly

Handle Security Key requests from outside the remoted session correctly

The recent Mojo conversion changed the communication pattern between the
IPC Server and Client classes used for remote security key message
forwarding.  This change has caused a regression in the scenario where
the security key request originates from outside the remoted session.
The desired behavior is to pass a message to the local SK handler so
it can route/handle the request.  Due to the change in the following
CL, we would instead report that the remote client would handle it,
then return a failure when they sent the request:
https://codereview.chromium.org/2478443002/

The reason for this change is that the session detection logic was wrapped
in the OnChannelConnected() method.  Because you cannot close an IPC channel
from within that method, we would defer the close operation.  With the old
architecture, the client would be waiting for a specific IPC message to
complete the connection (and signal success) but instead would receive
the deferred close/error message and pass on the right message to the local
handler.  This behavior was changed such that the client would now signal
success in its own 'OnChannelConnected()' and then receive the deferred
error.

In order to make this scenario more robust, I have added specific success
and failure IPC messages to be passed to the client from the server and
will use that when establishing the connection.  I've also separated out
the concept of an invalid session vs. and actual connection error so the
IPC client class can provide more accurate details to the local handler.

BUG=674236
Committed: https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58
Cr-Commit-Position: refs/heads/master@{#439685}

[joedow, 2016-12-14 19:43:06]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 19:43:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 19:57:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 19:57:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-12-14 20:49:25]

Patchset #2 (id:20001) has been deleted

[joedow, 2016-12-14 20:51:00]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 20:52:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 22:04:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:04:23]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-12-14 22:22:05]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2016-12-14 22:22:06]

PTAL!

[Sergey Ulanov, 2016-12-15 19:47:33]

lgtm, but see my comment regarding naming.

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.h (right):

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.h:38: typedef
base::Callback<void(bool connection_ready)> ConnectionReadyCallback;
It seems wrong to call it ConnectionReadyCallback while it has boolean
connection_ready argument. I suggest renaming the callback, or parameter or
both. Maybe call it ConnectedCallback(bool usable)?

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client_unittest.cc (right):

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client_unittest.cc:97: // used to
drive invalid session behavior for testing the client.
nit: uppercase U

[joedow, 2016-12-15 21:17:00]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 21:17:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-12-15 21:18:32]

joedow@chromium.org changed reviewers:
+ dcheng@chromium.org

[joedow, 2016-12-15 21:18:33]

Thanks Sergey!

Adding dcheng for chromoting_messages.h approval.

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.h (right):

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.h:38: typedef
base::Callback<void(bool connection_ready)> ConnectionReadyCallback;
On 2016/12/15 19:47:33, Sergey Ulanov wrote:
> It seems wrong to call it ConnectionReadyCallback while it has boolean
> connection_ready argument. I suggest renaming the callback, or parameter or
> both. Maybe call it ConnectedCallback(bool usable)?

Done.

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client_unittest.cc (right):

https://codereview.chromium.org/2575963002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client_unittest.cc:97: // used to
drive invalid session behavior for testing the client.
On 2016/12/15 19:47:33, Sergey Ulanov wrote:
> nit: uppercase U

Done.

[commit-bot: I haz the power, 2016-12-15 21:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 21:45:58]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-16 02:33:07]

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:154:
DCHECK(!connected_callback_.is_null());
Just to double-check: is the server end (the one sending us messages) considered
more privileged, and therefore trusted? What do server and client mean in this
context?

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.h (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.h:38: typedef
base::Callback<void(bool connection_usable)> ConnectedCallback;
Nit: it's preferred to use 'using A = B' over 'typedef B A'

[joedow, 2016-12-16 03:36:22]

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:154:
DCHECK(!connected_callback_.is_null());
On 2016/12/16 02:33:07, dcheng wrote:
> Just to double-check: is the server end (the one sending us messages)
considered
> more privileged, and therefore trusted? What do server and client mean in this
> context?

The server in this context is our network process (running in session 0) which
owns a well-known IPC(mojo) channel that the client connects to.  The client
forwards a request from a local security key process (which launches the
component that hosts this class and communicates via STDIO) over this channel to
the 'server' component running in our network process which then forwards the
request to the remote machine to service.  The response is then returned back to
local sk process in reverse order.

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.h (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.h:38: typedef
base::Callback<void(bool connection_usable)> ConnectedCallback;
On 2016/12/16 02:33:07, dcheng wrote:
> Nit: it's preferred to use 'using A = B' over 'typedef B A'

Acknowledged.  I'll switch to that style in future CLs.

[dcheng, 2016-12-16 09:12:03]

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:154:
DCHECK(!connected_callback_.is_null());
On 2016/12/16 03:36:22, joedow wrote:
> On 2016/12/16 02:33:07, dcheng wrote:
> > Just to double-check: is the server end (the one sending us messages)
> considered
> > more privileged, and therefore trusted? What do server and client mean in
this
> > context?
> 
> The server in this context is our network process (running in session 0) which
> owns a well-known IPC(mojo) channel that the client connects to.  The client
> forwards a request from a local security key process (which launches the
> component that hosts this class and communicates via STDIO) over this channel
to
> the 'server' component running in our network process which then forwards the
> request to the remote machine to service.  The response is then returned back
to
> local sk process in reverse order.
> 

It's hard for me to tell if the network service is more or less trusted. Since
the network process is brokering this access and running in Session 0, I feel
like I should be able to assume that it does sufficient validation to insure
that it never sends IPCs in a way that would violate invariants (such as the
DCHECK here that the callbacks are appropriately set and are not called twice).

However, if it's the network process, and it's communicating with remote
machines... is it doing a lot of parsing? Is it something that could be
compromised?

SOrry for all the questions, I'm just trying to understand how the pieces here
fit together.

[joedow, 2016-12-16 16:20:18]

In addition to the response, I've sent you my design doc if you'd like to get a
high level overview and additional details of the system.

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:154:
DCHECK(!connected_callback_.is_null());
On 2016/12/16 09:12:03, dcheng wrote:
> On 2016/12/16 03:36:22, joedow wrote:
> > On 2016/12/16 02:33:07, dcheng wrote:
> > > Just to double-check: is the server end (the one sending us messages)
> > considered
> > > more privileged, and therefore trusted? What do server and client mean in
> this
> > > context?
> > 
> > The server in this context is our network process (running in session 0)
which
> > owns a well-known IPC(mojo) channel that the client connects to.  The client
> > forwards a request from a local security key process (which launches the
> > component that hosts this class and communicates via STDIO) over this
channel
> to
> > the 'server' component running in our network process which then forwards
the
> > request to the remote machine to service.  The response is then returned
back
> to
> > local sk process in reverse order.
> > 
> 
> It's hard for me to tell if the network service is more or less trusted. Since
> the network process is brokering this access and running in Session 0, I feel
> like I should be able to assume that it does sufficient validation to insure
> that it never sends IPCs in a way that would violate invariants (such as the
> DCHECK here that the callbacks are appropriately set and are not called
twice).
> 
> However, if it's the network process, and it's communicating with remote
> machines... is it doing a lot of parsing? Is it something that could be
> compromised?
> 
> SOrry for all the questions, I'm just trying to understand how the pieces here
> fit together.

The network process is a low-integrity process.  It communicates with the and
essentially forwards the SK request bytes to the remote machine w/o
interpretation.  This behavior has existed since M52, however the recent Mojo
work changed the initial connection pattern and broke this scenario (where a CRD
connection is active but not for the user's current session).  The other aspects
of the system should remain the same.

The previous connection behavior relied on the IPC channel creating a new,
private channel and sending that connection info to the sk forwarding process
(i.e. remote_security_key).  The mojo changes removed this extra step and
replaced it was a different mechanism to allow multiple connections, however we
want to enforce a rule that SK requests must originate from the currently
remoted session.  Using the IPC messages is less error prone (IMO) than relying
on the reception of a specific message or the IPC channel being closed (which
could happen for any reason).

[dcheng, 2016-12-17 01:06:09]

On 2016/12/16 16:20:18, joedow wrote:
> In addition to the response, I've sent you my design doc if you'd like to get
a
> high level overview and additional details of the system.
> 
>
https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
> File remoting/host/security_key/security_key_ipc_client.cc (right):
> 
>
https://codereview.chromium.org/2575963002/diff/60001/remoting/host/security_...
> remoting/host/security_key/security_key_ipc_client.cc:154:
> DCHECK(!connected_callback_.is_null());
> On 2016/12/16 09:12:03, dcheng wrote:
> > On 2016/12/16 03:36:22, joedow wrote:
> > > On 2016/12/16 02:33:07, dcheng wrote:
> > > > Just to double-check: is the server end (the one sending us messages)
> > > considered
> > > > more privileged, and therefore trusted? What do server and client mean
in
> > this
> > > > context?
> > > 
> > > The server in this context is our network process (running in session 0)
> which
> > > owns a well-known IPC(mojo) channel that the client connects to.  The
client
> > > forwards a request from a local security key process (which launches the
> > > component that hosts this class and communicates via STDIO) over this
> channel
> > to
> > > the 'server' component running in our network process which then forwards
> the
> > > request to the remote machine to service.  The response is then returned
> back
> > to
> > > local sk process in reverse order.
> > > 
> > 
> > It's hard for me to tell if the network service is more or less trusted.
Since
> > the network process is brokering this access and running in Session 0, I
feel
> > like I should be able to assume that it does sufficient validation to insure
> > that it never sends IPCs in a way that would violate invariants (such as the
> > DCHECK here that the callbacks are appropriately set and are not called
> twice).
> > 
> > However, if it's the network process, and it's communicating with remote
> > machines... is it doing a lot of parsing? Is it something that could be
> > compromised?
> > 
> > SOrry for all the questions, I'm just trying to understand how the pieces
here
> > fit together.
> 
> The network process is a low-integrity process.  It communicates with the and
> essentially forwards the SK request bytes to the remote machine w/o
> interpretation.  This behavior has existed since M52, however the recent Mojo
> work changed the initial connection pattern and broke this scenario (where a
CRD
> connection is active but not for the user's current session).  The other
aspects
> of the system should remain the same.
> 
> The previous connection behavior relied on the IPC channel creating a new,
> private channel and sending that connection info to the sk forwarding process
> (i.e. remote_security_key).  The mojo changes removed this extra step and
> replaced it was a different mechanism to allow multiple connections, however
we
> want to enforce a rule that SK requests must originate from the currently
> remoted session.  Using the IPC messages is less error prone (IMO) than
relying
> on the reception of a specific message or the IPC channel being closed (which
> could happen for any reason).

OK, thanks, this is pretty useful information to know. Some followup questions:

1) Is this documented somewhere in the code? =) For security reviewers, it's
super helpful to understand which processes are trusted, which are not, and what
sort of inputs they're handling. The design doc was pretty helpful, but perhaps
could do a bit more to highlight the relative trust between processes.

2) Since the network process is low-integrity, I assume it's expected to a point
of potential compromise. Given that, is it really OK to DCHECK() that callbacks
are set?

(It might be faster to talk in person, if you want to chat sometime on Monday)

[joedow, 2016-12-18 20:52:14]

Let's meet up Monday and discuss.  This is a fix for a regression that occurred
in M56 and I'd like to get it corrected in that branch before everyone leaves
for the holidays.

[joedow, 2016-12-20 00:25:30]

Discussed architecture offline with dcheng.  Can I get approval for the
chromoting_messages.h file?

Thanks!
Joe

[dcheng, 2016-12-20 00:31:46]

This CL LGTM.

However, please file a followup bug to remove the DCHECKs and have the client
side protect against incorrect IPC ordering. The network process (which hosts
the ipc server) is low integrity, which indicates we sort of don't trust it
(even if it is running as a service), while the other end (the ipc client,
running in the desktop session), is not.

[joedow, 2016-12-20 02:27:32]

Sounds good.  I've file 675818 to track the client change, will try to get that
done before the end of the year.

[joedow, 2016-12-20 02:27:44]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-12-20 02:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2575963002/#ps60001
(title: "Addressing CR Feedback")

[commit-bot: I haz the power, 2016-12-20 02:27:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 03:01:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482200864124710,
"parent_rev": "dfedccba3cb103c3bea0939fafb2ab39f93e4960", "commit_rev":
"c12ccfe11396646fffdfd11c89607f65182902ff"}

[commit-bot: I haz the power, 2016-12-20 03:02:52]

Description was changed from

==========
Handle Security Key requests from outside the remoted session correctly

The recent Mojo conversion changed the communication pattern between the
IPC Server and Client classes used for remote security key message
forwarding.  This change has caused a regression in the scenario where
the security key request originates from outside the remoted session.
The desired behavior is to pass a message to the local SK handler so
it can route/handle the request.  Due to the change in the following
CL, we would instead report that the remote client would handle it,
then return a failure when they sent the request:
https://codereview.chromium.org/2478443002/

The reason for this change is that the session detection logic was wrapped
in the OnChannelConnected() method.  Because you cannot close an IPC channel
from within that method, we would defer the close operation.  With the old
architecture, the client would be waiting for a specific IPC message to
complete the connection (and signal success) but instead would receive
the deferred close/error message and pass on the right message to the local
handler.  This behavior was changed such that the client would now signal
success in its own 'OnChannelConnected()' and then receive the deferred
error.

In order to make this scenario more robust, I have added specific success
and failure IPC messages to be passed to the client from the server and
will use that when establishing the connection.  I've also separated out
the concept of an invalid session vs. and actual connection error so the
IPC client class can provide more accurate details to the local handler.

BUG=674236
==========

to

==========
Handle Security Key requests from outside the remoted session correctly

The recent Mojo conversion changed the communication pattern between the
IPC Server and Client classes used for remote security key message
forwarding.  This change has caused a regression in the scenario where
the security key request originates from outside the remoted session.
The desired behavior is to pass a message to the local SK handler so
it can route/handle the request.  Due to the change in the following
CL, we would instead report that the remote client would handle it,
then return a failure when they sent the request:
https://codereview.chromium.org/2478443002/

The reason for this change is that the session detection logic was wrapped
in the OnChannelConnected() method.  Because you cannot close an IPC channel
from within that method, we would defer the close operation.  With the old
architecture, the client would be waiting for a specific IPC message to
complete the connection (and signal success) but instead would receive
the deferred close/error message and pass on the right message to the local
handler.  This behavior was changed such that the client would now signal
success in its own 'OnChannelConnected()' and then receive the deferred
error.

In order to make this scenario more robust, I have added specific success
and failure IPC messages to be passed to the client from the server and
will use that when establishing the connection.  I've also separated out
the concept of an invalid session vs. and actual connection error so the
IPC client class can provide more accurate details to the local handler.

BUG=674236

Review-Url: https://codereview.chromium.org/2575963002
Committed:
https://chromium.googlesource.com/chromium/src/+/c12ccfe11396646fffdfd11c8960...
==========

[commit-bot: I haz the power, 2016-12-20 03:02:54]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c12ccfe11396646fffdfd11c8960...

[commit-bot: I haz the power, 2016-12-20 03:05:45]

Description was changed from

==========
Handle Security Key requests from outside the remoted session correctly

The recent Mojo conversion changed the communication pattern between the
IPC Server and Client classes used for remote security key message
forwarding.  This change has caused a regression in the scenario where
the security key request originates from outside the remoted session.
The desired behavior is to pass a message to the local SK handler so
it can route/handle the request.  Due to the change in the following
CL, we would instead report that the remote client would handle it,
then return a failure when they sent the request:
https://codereview.chromium.org/2478443002/

The reason for this change is that the session detection logic was wrapped
in the OnChannelConnected() method.  Because you cannot close an IPC channel
from within that method, we would defer the close operation.  With the old
architecture, the client would be waiting for a specific IPC message to
complete the connection (and signal success) but instead would receive
the deferred close/error message and pass on the right message to the local
handler.  This behavior was changed such that the client would now signal
success in its own 'OnChannelConnected()' and then receive the deferred
error.

In order to make this scenario more robust, I have added specific success
and failure IPC messages to be passed to the client from the server and
will use that when establishing the connection.  I've also separated out
the concept of an invalid session vs. and actual connection error so the
IPC client class can provide more accurate details to the local handler.

BUG=674236

Review-Url: https://codereview.chromium.org/2575963002
Committed:
https://chromium.googlesource.com/chromium/src/+/c12ccfe11396646fffdfd11c8960...
==========

to

==========
Handle Security Key requests from outside the remoted session correctly

The recent Mojo conversion changed the communication pattern between the
IPC Server and Client classes used for remote security key message
forwarding.  This change has caused a regression in the scenario where
the security key request originates from outside the remoted session.
The desired behavior is to pass a message to the local SK handler so
it can route/handle the request.  Due to the change in the following
CL, we would instead report that the remote client would handle it,
then return a failure when they sent the request:
https://codereview.chromium.org/2478443002/

The reason for this change is that the session detection logic was wrapped
in the OnChannelConnected() method.  Because you cannot close an IPC channel
from within that method, we would defer the close operation.  With the old
architecture, the client would be waiting for a specific IPC message to
complete the connection (and signal success) but instead would receive
the deferred close/error message and pass on the right message to the local
handler.  This behavior was changed such that the client would now signal
success in its own 'OnChannelConnected()' and then receive the deferred
error.

In order to make this scenario more robust, I have added specific success
and failure IPC messages to be passed to the client from the server and
will use that when establishing the connection.  I've also separated out
the concept of an invalid session vs. and actual connection error so the
IPC client class can provide more accurate details to the local handler.

BUG=674236

Committed: https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58
Cr-Commit-Position: refs/heads/master@{#439685}
==========

[commit-bot: I haz the power, 2016-12-20 03:05:46]

Patchset 3 (id:??) landed as
https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58
Cr-Commit-Position: refs/heads/master@{#439685}