Fix for 0-length file problem in content verification

extensions/browser/computed_hashes.cc

 // Copyright 2014 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 "extensions/browser/computed_hashes.h"
 
 #include "base/base64.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
+#include "base/stl_util.h"
+#include "base/values.h"
+#include "crypto/secure_hash.h"
+#include "crypto/sha2.h"
 
 namespace {
+const char kBlockHashesKey[] = "block_hashes";
+const char kBlockSizeKey[] = "block_size";
+const char kFileHashesKey[] = "file_hashes";
 const char kPathKey[] = "path";
-const char kBlockSizeKey[] = "block_size";
-const char kBlockHashesKey[] = "block_hashes";
+const char kVersionKey[] = "version";
+const int kVersion = 2;
 }

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

nit: It's still a pretty small block, but perhaps as it grows it's worth adding
the closing  // namespace 

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

Done.

 
 namespace extensions {
 
 ComputedHashes::Reader::Reader() {
 }

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

nit on existing code: Missing blank line

[asargent_no_longer_on_chrome, 2014/08/06 04:47:33]

Done.

 ComputedHashes::Reader::~Reader() {
 }
 
 bool ComputedHashes::Reader::InitFromFile(const base::FilePath& path) {
   std::string contents;
   if (!base::ReadFileToString(path, &contents))
     return false;
 
+  base::DictionaryValue* top_dictionary = NULL;
+  scoped_ptr<base::Value> value(base::JSONReader::Read(contents));
+  if (!value.get() || !value->GetAsDictionary(&top_dictionary))
+    return false;
+
+  // For now we don't support forwards or backwards compatability in the
+  // format, so we return false on version mismatch.
+  int version = 0;
+  if (!top_dictionary->GetInteger(kVersionKey, &version) || version != kVersion)
+    return false;
+
   base::ListValue* all_hashes = NULL;
-  scoped_ptr<base::Value> value(base::JSONReader::Read(contents));
-  if (!value.get() || !value->GetAsList(&all_hashes))
+  if (!top_dictionary->GetList(kFileHashesKey, &all_hashes))
     return false;
 
   for (size_t i = 0; i < all_hashes->GetSize(); i++) {
     base::DictionaryValue* dictionary = NULL;
     if (!all_hashes->GetDictionary(i, &dictionary))
       return false;
 
     std::string relative_path_utf8;
     if (!dictionary->GetString(kPathKey, &relative_path_utf8))
       return false;
@@ skipping 40 matching lines @@
   base::FilePath path = relative_path.NormalizePathSeparatorsTo('/');
   std::map<base::FilePath, HashInfo>::iterator i = data_.find(path);
   if (i == data_.end())
     return false;
   HashInfo& info = i->second;
   *block_size = info.first;
   *hashes = info.second;
   return true;
 }
 
-ComputedHashes::Writer::Writer() {
+ComputedHashes::Writer::Writer() : file_list_(new base::ListValue) {
 }

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

nit on existing code: Missing blank line.

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

Done.

 ComputedHashes::Writer::~Writer() {
 }
 
 void ComputedHashes::Writer::AddHashes(const base::FilePath& relative_path,
                                        int block_size,
                                        const std::vector<std::string>& hashes) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   base::ListValue* block_hashes = new base::ListValue();
-  file_list_.Append(dict);
+  file_list_->Append(dict);
   dict->SetString(kPathKey,
                   relative_path.NormalizePathSeparatorsTo('/').AsUTF8Unsafe());
   dict->SetInteger(kBlockSizeKey, block_size);
   dict->Set(kBlockHashesKey, block_hashes);
 
   for (std::vector<std::string>::const_iterator i = hashes.begin();
        i != hashes.end();
        ++i) {
     std::string encoded;
     base::Base64Encode(*i, &encoded);
     block_hashes->AppendString(encoded);
   }
 }
 
 bool ComputedHashes::Writer::WriteToFile(const base::FilePath& path) {
   std::string json;
-  if (!base::JSONWriter::Write(&file_list_, &json))
+  base::DictionaryValue top_dictionary;
+  top_dictionary.SetInteger(kVersionKey, kVersion);
+  top_dictionary.Set(kFileHashesKey, file_list_.release());
+
+  if (!base::JSONWriter::Write(&top_dictionary, &json))
     return false;
   int written = base::WriteFile(path, json.data(), json.size());
   if (static_cast<unsigned>(written) != json.size()) {
-    LOG(ERROR) << "Error writing " << path.MaybeAsASCII()
+    LOG(ERROR) << "Error writing " << path.AsUTF8Unsafe()
                << " ; write result:" << written << " expected:" << json.size();
     return false;
   }
   return true;
 }
 
+void ComputedHashes::ComputeHashesForContent(const std::string& contents,
+                                             size_t block_size,
+                                             std::vector<std::string>* hashes) {
+  size_t offset = 0;
+  do {
+    const char* block_start = contents.data() + offset;
+    size_t bytes_to_read = std::min(contents.size() - offset, block_size);
+    DCHECK(bytes_to_read >= 0);

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

Sort of an invalid DCHECK because a size_t is always non-negative.

How about instead DCHECKing before the std::min that offset <= contents.size()?

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

Ah, good catch. Done.

+    scoped_ptr<crypto::SecureHash> hash(
+        crypto::SecureHash::Create(crypto::SecureHash::SHA256));
+    hash->Update(block_start, bytes_to_read);
+
+    hashes->push_back(std::string());
+    std::string* buffer = &(hashes->back());

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

Can you not use a reference here instead? Or is that discouraged for some
reason?

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

I don't think there's a rule about it one way or another. I guess I'm in the
habit of using pointers in these kinds of situations to avoid accidentally
making a copy, and also I need to use a pointer to it below on line 168.

+    buffer->resize(crypto::kSHA256Length);
+    hash->Finish(string_as_array(buffer), buffer->size());
+
+    if (bytes_to_read == 0)

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

nit: Might want to comment to make this escape more explicit (specifically that
it's only reachable if this is the first block).

It took me a second read to understand why this function wouldn't always output
a final empty-block-hash regardless of |contents|.

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

Done.

+      break;
+    else

[Ken Rockot(use gerrit already), 2014/08/05 23:10:32]

nit: no need for the else

[asargent_no_longer_on_chrome, 2014/08/06 04:47:32]

Done.

+      offset += bytes_to_read;
+  } while (offset < contents.size());
+}
+
 }  // namespace extensions