NaCl: enable meta-based validation for shared libraries.

chrome/browser/nacl_host/nacl_browser.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 "chrome/browser/nacl_host/nacl_browser.h"
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #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"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/common/url_pattern.h"
 #include "googleurl/src/gurl.h"
 
@@ skipping 76 matching lines @@
   UMA_HISTOGRAM_ENUMERATION("NaCl.ValidationCache.Query", status, CACHE_MAX);
 }
 
 void LogCacheSet(ValidationCacheStatus status) {
   // Bucket zero is reserved for future use.
   UMA_HISTOGRAM_ENUMERATION("NaCl.ValidationCache.Set", status, CACHE_MAX);
 }
 
 }  // 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),
       irt_filepath_(),
       irt_state_(NaClResourceUninitialized),
       debug_patterns_(),
       inverse_debug_patterns_(false),
       validation_cache_file_path_(),
       validation_cache_is_enabled_(
           CheckEnvVar("NACL_VALIDATION_CACHE",
                       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();
 }
 
 NaClBrowser::~NaClBrowser() {
   if (irt_platform_file_ != base::kInvalidPlatformFileValue)
     base::ClosePlatformFile(irt_platform_file_);
 }
 
@@ skipping 233 matching lines @@
 void NaClBrowser::WaitForResources(const base::Closure& reply) {
   waiting_.push_back(reply);
   EnsureAllResourcesAvailable();
   CheckWaiting();
 }
 
 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) {
     // If we're off the record, don't reorder the main cache.
     return validation_cache_.QueryKnownToValidate(signature, false) ||
         off_the_record_validation_cache_.QueryKnownToValidate(signature, true);
   } else {
     bool result = validation_cache_.QueryKnownToValidate(signature, true);
     LogCacheQuery(result ? CACHE_HIT : CACHE_MISS);
     // Queries can modify the MRU order of the cache.
@@ skipping 80 matching lines @@
     // because it can degrade the responsiveness of the browser.
     // The task is sequenced so that multiple writes happen in order.
     content::BrowserThread::PostBlockingPoolSequencedTask(
         kValidationCacheSequenceName,
         FROM_HERE,
         base::Bind(WriteCache, validation_cache_file_path_,
                    base::Owned(pickle)));
   }
   validation_cache_is_modified_ = false;
 }