NaCl: Simpler validation for main nexe.

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 212 matching lines @@
       reinterpret_cast<nacl::FileDescriptor>(channel));
 #else
   nacl::FileDescriptor channel;
   channel.fd = sourceh;
   channel.auto_close = close_source;
   handles_for_sel_ldr->push_back(channel);
 #endif
   return true;
 }
 
+void CloseFile(base::File file) {
+  // The base::File destructor will close the file for us.
+}
+
 }  // namespace
 
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
 NaClProcessHost::NaClProcessHost(const GURL& manifest_url,
                                  base::File nexe_file,
                                  const NaClFileToken& nexe_token,
                                  ppapi::PpapiPermissions permissions,
                                  int render_view_id,
@@ skipping 382 matching lines @@
       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_DELAY_REPLY(NaClProcessMsg_ResolveFileToken,
-                                      OnResolveFileToken)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileTokenAsync,
-                          OnResolveFileTokenAsync)
+      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
+                          OnResolveFileToken)
 
 #if defined(OS_WIN)
       IPC_MESSAGE_HANDLER_DELAY_REPLY(
           NaClProcessMsg_AttachDebugExceptionHandler,
           OnAttachDebugExceptionHandler)
       IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected,
                           OnDebugStubPortSelected)
 #endif
       IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
                           OnPpapiChannelsCreated)
@@ skipping 166 matching lines @@
     // not created.
     DCHECK(!socket_for_sel_ldr_.IsValid());
 #endif
   } 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_debug_stub_ &&
         NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
 
-    // TODO(teravest): Resolve the file tokens right now instead of making the
-    // loader send IPC to resolve them later.
-    params.nexe_token_lo = nexe_token_.lo;
-    params.nexe_token_hi = nexe_token_.hi;
-
     const ChildProcessData& data = process_->GetData();
     if (!ShareHandleToSelLdr(data.handle,
                              socket_for_sel_ldr_.TakePlatformFile(),
                              true,
                              &params.handles)) {
       return false;
     }
 
     // Currently, everything uses the IRT except for the PNaCl Translators.
     bool uses_irt = process_type_ != kPNaClTranslatorProcessType;
@@ skipping 34 matching lines @@
     if (params.enable_debug_stub) {
       net::SocketDescriptor server_bound_socket = GetDebugStubSocketHandle();
       if (server_bound_socket != net::kInvalidSocket) {
         params.debug_stub_server_bound_socket =
             FileDescriptor(server_bound_socket, true);
       }
     }
 #endif
   }
 
-  params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
-                                                   process_->GetData().handle);
   if (!crash_info_shmem_.ShareToProcess(process_->GetData().handle,
                                         &params.crash_info_shmem_handle)) {
     DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
     return false;
   }
 
+  base::FilePath file_path;
+  if (NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
+                                              nexe_token_.hi,
+                                              &file_path)) {
+    // We have to reopen the file in the browser process; we don't want a
+    // compromised renderer to pass an arbitrary fd that could get loaded
+    // into the plugin process.
+    if (base::PostTaskAndReplyWithResult(
+           content::BrowserThread::GetBlockingPool(),
+           FROM_HERE,
+           base::Bind(OpenNaClReadExecImpl,
+                      file_path,
+                      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));
   return true;
 }
 
+void NaClProcessHost::StartNaClFileResolved(
+    NaClStartParams params,
+    const base::FilePath& file_path,
+    base::File nexe_file) {

[Mark Seaborn, 2014/10/16 18:01:01]

How about renaming this to "checked_nexe_file" or something similar?

Otherwise you have both "nexe_file" (safe) and "nexe_file_" (unsafe) in scope. 
It would be easy to make a mistake with that.

[teravest, 2014/10/16 21:40:40]

Done.

+  if (nexe_file.IsValid()) {
+    // Release the file received from the renderer. This has to be done on a

[Mark Seaborn, 2014/10/16 18:01:01]

Does this mean that StartNaClExecution() and StartNaClFileResolved() are running
in different threads?  Wouldn't that mean that multiple threads are accessing
nexe_file_?  That doesn't sound like it would be thread-safe.

[teravest, 2014/10/16 21:40:40]

StartNaClExecution() and StartNaClFileResolved() are called on the same thread. 

[Mark Seaborn, 2014/10/16 21:58:40]

Let me check I understand this:

StartNaClExecution() is called in the IO thread (in response to incoming IPCs).

StartNaClExecution() doesn't need to close a handle (so it doesn't need to
interact with the blocking-IO-thread).

Earlier we do:
PostTaskAndReplyWithResult(..., bind(OpenNaClReadExecImpl),
bind(StartNaClFileResolved))
-- This calls OpenNaClReadExecImpl on a blocking-file-IO thread, but it calls
StartNaClFileResolved on the IO thread.  (I hadn't realised about the latter
part before.)

+    // 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(
+        nexe_file.Pass(), process_->GetData().handle);
+  } else {
+    params.nexe_file = IPC::TakeFileHandleForProcess(
+        nexe_file_.Pass(), process_->GetData().handle);
+  }
+  process_->Send(new NaClProcessMsg_Start(params));
+}
+
 // This method is called when NaClProcessHostMsg_PpapiChannelCreated is
 // received.
 void NaClProcessHost::OnPpapiChannelsCreated(
     const IPC::ChannelHandle& browser_channel_handle,
     const IPC::ChannelHandle& ppapi_renderer_channel_handle,
     const IPC::ChannelHandle& trusted_renderer_channel_handle,
     const IPC::ChannelHandle& manifest_service_channel_handle) {
   if (!enable_ppapi_proxy()) {
     ReplyToRenderer(IPC::ChannelHandle(),
                     trusted_renderer_channel_handle,
@@ skipping 83 matching lines @@
   *result = nacl_browser->QueryKnownToValidate(signature, off_the_record_);
 }
 
 void NaClProcessHost::OnSetKnownToValidate(const std::string& signature) {
   CHECK(!uses_nonsfi_mode_);
   NaClBrowser::GetInstance()->SetKnownToValidate(
       signature, off_the_record_);
 }
 
 void NaClProcessHost::OnResolveFileToken(uint64 file_token_lo,
-                                         uint64 file_token_hi,
-                                         IPC::Message* reply_msg) {
+                                         uint64 file_token_hi) {
   // Was the file registered?
   //
   // Note that the file path cache is of bounded size, and old entries can get
   // evicted.  If a large number of NaCl modules are being launched at once,
   // resolving the file_token may fail because the path cache was thrashed
   // while the file_token was in flight.  In this case the query fails, and we
   // need to fall back to the slower path.
   //
   // However: each NaCl process will consume 2-3 entries as it starts up, this
   // means that eviction will not happen unless you start up 33+ NaCl processes
   // at the same time, and this still requires worst-case timing.  As a
   // practical matter, no entries should be evicted prematurely.
   // The cache itself should take ~ (150 characters * 2 bytes/char + ~60 bytes
   // data structure overhead) * 100 = 35k when full, so making it bigger should
   // not be a problem, if needed.
   //
   // Each NaCl process will consume 2-3 entries because the manifest and main
   // nexe are currently not resolved.  Shared libraries will be resolved.  They
   // will be loaded sequentially, so they will only consume a single entry
   // while the load is in flight.
-  //
-  // TODO(ncbray): track behavior with UMA. If entries are getting evicted or

[Mark Seaborn, 2014/10/16 18:01:00]

Why remove this comment?  AFAIK this still applies.

[teravest, 2014/10/16 21:40:40]

Added this back.

-  // bogus keys are getting queried, this would be good to know.
   CHECK(!uses_nonsfi_mode_);
   base::FilePath file_path;
   if (!NaClBrowser::GetInstance()->GetFilePath(
         file_token_lo, file_token_hi, &file_path)) {
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        IPC::InvalidPlatformFileForTransit(),
-        base::FilePath());
-    Send(reply_msg);
-    return;
-  }
-
-  // Open the file.
-  if (!base::PostTaskAndReplyWithResult(
-          content::BrowserThread::GetBlockingPool(),
-          FROM_HERE,
-          base::Bind(OpenNaClReadExecImpl, file_path, true /* is_executable */),
-          base::Bind(&NaClProcessHost::FileResolved,
-                     weak_factory_.GetWeakPtr(),
-                     file_path,
-                     reply_msg))) {
-     NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-         reply_msg,
-         IPC::InvalidPlatformFileForTransit(),
-         base::FilePath());
-     Send(reply_msg);
-  }
-}
-
-void NaClProcessHost::OnResolveFileTokenAsync(uint64 file_token_lo,
-                                              uint64 file_token_hi) {
-  // See the comment at OnResolveFileToken() for details of the file path cache
-  // behavior.
-  CHECK(!uses_nonsfi_mode_);
-  base::FilePath file_path;
-  if (!NaClBrowser::GetInstance()->GetFilePath(
-        file_token_lo, file_token_hi, &file_path)) {
-    Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+    Send(new NaClProcessMsg_ResolveFileTokenReply(
              file_token_lo,
              file_token_hi,
              IPC::PlatformFileForTransit(),
              base::FilePath()));
     return;
   }
 
   // Open the file.
   if (!base::PostTaskAndReplyWithResult(
           content::BrowserThread::GetBlockingPool(),
           FROM_HERE,
           base::Bind(OpenNaClReadExecImpl, file_path, true /* is_executable */),
-          base::Bind(&NaClProcessHost::FileResolvedAsync,
+          base::Bind(&NaClProcessHost::FileResolved,
                      weak_factory_.GetWeakPtr(),
                      file_token_lo,
                      file_token_hi,
                      file_path))) {
-    Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+    Send(new NaClProcessMsg_ResolveFileTokenReply(
             file_token_lo,
             file_token_hi,
             IPC::PlatformFileForTransit(),
             base::FilePath()));
   }
 }
 
 void NaClProcessHost::FileResolved(
-    const base::FilePath& file_path,
-    IPC::Message* reply_msg,
-    base::File file) {
-  if (file.IsValid()) {
-    IPC::PlatformFileForTransit handle = IPC::TakeFileHandleForProcess(
-        file.Pass(),
-        process_->GetData().handle);
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        handle,
-        file_path);
-  } else {
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        IPC::InvalidPlatformFileForTransit(),
-        base::FilePath());
-  }
-  Send(reply_msg);
-}
-
-void NaClProcessHost::FileResolvedAsync(
     uint64_t file_token_lo,
     uint64_t file_token_hi,
     const base::FilePath& file_path,
     base::File file) {
   base::FilePath out_file_path;
   IPC::PlatformFileForTransit out_handle;
   if (file.IsValid()) {
     out_file_path = file_path;
     out_handle = IPC::TakeFileHandleForProcess(
         file.Pass(),
         process_->GetData().handle);
   } else {
     out_handle = IPC::InvalidPlatformFileForTransit();
   }
-  Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+  Send(new NaClProcessMsg_ResolveFileTokenReply(
            file_token_lo,
            file_token_hi,
            out_handle,
            out_file_path));
 }
 
 #if defined(OS_WIN)
 void NaClProcessHost::OnAttachDebugExceptionHandler(const std::string& info,
                                                     IPC::Message* reply_msg) {
   CHECK(!uses_nonsfi_mode_);
@@ skipping 60 matching lines @@
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl