Check that the PNaCl cache hash is truly derived from the bitcode content.

ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc

Index: ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc
diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc
index bcf636254aa907ded1866aa8ca97aca6ee42fc4a..3e1d335aa63933e6317ccc25cace7dfd97aaa038 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc
@@ -365,6 +365,11 @@ PnaclCoordinator::PnaclCoordinator(
                  static_cast<void*>(this), static_cast<void*>(plugin)));
   callback_factory_.Initialize(this);
   ld_manifest_.reset(new PnaclLDManifest(plugin_->manifest(), manifest_.get()));
+  pp::Module *module = pp::Module::Get();
+  DCHECK(module);
+  nacl_hash_private_interface_ = static_cast<const PPB_NaCl_Hash_Private*>(
+      module->GetBrowserInterface(PPB_NACL_HASH_PRIVATE_INTERFACE));
+  bitcode_hash_verifier_ = nacl_hash_private_interface_->CreateSHA256Hash();
 }
 
 PnaclCoordinator::~PnaclCoordinator() {
@@ -379,6 +384,7 @@ PnaclCoordinator::~PnaclCoordinator() {
   if (translate_thread_.get() != NULL) {
     translate_thread_->AbortSubprocesses();
   }
+  nacl_hash_private_interface_->Delete(bitcode_hash_verifier_);
 }
 
 void PnaclCoordinator::ReportNonPpapiError(enum PluginErrorCode err_code,
@@ -470,13 +476,20 @@ void PnaclCoordinator::TranslateFinished(int32_t pp_error) {
 
   if (pnacl_options_.HasCacheKey() && cached_nexe_file_ != NULL) {
     // We are using a cache, but had a cache miss, which is why we did the
-    // translation.  Reset cached_nexe_file_ to have a random name,
-    // for scratch purposes, before renaming to the final cache_identity.
-    cached_nexe_file_.reset(new LocalTempFile(plugin_, file_system_.get(),
-                                              nacl::string(kPnaclTempDir)));
-    pp::CompletionCallback cb = callback_factory_.NewCallback(
-        &PnaclCoordinator::CachedNexeOpenedForWrite);
-    cached_nexe_file_->OpenWrite(cb);
+    // translation.
+    if (BitcodeHashMatchesContents()) {
+      // Reset cached_nexe_file_ to have a random name, for scratch purposes,
+      // before renaming to the final cache_identity.
+      cached_nexe_file_.reset(new LocalTempFile(plugin_, file_system_.get(),
+                                                nacl::string(kPnaclTempDir)));
+      pp::CompletionCallback cb = callback_factory_.NewCallback(
+          &PnaclCoordinator::CachedNexeOpenedForWrite);
+      cached_nexe_file_->OpenWrite(cb);
+    } else {
+      ReportNonPpapiError(
+          ERROR_PNACL_CACHE_HASH_MISMATCH,
+          "Bitcode hash does not match bitcode contents.");
+    }
   } else {
     // For now, tolerate bitcode that is missing a cache identity, and
     // tolerate the lack of caching in incognito mode.
@@ -485,6 +498,31 @@ void PnaclCoordinator::TranslateFinished(int32_t pp_error) {
   }
 }
 
+bool PnaclCoordinator::BitcodeHashMatchesContents() {
+  const nacl::string& expected_hash = pnacl_options_.GetBitcodeHash();
+  nacl::string actual_hash;
+  uint8_t hash_digest[32];
+  nacl_hash_private_interface_->Finish(bitcode_hash_verifier_,
+                                       &hash_digest,
+                                       sizeof hash_digest);
+  // Convert 32-byte binary digest to 64-byte hex digest for comparison.
+  for (size_t i = 0; i < sizeof hash_digest; i+=8) {
+    char buf[17]; // do 16 chars + a null byte at a time.
+    SNPRINTF(buf, sizeof buf, "%02x%02x%02x%02x%02x%02x%02x%02x",
+             hash_digest[i  ], hash_digest[i+1],
+             hash_digest[i+2], hash_digest[i+3],
+             hash_digest[i+4], hash_digest[i+5],
+             hash_digest[i+6], hash_digest[i+7]);
+    actual_hash.append(buf);
+  }
+  if (expected_hash.size() != actual_hash.size()) {
+    return false;
+  }
+  return nacl_hash_private_interface_->SecureMemEqual(expected_hash.c_str(),
+                                                      actual_hash.c_str(),
+                                                      expected_hash.size());
+}
+
 void PnaclCoordinator::CachedNexeOpenedForWrite(int32_t pp_error) {
   if (pp_error != PP_OK) {
     if (pp_error == PP_ERROR_NOACCESS) {
@@ -910,11 +948,17 @@ void PnaclCoordinator::BitcodeStreamGotData(int32_t pp_error,
   PLUGIN_PRINTF(("PnaclCoordinator::BitcodeStreamGotData (pp_error=%"
                  NACL_PRId32", data=%p)\n", pp_error, data ? &(*data)[0] : 0));
   DCHECK(translate_thread_.get());
-  translate_thread_->PutBytes(data, pp_error);
   // If pp_error > 0, then it represents the number of bytes received.
-  if (data && pp_error > 0) {
+  if (pp_error > 0) {
     pexe_size_ += pp_error;
+    // Update hash for verification before data is taken by PutBytes.
+    if (pnacl_options_.HasCacheKey()) {
+      nacl_hash_private_interface_->Update(bitcode_hash_verifier_,

[Derek Schuff, 2013/04/30 23:52:38]

The hash we put in the bitcode header is not the hash of the whole file, but
only the hash of the contents that are actually the bitcode. So this hash will
not match that one. 
I'm not sure how you are generating the hashes in the manifest now (presumably
just commandline sha256sum if this is actually working). If we do go with the
hash in the bitcode header, then will just read it directly and can also get the
offset of the actual bitcode and adjust this accordingly.

[jvoung (off chromium), 2013/05/01 00:01:30]

The current hash generator is a function in the SDK's generate_nmf.py.  It just
takes the whole file and hashes it to make this work.

Yes if we use the bitcode header hash, we would need to skip ahead N bytes
before hashing.

+                                           &(*data)[0],
+                                           pp_error);
+    }
   }
+  translate_thread_->PutBytes(data, pp_error);
 }
 
 StreamCallback PnaclCoordinator::GetCallback() {