NaCl: enable meta-based validation for shared libraries.

chrome/browser/nacl_host/nacl_browser.cc

Index: chrome/browser/nacl_host/nacl_browser.cc
diff --git a/chrome/browser/nacl_host/nacl_browser.cc b/chrome/browser/nacl_host/nacl_browser.cc
index d05d42e0b6ece6e934517557f110019fe93345e1..1908e156096d7c84b4f5f43c090e5e05c7bbbaa8 100644
--- a/chrome/browser/nacl_host/nacl_browser.cc
+++ b/chrome/browser/nacl_host/nacl_browser.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
+#include "base/rand_util.h"
 #include "base/strings/string_split.h"
 #include "base/win/windows_version.h"
 #include "build/build_config.h"
@@ -106,6 +107,39 @@ void LogCacheSet(ValidationCacheStatus status) {
 
 }  // namespace
 
+namespace nacl {
+
+void OpenNaClExecutableImpl(const base::FilePath& file_path,
+                            base::PlatformFile* file) {
+  // Get a file descriptor. On Windows, we need 'GENERIC_EXECUTE' in order to
+  // memory map the executable.
+  // IMPORTANT: This file descriptor must not have write access - that could
+  // allow a sandbox escape.

[Mark Seaborn, 2013/05/16 23:01:47]

"NaCl inner sandbox escape"?

[Nick Bray (chromium), 2013/05/21 20:09:06]

Done.

+  base::PlatformFileError error_code;
+  *file = base::CreatePlatformFile(
+      file_path,
+      base::PLATFORM_FILE_OPEN |
+          base::PLATFORM_FILE_READ |

[Mark Seaborn, 2013/05/16 23:01:47]

Maybe format as:
(FLAG |
 FLAG |
 FLAG)

[Nick Bray (chromium), 2013/05/21 20:09:06]

Done.

+          base::PLATFORM_FILE_EXECUTE,  // Windows only flag.
+      NULL,
+      &error_code);
+  if (error_code != base::PLATFORM_FILE_OK) {
+    *file = base::kInvalidPlatformFileValue;
+    return;
+  }
+  // Check that the file does not reference a directory. Returning a descriptor
+  // to an extension directory could allow a sandbox escape.

[Mark Seaborn, 2013/05/16 23:01:47]

Same here - which sandbox?  Probably not the inner sandbox this time, but
certainly some Linux outer sandboxes.

[Nick Bray (chromium), 2013/05/21 20:09:06]

Done.

+  base::PlatformFileInfo file_info;
+  if (!base::GetPlatformFileInfo(*file, &file_info) || file_info.is_directory)
+  {

[Mark Seaborn, 2013/05/16 23:01:47]

Put on previous line

[Nick Bray (chromium), 2013/05/21 20:09:06]

Done.

+    base::ClosePlatformFile(*file);
+    *file = base::kInvalidPlatformFileValue;
+    return;
+  }
+}
+
+}
+
 NaClBrowser::NaClBrowser()
     : weak_factory_(this),
       irt_platform_file_(base::kInvalidPlatformFileValue),
@@ -119,6 +153,7 @@ NaClBrowser::NaClBrowser()
                       kValidationCacheEnabledByDefault)),
       validation_cache_is_modified_(false),
       validation_cache_state_(NaClResourceUninitialized),
+      path_cache_(10),

[Mark Seaborn, 2013/05/16 23:01:47]

From discussion in person:  10 is the number of requests that can be in progress
at once.  If you have 11 requests in progress simultaneously, some can fail.  So
this isn't a cache -- cache misses shouldn't affect correctness.

So you either need to fix that, or put in a big TODO and explanation here of the
meaning of 10.  And don't call it "path_cache_". :-)

Why is this limit set so low when you're only mapping IDs -> filenames?  It's
not like this is holding on to file descriptors.

[Nick Bray (chromium), 2013/05/21 20:09:06]

Done, modulo being able to think of a better name.

       ok_(true) {
   InitIrtFilePath();
   InitValidationCacheFilePath();
@@ -372,6 +407,36 @@ const base::FilePath& NaClBrowser::GetIrtFilePath() {
   return irt_filepath_;
 }
 
+uint64 NaClBrowser::PutFilePath(const base::FilePath& path) {
+  uint64 nonce;
+  while (true) {
+    nonce = base::RandUint64();

[Mark Seaborn, 2013/05/16 23:01:47]

I'm not sure 'nonce' is the appropriate term here, since it suggests the number
is unguessable, that this is a cryptographic protocol, etc., which isn't the
case.

Something like "file_token" instead, perhaps?

[Nick Bray (chromium), 2013/05/21 20:09:06]

Moving to 128 bits and Justin's request, which implies it is a nonce.

+    // A zero nonce indicates there is no nonce, if we get zero, ask for another
+    // number.
+    if (nonce != 0) {

[Mark Seaborn, 2013/05/16 23:01:47]

Has the nonce == 0 code path ever been executed? :-)

[Nick Bray (chromium), 2013/05/21 20:09:06]

It may be.  Some day.  We should add a prize.  On the other hand, once it's
moved to 128 bits, probably not.

+      // If the nonce is in use, ask for another number.
+      PathCacheType::iterator iter  = path_cache_.Peek(nonce);
+      if (iter == path_cache_.end()) {
+        path_cache_.Put(nonce, path);
+        break;
+      }
+    }
+  }
+  return nonce;
+}
+
+bool NaClBrowser::GetFilePath(uint64 nonce, base::FilePath* path) {
+  PathCacheType::iterator iter  = path_cache_.Peek(nonce);
+  if (iter == path_cache_.end()) {
+    *path = base::FilePath(FILE_PATH_LITERAL(""));
+    return false;
+  }
+  *path = iter->second;
+  path_cache_.Erase(iter);
+  return true;
+}
+
+
 bool NaClBrowser::QueryKnownToValidate(const std::string& signature,
                                        bool off_the_record) {
   if (off_the_record) {