NaCl: Simpler validation for main nexe.

components/nacl/loader/nacl_listener.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_listener.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdlib.h>
 #include <string.h>
@@ skipping 11 matching lines @@
 #include "components/nacl/common/nacl_renderer_messages.h"
 #include "components/nacl/loader/nacl_ipc_adapter.h"
 #include "components/nacl/loader/nacl_validation_db.h"
 #include "components/nacl/loader/nacl_validation_query.h"
 #include "ipc/ipc_channel_handle.h"
 #include "ipc/ipc_switches.h"
 #include "ipc/ipc_sync_channel.h"
 #include "ipc/ipc_sync_message_filter.h"
 #include "native_client/src/public/chrome_main.h"
 #include "native_client/src/public/nacl_app.h"
+#include "native_client/src/public/nacl_desc.h"
 #include "native_client/src/public/nacl_file_info.h"
+#include "native_client/src/trusted/desc/nacl_desc_io.h"
 #include "native_client/src/trusted/service_runtime/include/sys/fcntl.h"
+#include "native_client/src/trusted/validator/rich_file_info.h"

[Mark Seaborn, 2014/10/14 17:37:24]

This isn't needed, is it?

[teravest, 2014/10/14 18:15:00]

Nope, removed. 

 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
 #endif
 
 #if defined(OS_LINUX)
 #include "content/public/common/child_process_sandbox_support_linux.h"
 #endif
 
 #if defined(OS_WIN)
@@ skipping 132 matching lines @@
     return result;
   }
 
   virtual void SetKnownToValidate(const std::string& signature) override {
     // Caching is optional: NaCl will still work correctly if the IPC fails.
     if (!listener_->Send(new NaClProcessMsg_SetKnownToValidate(signature))) {
       LOG(ERROR) << "Failed to update NaCl validation cache.";
     }
   }
 
-  // This is the "old" code path for resolving file tokens. It's only
-  // used for resolving the main nexe.
-  // TODO(teravest): Remove this.
+  // This function is no longer used.
   virtual bool ResolveFileToken(struct NaClFileToken* file_token,
                                 int32* fd, std::string* path) override {
-    *fd = -1;
-    *path = "";
-    if (!NaClFileTokenIsValid(file_token)) {
-      return false;
-    }
-    IPC::PlatformFileForTransit ipc_fd = IPC::InvalidPlatformFileForTransit();
-    base::FilePath ipc_path;
-    if (!listener_->Send(new NaClProcessMsg_ResolveFileToken(file_token->lo,
-                                                             file_token->hi,
-                                                             &ipc_fd,
-                                                             &ipc_path))) {
-      return false;
-    }
-    if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
-      return false;
-    }
-    base::PlatformFile handle =
-        IPC::PlatformFileForTransitToPlatformFile(ipc_fd);
-#if defined(OS_WIN)
-    // On Windows, valid handles are 32 bit unsigned integers so this is safe.
-    *fd = reinterpret_cast<uintptr_t>(handle);
-#else
-    *fd = handle;
-#endif
-    // It doesn't matter if the path is invalid UTF8 as long as it's consistent
-    // and unforgeable.
-    *path = ipc_path.AsUTF8Unsafe();
-    return true;
+    CHECK(false);
+    return false;
   }
 
  private:
   // The listener never dies, otherwise this might be a dangling reference.
   NaClListener* listener_;
 };
 
 
 NaClListener::NaClListener() : shutdown_event_(true, false),
                                io_thread_("NaCl_IOThread"),
@@ skipping 218 matching lines @@
 #if defined(OS_WIN)
   args->broker_duplicate_handle_func = BrokerDuplicateHandle;
   args->attach_debug_exception_handler_func = AttachDebugExceptionHandler;
   args->debug_stub_server_port_selected_handler_func =
       DebugStubPortSelectedHandler;
 #endif
 #if defined(OS_LINUX)
   args->prereserved_sandbox_size = prereserved_sandbox_size_;
 #endif
 
-  NaClFileInfo nexe_file_info;
   base::PlatformFile nexe_file = IPC::PlatformFileForTransitToPlatformFile(
       params.nexe_file);
+
+  // If nexe_file_path is valid, that metadata has to be added to the desc and
+  // it can be marked safe to mmap (since it came from the browser).
+  if (!params.nexe_file_path.empty()) {

[Mark Seaborn, 2014/10/14 17:37:24]

Hmm, if this check were omitted, would this be insecure (or less secure)?  Would
we accidentally treat all nexes as having the same identity?  (Or would the
identity be diluted?  We'd include fstat() metadata in the identity but not the
filename.)

How about changing NaClDescCreateWithFilePathMetadata() so that either:

 * it rejects an empty filename;

 * better: when given an empty filename, it falls back to the non-caching
version (NaClDescIoDescFromDescAllocCtor() as below).  Then we wouldn't need 2
code paths in this Chromium code.

[teravest, 2014/10/14 18:15:00]

Sounds good, I'll mail out a NaCl change that does the fallback in
NaClDescCreateWithFilePathMetadata shortly.

+    std::string file_path_str = params.nexe_file_path.AsUTF8Unsafe();
+    args->nexe_desc = NaClDescCreateWithFilePathMetadata(nexe_file,
+                                                         file_path_str.c_str());
+  } else {
+    int desc;
 #if defined(OS_WIN)
-  nexe_file_info.desc =
-      _open_osfhandle(reinterpret_cast<intptr_t>(nexe_file),
-                      _O_RDONLY | _O_BINARY);
+    desc = _open_osfhandle(reinterpret_cast<intptr_t>(nexe_file),
+                           _O_RDONLY | _O_BINARY);
 #elif defined(OS_POSIX)
-  nexe_file_info.desc = nexe_file;
+    desc = nexe_file;
 #else
 #error Unsupported target platform.
 #endif
-  nexe_file_info.file_token.lo = params.nexe_token_lo;
-  nexe_file_info.file_token.hi = params.nexe_token_hi;
-  args->nexe_desc = NaClDescIoFromFileInfo(nexe_file_info, NACL_ABI_O_RDONLY);
+    args->nexe_desc = NaClDescIoDescFromDescAllocCtor(desc, NACL_ABI_O_RDONLY);
+  }
 
   int exit_status;
   if (!NaClChromeMainStart(nap, args, &exit_status))
     NaClExit(1);
 
   // Report the plugin's exit status if the application started successfully.
   trusted_listener_->Send(new NaClRendererMsg_ReportExitStatus(exit_status));
   NaClExit(exit_status);
 }
 
 void NaClListener::ResolveFileToken(
     uint64_t token_lo,
     uint64_t token_hi,
     base::Callback<void(IPC::PlatformFileForTransit, base::FilePath)> cb) {
   if (!Send(new NaClProcessMsg_ResolveFileTokenAsync(token_lo, token_hi))) {
     cb.Run(IPC::PlatformFileForTransit(), base::FilePath());
     return;
   }
   resolved_cb_ = cb;
 }
 
 void NaClListener::OnFileTokenResolved(
     uint64_t token_lo,
     uint64_t token_hi,
     IPC::PlatformFileForTransit ipc_fd,
     base::FilePath file_path) {
   resolved_cb_.Run(ipc_fd, file_path);
   resolved_cb_.Reset();
 }