Non-SFI: move socketpair() from plugin process to browser process.

components/nacl/browser/nacl_process_host.cc

 // Copyright (c) 2012 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 "components/nacl/browser/nacl_process_host.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 654 matching lines @@
   }
 #endif
   process_->Launch(
       new NaClSandboxedProcessLauncherDelegate(process_->GetHost()),
       cmd_line.release(),
       true);
   return true;
 }
 
 bool NaClProcessHost::OnMessageReceived(const IPC::Message& msg) {
+  if (uses_nonsfi_mode_) {
+    // In Non-SFI mode, no message is expected.
+    return false;
+  }
+
   bool handled = true;
-  if (uses_nonsfi_mode_) {
-    // IPC messages relating to NaCl's validation cache must not be exposed

[Mark Seaborn, 2015/05/12 23:24:06]

Please keep this comment, and add it after "// In Non-SFI mode, no message is
expected."

[hidehiko, 2015/05/13 06:39:20]

Done.

-    // in Non-SFI Mode, otherwise a Non-SFI nexe could use
-    // SetKnownToValidate to create a hole in the SFI sandbox.
-    IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
-                          OnPpapiChannelsCreated)
-      IPC_MESSAGE_UNHANDLED(handled = false)
-    IPC_END_MESSAGE_MAP()
-  } else {
-    IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
-                          OnQueryKnownToValidate)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
-                          OnSetKnownToValidate)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
-                          OnResolveFileToken)
+  IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
+                        OnQueryKnownToValidate)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
+                        OnSetKnownToValidate)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
+                        OnResolveFileToken)
 
 #if defined(OS_WIN)
-      IPC_MESSAGE_HANDLER_DELAY_REPLY(
-          NaClProcessMsg_AttachDebugExceptionHandler,
-          OnAttachDebugExceptionHandler)
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected,
-                          OnDebugStubPortSelected)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(
+        NaClProcessMsg_AttachDebugExceptionHandler,
+        OnAttachDebugExceptionHandler)
+    IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected,
+                        OnDebugStubPortSelected)
 #endif
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
-                          OnPpapiChannelsCreated)
-      IPC_MESSAGE_UNHANDLED(handled = false)
-    IPC_END_MESSAGE_MAP()
-  }
+    IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
+                        OnPpapiChannelsCreated)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void NaClProcessHost::OnProcessLaunched() {
   if (!StartWithLaunchedProcess())
     delete this;
 }
 
 // Called when the NaClBrowser singleton has been fully initialized.
 void NaClProcessHost::OnResourcesReady() {
@@ skipping 149 matching lines @@
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
     // In non-SFI mode, we do not use SRPC. Make sure that the socketpair is
     // not created.
     DCHECK(!socket_for_sel_ldr_.IsValid());
 #endif
     if (enable_nacl_debug) {
       base::ProcessId pid = base::GetProcId(process_->GetData().handle);
       LOG(WARNING) << "nonsfi nacl plugin running in " << pid;
     }
+
+    DCHECK(params.enable_ipc_proxy);
+    if (!params.enable_ipc_proxy) {

[Mark Seaborn, 2015/05/12 23:24:06]

This is basically input validation of what the renderer passed to the browser
process.  It would be better to do this earlier, in Launch().

[hidehiko, 2015/05/13 06:39:20]

Done.

+      LOG(ERROR) << "In Non-SFI mode, PPAPI proxy must be enabled.";
+      return false;
+    }
   } else {
     params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
     params.validation_cache_key = nacl_browser->GetValidationCacheKey();
     params.version = NaClBrowser::GetDelegate()->GetVersionString();
     params.enable_debug_stub = enable_nacl_debug;
 
     const ChildProcessData& data = process_->GetData();
     params.imc_bootstrap_handle =
         IPC::TakeFileHandleForProcess(socket_for_sel_ldr_.Pass(), data.handle);
     if (params.imc_bootstrap_handle == IPC::InvalidPlatformFileForTransit()) {
@@ skipping 83 matching lines @@
                          true /* is_executable */),
               base::Bind(&NaClProcessHost::StartNaClFileResolved,
                          weak_factory_.GetWeakPtr(),
                          params,
                          file_path))) {
         return true;
       }
     }
   }
 
