Handle Security Key requests from outside the remoted session correctly

remoting/host/security_key/security_key_ipc_client.cc

Index: remoting/host/security_key/security_key_ipc_client.cc
diff --git a/remoting/host/security_key/security_key_ipc_client.cc b/remoting/host/security_key/security_key_ipc_client.cc
index 43ad72f17b8e735bbd61efd24a502e1f3e6c7e6d..994f92ef231005319d07bba22c8a3451d94f294a 100644
--- a/remoting/host/security_key/security_key_ipc_client.cc
+++ b/remoting/host/security_key/security_key_ipc_client.cc
@@ -38,14 +38,14 @@ bool SecurityKeyIpcClient::CheckForSecurityKeyIpcServerChannel() {
 }
 
 void SecurityKeyIpcClient::EstablishIpcConnection(
-    const base::Closure& connection_ready_callback,
+    const ConnectedCallback& connected_callback,
     const base::Closure& connection_error_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!connection_ready_callback.is_null());
+  DCHECK(!connected_callback.is_null());
   DCHECK(!connection_error_callback.is_null());
   DCHECK(!ipc_channel_);
 
-  connection_ready_callback_ = connection_ready_callback;
+  connected_callback_ = connected_callback;
   connection_error_callback_ = connection_error_callback;
 
   ConnectToIpcChannel();
@@ -96,6 +96,10 @@ bool SecurityKeyIpcClient::OnMessageReceived(const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(SecurityKeyIpcClient, message)
     IPC_MESSAGE_HANDLER(ChromotingNetworkToRemoteSecurityKeyMsg_Response,
                         OnSecurityKeyResponse)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkToRemoteSecurityKeyMsg_ConnectionReady,
+                        OnConnectionReady)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkToRemoteSecurityKeyMsg_InvalidSession,
+                        OnInvalidSession)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
@@ -122,8 +126,6 @@ void SecurityKeyIpcClient::OnChannelConnected(int32_t peer_pid) {
     return;
   }
 #endif  // defined(OS_WIN)
-
-  base::ResetAndReturn(&connection_ready_callback_).Run();
 }
 
 void SecurityKeyIpcClient::OnChannelError() {
@@ -147,6 +149,20 @@ void SecurityKeyIpcClient::OnSecurityKeyResponse(
   }
 }
 
+void SecurityKeyIpcClient::OnConnectionReady() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!connected_callback_.is_null());

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

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?

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

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.

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

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]

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).

+
+  base::ResetAndReturn(&connected_callback_).Run(/*connection_usable=*/true);
+}
+
+void SecurityKeyIpcClient::OnInvalidSession() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!connected_callback_.is_null());
+
+  base::ResetAndReturn(&connected_callback_).Run(/*connection_usable=*/false);
+}
+
 void SecurityKeyIpcClient::ConnectToIpcChannel() {
   DCHECK(thread_checker_.CalledOnValidThread());