NaCl: Simpler validation for main nexe.

components/nacl/loader/nacl_listener.cc

Index: components/nacl/loader/nacl_listener.cc
diff --git a/components/nacl/loader/nacl_listener.cc b/components/nacl/loader/nacl_listener.cc
index ed7f05d72a20a48c08770bb7a7097e23d36b0482..257cd9147a0afeb1a7e5a171b98144ff74ea0c5a 100644
--- a/components/nacl/loader/nacl_listener.cc
+++ b/components/nacl/loader/nacl_listener.cc
@@ -29,8 +29,11 @@
 #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"
@@ -183,39 +186,11 @@ class BrowserValidationDBProxy : public NaClValidationDB {
     }
   }
 
-  // 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:
@@ -454,21 +429,27 @@ void NaClListener::OnStart(const nacl::NaClStartParams& params) {
   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))