-  params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
-                                                   process_->GetData().handle);
-  process_->Send(new NaClProcessMsg_Start(params));
+  StartNaClFileResolved(params, base::FilePath(), base::File());
   return true;
 }
 
 void NaClProcessHost::StartNaClFileResolved(
     NaClStartParams params,
     const base::FilePath& file_path,
     base::File checked_nexe_file) {
   if (checked_nexe_file.IsValid()) {
     // Release the file received from the renderer. This has to be done on a
     // thread where IO is permitted, though.
     content::BrowserThread::GetBlockingPool()->PostTask(
         FROM_HERE,
         base::Bind(&CloseFile, base::Passed(nexe_file_.Pass())));
     params.nexe_file_path_metadata = file_path;
     params.nexe_file = IPC::TakeFileHandleForProcess(
         checked_nexe_file.Pass(), process_->GetData().handle);
   } else {
     params.nexe_file = IPC::TakeFileHandleForProcess(
         nexe_file_.Pass(), process_->GetData().handle);
   }
+
+#if defined(OS_LINUX)
+  // In Non-SFI mode, create socket pairs for IPC channels, here, unlike in

[Mark Seaborn, 2015/05/12 23:24:06]

Grammar nit: could remove comma before "here"

[hidehiko, 2015/05/13 06:39:20]

Done.

+  // SFI-mode, in which those channels are created in nacl_listener.cc.
+  // This is for security hardening. We can then prohibit the socketpair()
+  // system call in nacl_helper and nacl_helper_nonsfi.
+  if (uses_nonsfi_mode_) {
+    // Note: here, because some resources for the plugin process are already

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: "plugin process" -> "NaCl loader process", x2

Also "resources" -> "FDs" to be more specific

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // opened, they must be sent to plugin process because these cannot be

[Mark Seaborn, 2015/05/12 23:24:06]

"cannot be closed in this process" could be misleading for someone who doesn't
understand the background.  (They *could* be closed on Unix, but making the same
logic work on Windows adds further complication.)

A better explanation might be something like "...are transferred to NaCl loader
process even if an error occurs first, because this is the simplest way to
ensure that these FDs don't get leaked and that the NaCl loader process will
exit properly".

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // closed in this process properly.
+    bool has_error = false;
+
+    // The case this condition is not satisfied should be handled in

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: "The case where this condition..." perhaps?  Or you could say: "Note that
this check is redundant; we check this earlier".

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // StartNaClExecution().
+    DCHECK(params.enable_ipc_proxy);
+
+    ScopedChannelHandle ppapi_browser_server_channel_handle;

[Mark Seaborn, 2015/05/12 23:24:06]

I wonder if it would it be worth doing:

typedef std::pair<ScopedChannelHandle, ScopedChannelHandle> ChannelHandlePair;
bool CreateChannelHandlePair(ChannelHandlePair* pair);

That would make this a bit less repetitive.

[hidehiko, 2015/05/13 06:39:20]

I do not want to introduce such a pair. Indeed it removes 8 lines from here, but
instead we need to distribute .first and .second.

+    ScopedChannelHandle ppapi_browser_client_channel_handle;
+    ScopedChannelHandle ppapi_renderer_server_channel_handle;
+    ScopedChannelHandle ppapi_renderer_client_channel_handle;
+    ScopedChannelHandle trusted_service_server_channel_handle;
+    ScopedChannelHandle trusted_service_client_channel_handle;
+    ScopedChannelHandle manifest_service_server_channel_handle;
+    ScopedChannelHandle manifest_service_client_channel_handle;
+
+    if (!CreateChannelHandlePair(&ppapi_browser_server_channel_handle,
+                                 &ppapi_browser_client_channel_handle) ||
+        !CreateChannelHandlePair(&ppapi_renderer_server_channel_handle,
+                                 &ppapi_renderer_client_channel_handle) ||
+        !CreateChannelHandlePair(&trusted_service_server_channel_handle,
+                                 &trusted_service_client_channel_handle) ||
+        !CreateChannelHandlePair(&manifest_service_server_channel_handle,
+                                 &manifest_service_client_channel_handle)) {
+      SendErrorToRenderer("Failed to create socket pairs.");
+      has_error = true;
+    }
+
+    if (!has_error &&
+        !StartPPAPIProxy(ppapi_browser_client_channel_handle.Pass())) {
+      SendErrorToRenderer("Failed to start browser PPAPI proxy.");
+      has_error = true;
+    }
+
+    if (!has_error) {
+      // On success, send back a success message to the renderer process,
+      // and, transfer the channel handles for the plugin process to |params|.

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: remove comma after "and"

Also "plugin process" -> "NaCl loader process"

However, isn't this comment just echoing the code below?  You could omit it.

[hidehiko, 2015/05/13 06:39:20]

Done.

+      ReplyToRenderer(ppapi_renderer_client_channel_handle.Pass(),
+                      trusted_service_client_channel_handle.Pass(),
+                      manifest_service_client_channel_handle.Pass());
+      params.ppapi_browser_channel_handle =
+          ppapi_browser_server_channel_handle.release();
+      params.ppapi_renderer_channel_handle =
+          ppapi_renderer_server_channel_handle.release();
+      params.trusted_service_channel_handle =
+          trusted_service_server_channel_handle.release();
+      params.manifest_service_channel_handle =
+          manifest_service_server_channel_handle.release();
+    }
+  }
+#endif
+
   process_->Send(new NaClProcessMsg_Start(params));
 }
 
