Handle Security Key requests from outside the remoted session correctly

remoting/host/security_key/security_key_ipc_client.cc

 // Copyright 2016 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/security_key/security_key_ipc_client.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 20 matching lines @@
 bool SecurityKeyIpcClient::CheckForSecurityKeyIpcServerChannel() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!channel_handle_.is_valid()) {
     channel_handle_ = mojo::edk::CreateClientHandle(named_channel_handle_);
   }
   return channel_handle_.is_valid();
 }
 
 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();
 }
 
 bool SecurityKeyIpcClient::SendSecurityKeyRequest(
     const std::string& request_payload,
     const ResponseCallback& response_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!request_payload.empty());
@@ skipping 30 matching lines @@
   expected_ipc_server_session_id_ = expected_session_id;
 }
 
 bool SecurityKeyIpcClient::OnMessageReceived(const IPC::Message& message) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool handled = true;
   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()
 
   CHECK(handled) << "Received unexpected IPC type: " << message.type();
   return handled;
 }
 
 void SecurityKeyIpcClient::OnChannelConnected(int32_t peer_pid) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
 #if defined(OS_WIN)
   DWORD peer_session_id;
   if (!ProcessIdToSessionId(peer_pid, &peer_session_id)) {
     PLOG(ERROR) << "ProcessIdToSessionId failed";
     base::ResetAndReturn(&connection_error_callback_).Run();
     return;
   }
 
   if (peer_session_id != expected_ipc_server_session_id_) {
     LOG(ERROR)
         << "Cannot establish connection with IPC server running in session: "
         << peer_session_id;
     base::ResetAndReturn(&connection_error_callback_).Run();
     return;
   }
 #endif  // defined(OS_WIN)
-
-  base::ResetAndReturn(&connection_ready_callback_).Run();
 }
 
 void SecurityKeyIpcClient::OnChannelError() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!connection_error_callback_.is_null()) {
     base::ResetAndReturn(&connection_error_callback_).Run();
   }
 }
 
 void SecurityKeyIpcClient::OnSecurityKeyResponse(
     const std::string& response_data) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!connection_error_callback_.is_null());
 
   if (!response_data.empty()) {
     base::ResetAndReturn(&response_callback_).Run(response_data);
   } else {
     LOG(ERROR) << "Invalid response received";
     base::ResetAndReturn(&connection_error_callback_).Run();
   }
 }
 
+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());
 
   // Verify that any existing IPC connection has been closed.
   CloseIpcConnection();
 
   if (!channel_handle_.is_valid() && !CheckForSecurityKeyIpcServerChannel()) {
     if (!connection_error_callback_.is_null()) {
       base::ResetAndReturn(&connection_error_callback_).Run();
     }
     return;
   }
 
   ipc_channel_ = IPC::Channel::CreateClient(
       mojo::edk::ConnectToPeerProcess(std::move(channel_handle_)).release(),
       this);
   if (ipc_channel_->Connect()) {
     return;
   }
   ipc_channel_.reset();
 
   if (!connection_error_callback_.is_null()) {
     base::ResetAndReturn(&connection_error_callback_).Run();
   }
 }
 
 }  // namespace remoting