Pepper: Stop using SRPC for irt_open_resource().

components/nacl/loader/nacl_ipc_adapter.cc

 // Copyright 2013 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/loader/nacl_ipc_adapter.h"
 
 #include <limits.h>
 #include <string.h>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
+#include "base/task_runner_util.h"
 #include "build/build_config.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_platform_file.h"
 #include "native_client/src/trusted/desc/nacl_desc_base.h"
 #include "native_client/src/trusted/desc/nacl_desc_custom.h"
 #include "native_client/src/trusted/desc/nacl_desc_imc_shm.h"
 #include "native_client/src/trusted/desc/nacl_desc_io.h"
 #include "native_client/src/trusted/desc/nacl_desc_quota.h"
 #include "native_client/src/trusted/desc/nacl_desc_quota_interface.h"
 #include "native_client/src/trusted/desc/nacl_desc_sync_socket.h"
 #include "native_client/src/trusted/desc/nacl_desc_wrapper.h"
 #include "native_client/src/trusted/service_runtime/include/sys/fcntl.h"
+#include "native_client/src/trusted/validator/rich_file_info.h"
 #include "ppapi/c/ppb_file_io.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/serialized_handle.h"
 
 using ppapi::proxy::NaClMessageScanner;
 
 namespace {
 
 enum BufferSizeStatus {
   // The buffer contains a full message with no extra bytes.
@@ skipping 285 matching lines @@
 
 NaClIPCAdapter::IOThreadData::~IOThreadData() {
 }
 
 NaClIPCAdapter::NaClIPCAdapter(const IPC::ChannelHandle& handle,
                                base::TaskRunner* runner)
     : lock_(),
       cond_var_(&lock_),
       task_runner_(runner),
       locked_data_() {
+  main_task_runner_ = base::MessageLoop::current()->task_runner();
   io_thread_data_.channel_ = IPC::Channel::CreateServer(handle, this);
   // Note, we can not PostTask for ConnectChannelOnIOThread here. If we did,
   // and that task ran before this constructor completes, the reference count
   // would go to 1 and then to 0 because of the Task, before we've been returned
   // to the owning scoped_refptr, which is supposed to give us our first
   // ref-count.
 }
 
 NaClIPCAdapter::NaClIPCAdapter(scoped_ptr<IPC::Channel> channel,
                                base::TaskRunner* runner)
@@ skipping 116 matching lines @@
 }
 
 #if defined(OS_POSIX)
 int NaClIPCAdapter::TakeClientFileDescriptor() {
   return io_thread_data_.channel_->TakeClientFileDescriptor();
 }
 #endif
 
 bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
   uint32_t type = msg.type();
+
   if (type == IPC_REPLY_ID) {
     int id = IPC::SyncMessage::GetMessageId(msg);
     IOThreadData::PendingSyncMsgMap::iterator it =
         io_thread_data_.pending_sync_msgs_.find(id);
     DCHECK(it != io_thread_data_.pending_sync_msgs_.end());
     if (it != io_thread_data_.pending_sync_msgs_.end()) {
       type = it->second;
       io_thread_data_.pending_sync_msgs_.erase(it);
     }
   }
+  // Handle PpapiHostMsg_OpenResource outside the lock as it requires sending
+  // IPC to handle properly.

[dmichael (off chromium), 2014/08/27 20:49:14]

I know Mark strongly preferred knowledge of proxy messages stayed in
ppapi/proxy. (Arguably this one is more of an IRT than proxy thing, I guess.)

Also FYI...  Bill had written something to send a message while handling another
message. He had added a new "ChannelHandle" thing to SerializedHandle....  looks
like Hidehiko removed it
https://codereview.chromium.org/150713003/diff/390001/components/nacl/loader/...
a while back. I think more manual work is probably appropriate here though.

Now that you're doing Async messages, do you still care about doing this without
the lock? Maybe it would be fine to move this in to the NaClMessageScanner?

[Mark Seaborn, 2014/08/28 21:33:49]

Yeah, open_resource() isn't part of PPAPI, so it's more appropriate to have
handling of open_resource messages in a */nacl/* file than in ppapi/proxy/.  So
this part seems OK as it is.

+  if (type == PpapiHostMsg_OpenResource::ID) {
+    PickleIterator iter = IPC::SyncMessage::GetDataIterator(&msg);
+    ppapi::proxy::SerializedHandle ignore_fd;
+    uint64_t token_lo;
+    uint64_t token_hi;
+    if (!IPC::ReadParam(&msg, &iter, &ignore_fd) ||
+        !IPC::ReadParam(&msg, &iter, &token_lo) ||
+        !IPC::ReadParam(&msg, &iter, &token_hi)) {
+      return false;
+    }
 
+    if (token_lo != 0 || token_hi != 0) {
+      // We've received a valid file token, but we have to validate it with the
+      // browser before it can be used. This is to prevent a compromised
+      // renderer from running arbitrary code in the plugin process.

[Mark Seaborn, 2014/08/28 23:40:22]

"plugin process" -> "NaCl process" to clarify

[teravest, 2014/09/04 22:13:30]

Done.

+      DCHECK(!resolve_file_token_cb_.is_null());
+
+      // resolve_file_token_cb_ must be invoked from the main thread.
+      resolve_file_token_cb_.Run(
+          token_lo,
+          token_hi,
+          base::Bind(&NaClIPCAdapter::OnFileTokenResolved,
+                     this,
+                     msg));
+      return true;

[Mark Seaborn, 2014/08/28 23:40:22]

Maybe comment: We don't release the message to NaCl untrusted code immediately. 
We defer it until we get an async message back from the browser process.

[teravest, 2014/09/04 22:13:30]

Done.

+    }
+  }
+  return RewriteMessage(msg, type);
+}
+
+bool NaClIPCAdapter::RewriteMessage(const IPC::Message& msg, uint32_t type) {
   {
     base::AutoLock lock(lock_);
     scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
 
     typedef std::vector<ppapi::proxy::SerializedHandle> Handles;
     Handles handles;
     scoped_ptr<IPC::Message> new_msg;
 
     if (!locked_data_.nacl_msg_scanner_.ScanMessage(
             msg, type, &handles, &new_msg))
@@ skipping 60 matching lines @@
     }
     if (new_msg)
       SaveMessage(*new_msg, rewritten_msg.get());
     else
       SaveMessage(msg, rewritten_msg.get());
   }
   cond_var_.Signal();
   return true;
 }
 
+void NaClIPCAdapter::OnFileTokenResolved(const IPC::Message& orig_msg,
+                                         IPC::PlatformFileForTransit ipc_fd,
+                                         base::FilePath file_path) {
+  if (ipc_fd != IPC::InvalidPlatformFileForTransit()) {

[Mark Seaborn, 2014/08/28 23:40:22]

Maybe handle the error case first for readability:

if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
  RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
  return;
}

[teravest, 2014/09/04 22:13:30]

Done.

+    // The file token was sucessfully resolved.
+    std::string file_path_str = file_path.AsUTF8Unsafe();
+    base::PlatformFile handle =
+        IPC::PlatformFileForTransitToPlatformFile(ipc_fd);
+    int32_t new_fd;
+#if defined(OS_WIN)
+    // On Windows, valid handles are 32 bit unsigned integers so this is
+    // safe.
+    new_fd = reinterpret_cast<uintptr_t>(handle);
+#else
+    new_fd = handle;
+#endif
+    // The file token was resolved successfully, so we populate the new
+    // NaClDesc with that information.
+    char* alloc_file_path = static_cast<char*>(
+        malloc(file_path_str.length() + 1));
+    strcpy(alloc_file_path, file_path_str.c_str());
+    scoped_ptr<NaClDescWrapper> desc_wrapper(new NaClDescWrapper(
+        NaClDescIoDescFromHandleAllocCtor(
+            (NaClHandle) new_fd, NACL_ABI_O_RDONLY)));
+
+    // Mark the desc as OK for mmapping.
+    NaClDescMarkSafeForMmap(desc_wrapper->desc());
+
+    // Provide metadata for validation.
+    struct NaClRichFileInfo info;
+    NaClRichFileInfoCtor(&info);
+    info.known_file = 1;
+    info.file_path = alloc_file_path;  // Takes ownership.
+    info.file_path_length =
+        static_cast<uint32_t>(file_path_str.length());
+    NaClSetFileOriginInfo(desc_wrapper->desc(), &info);
+    NaClRichFileInfoDtor(&info);
+    {
+      scoped_ptr<IPC::Message> new_msg(new IPC::Message(
+          orig_msg.routing_id(),
+          orig_msg.type(),
+          IPC::Message::PRIORITY_NORMAL));
+
+      scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
+      // We have to rewrite the message a bit here to clear the file token
+      // data.
+      new_msg->set_reply();
+      new_msg->WriteInt(IPC::SyncMessage::GetMessageId(orig_msg));
+
+      // Write the SerializedHandle. There's only ever one in this message, so
+      // we just write an index of zero.
+      ppapi::proxy::SerializedHandle sh;
+      sh.set_file_handle(ipc_fd, PP_FILEOPENFLAG_READ, 0);
+      ppapi::proxy::SerializedHandle::WriteHeader(sh.header(),
+                                                  new_msg.get());
+      new_msg->WriteBool(true);
+      new_msg->WriteInt(0);
+
+      // Write empty file tokens.
+      new_msg->WriteUInt64(0);
+      new_msg->WriteUInt64(0);
+
+      rewritten_msg->AddDescriptor(desc_wrapper.release());
+      {
+        base::AutoLock lock(lock_);
+        SaveMessage(*new_msg, rewritten_msg.get());
+      }
+    }
+    cond_var_.Signal();
+  } else {
+    RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
+  }
+}
+
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
 }
 
 void NaClIPCAdapter::OnChannelError() {
   CloseChannel();
 }
 
 NaClIPCAdapter::~NaClIPCAdapter() {
   // Make sure the channel is deleted on the IO thread.
   task_runner_->PostTask(FROM_HERE,
@@ skipping 114 matching lines @@
   header.flags = msg.flags();
   header.num_fds = static_cast<int>(rewritten_msg->desc_count());
 
   rewritten_msg->SetData(header, msg.payload(), msg.payload_size());
   locked_data_.to_be_received_.push(rewritten_msg);
 }
 
 int TranslatePepperFileReadWriteOpenFlagsForTesting(int32_t pp_open_flags) {
   return TranslatePepperFileReadWriteOpenFlags(pp_open_flags);
 }