-// This method is called when NaClProcessHostMsg_PpapiChannelCreated is
-// received.
-void NaClProcessHost::OnPpapiChannelsCreated(
-    const IPC::ChannelHandle& raw_browser_channel_handle,
-    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
-    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
-    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
-  ScopedChannelHandle browser_channel_handle(raw_browser_channel_handle);
-  ScopedChannelHandle ppapi_renderer_channel_handle(
-      raw_ppapi_renderer_channel_handle);
-  ScopedChannelHandle trusted_renderer_channel_handle(
-      raw_trusted_renderer_channel_handle);
-  ScopedChannelHandle manifest_service_channel_handle(
-      raw_manifest_service_channel_handle);
+#if defined(OS_LINUX)
+// static
+bool NaClProcessHost::CreateChannelHandlePair(
+    ScopedChannelHandle* channel_handle1,
+    ScopedChannelHandle* channel_handle2) {
+  DCHECK(channel_handle1);
+  DCHECK(channel_handle2);
 
-  if (!enable_ppapi_proxy()) {
-    ReplyToRenderer(ScopedChannelHandle(),
-                    trusted_renderer_channel_handle.Pass(),
-                    manifest_service_channel_handle.Pass());
-    return;
+  int fd1 = -1;
+  int fd2 = -1;
+  if (!IPC::SocketPair(&fd1, &fd2)) {
+    return false;
   }
 
+  IPC::ChannelHandle handle = IPC::Channel::GenerateVerifiedChannelID("nacl");
+  handle.socket = base::FileDescriptor(fd1, true);
+  channel_handle1->reset(handle);
+  handle.socket = base::FileDescriptor(fd2, true);
+  channel_handle2->reset(handle);
+  return true;
+}
+#endif
+
+bool NaClProcessHost::StartPPAPIProxy(ScopedChannelHandle channel_handle) {
   if (ipc_proxy_channel_.get()) {
     // Attempt to open more than 1 browser channel is not supported.
     // Shut down the NaCl process.
     process_->GetHost()->ForceShutdown();
-    return;
+    return false;
   }
 
   DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
 
-  ipc_proxy_channel_ =
-      IPC::ChannelProxy::Create(browser_channel_handle.release(),
-                                IPC::Channel::MODE_CLIENT,
-                                NULL,
-                                base::MessageLoopProxy::current().get());
+  ipc_proxy_channel_ = IPC::ChannelProxy::Create(
+      channel_handle.release(),
+      IPC::Channel::MODE_CLIENT,
+      NULL,
+      base::MessageLoopProxy::current().get());
   // Create the browser ppapi host and enable PPAPI message dispatching to the
   // browser process.
   ppapi_host_.reset(content::BrowserPpapiHost::CreateExternalPluginProcess(
       ipc_proxy_channel_.get(),  // sender
       permissions_,
       process_->GetData().handle,
       ipc_proxy_channel_.get(),
       nacl_host_message_filter_->render_process_id(),
       render_view_id_,
       profile_directory_));
@@ skipping 19 matching lines @@
     }
   }
 
   ppapi_host_->GetPpapiHost()->AddHostFactoryFilter(
       scoped_ptr<ppapi::host::HostFactory>(
           NaClBrowser::GetDelegate()->CreatePpapiHostFactory(
               ppapi_host_.get())));
 
   // Send a message to initialize the IPC dispatchers in the NaCl plugin.
   ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
+  return true;
+}
+
+// This method is called when NaClProcessHostMsg_PpapiChannelCreated is
+// received.
+void NaClProcessHost::OnPpapiChannelsCreated(
+    const IPC::ChannelHandle& raw_ppapi_browser_channel_handle,
+    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
+  ScopedChannelHandle ppapi_browser_channel_handle(
+      raw_ppapi_browser_channel_handle);
+  ScopedChannelHandle ppapi_renderer_channel_handle(
+      raw_ppapi_renderer_channel_handle);
+  ScopedChannelHandle trusted_renderer_channel_handle(
+      raw_trusted_renderer_channel_handle);
+  ScopedChannelHandle manifest_service_channel_handle(
+      raw_manifest_service_channel_handle);
+
+  if (enable_ppapi_proxy()) {
+    if (!StartPPAPIProxy(ppapi_browser_channel_handle.Pass())) {
+      SendErrorToRenderer("Browser PPAPI proxy could not start.");
+      return;
+    }
+  } else {
+    // If PPAPI proxy is disabled, channel handles should be invalid.
+    DCHECK(ppapi_browser_channel_handle.get().name.empty());
+    DCHECK(ppapi_renderer_channel_handle.get().name.empty());
+    // Invalidate, just in case.
+    ppapi_browser_channel_handle.reset();
+    ppapi_renderer_channel_handle.reset();
+  }
 
   // Let the renderer know that the IPC channels are established.
   ReplyToRenderer(ppapi_renderer_channel_handle.Pass(),
                   trusted_renderer_channel_handle.Pass(),
                   manifest_service_channel_handle.Pass());
 }
 
 bool NaClProcessHost::StartWithLaunchedProcess() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
@@ skipping 165 matching lines @@
         process.Pass(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl