Refactor CRX verification in preparation to support CRX₃ files.

components/crx_file/crx_verifier.cc

Index: components/crx_file/crx_verifier.cc
diff --git a/components/crx_file/crx_verifier.cc b/components/crx_file/crx_verifier.cc
new file mode 100644
index 0000000000000000000000000000000000000000..766d6b744ed7399fba54d90b3eca47ef55089b84
--- /dev/null
+++ b/components/crx_file/crx_verifier.cc
@@ -0,0 +1,183 @@
+// Copyright 2017 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 "components/crx_file/crx_verifier.h"
+
+#include <cstring>
+#include <memory>
+
+#include "base/base64.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "components/crx_file/crx2_file.h"
+#include "components/crx_file/id_util.h"
+#include "crypto/secure_hash.h"
+#include "crypto/secure_util.h"
+#include "crypto/sha2.h"
+#include "crypto/signature_verifier.h"
+
+namespace crx_file {
+
+namespace {
+
+// The maximum size the crx2 parser will tolerate for a public key.
+static const uint32_t kMaxPublicKeySize = 1 << 16;

[Sorin Jianu, 2017/05/15 19:49:52]

static not needed here, also maybe use constexpr.

[waffles, 2017/05/16 00:29:02]

Done.

+
+// The maximum size the crx2 parser will tolerate for a signature.
+static const uint32_t kMaxSignatureSize = 1 << 16;
+
+int ReadAndHashBuffer(uint8_t* buffer,
+                      int length,
+                      base::File* file,
+                      crypto::SecureHash* hash) {
+  // We assume a char is 8 bits long.

[Sorin Jianu, 2017/05/15 19:49:52]

we can compile time assert on the size of the char type.

[waffles, 2017/05/16 00:29:02]

Done.

+  int read = file->ReadAtCurrentPos(reinterpret_cast<char*>(buffer), length);
+  hash->Update(buffer, read);
+  return read;
+}
+
+// Returns UINT32_MAX in the case of an unexpected EOF or read error.

[Sorin Jianu, 2017/05/15 19:49:52]

Can the hash on an int32 it be UINT32_MAX?

[waffles, 2017/05/16 00:29:02]

I don't understand what you mean. Can you rephrase?
Tried to clarify the comment.

+uint32_t ReadAndHashLittleEndianUInt32(base::File* file,
+                                       crypto::SecureHash* hash) {
+  uint8_t buffer[4];

[Sorin Jianu, 2017/05/15 19:49:52]

4 as a magic number used a several places, maybe use some sizeofs etc.

[Sorin Jianu, 2017/05/15 19:49:52]

not initialized

[waffles, 2017/05/16 00:29:02]

Do you feel strongly about this? The return expression becomes more complex to
read if it avoids the magic numbers.

[waffles, 2017/05/16 00:29:02]

Done.

+  if (ReadAndHashBuffer(buffer, 4, file, hash) != 4)
+    return UINT32_MAX;
+  return buffer[3] << 24 | buffer[2] << 16 | buffer[1] << 8 | buffer[0];
+}
+
+}  // namespace
+
+CrxVerifier::CrxVerifier() {}
+
+CrxVerifier::~CrxVerifier() {}
+
+void CrxVerifier::RequireKeyProof(const std::vector<uint8_t>& key_hash) {
+  key_hashes_.push_back(key_hash);

[Sorin Jianu, 2017/05/15 19:49:52]

We could handle the case where the key_hash lenght is different than
crypto::kSHA256Length and not even allow setting up the object with invalid data
members.

[waffles, 2017/05/16 00:29:01]

Obsolete.

+}
+
+void CrxVerifier::RequireFileHash(const std::vector<uint8_t>& expected_hash) {
+  DCHECK(!expected_hash.empty());
+  expected_hash_ = expected_hash;
+}
+
+void CrxVerifier::GetPublicKey(std::string* public_key) {
+  public_key_ = public_key;
+}
+
+void CrxVerifier::GetCrxId(std::string* crx_id) {
+  crx_id_ = crx_id;
+}
+
+void CrxVerifier::RequireCrx3() {
+  allow_crx2_ = false;
+}
+
+void CrxVerifier::RequirePublisherProof() {
+  DCHECK(!allow_crx2_);
+  require_publisher_proof_ = true;
+}
+
+CrxVerifier::Result CrxVerifier::Verify(const base::FilePath& crx_path) const {
+  base::File file(crx_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid())
+    return Result::ERROR_FILE_NOT_READABLE;
+
+  std::unique_ptr<crypto::SecureHash> file_hash =
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+
+  // Magic number.
+  bool diff = false;
+  char buffer[kCrx2FileHeaderMagicSize];

[Sorin Jianu, 2017/05/15 19:49:52]

init?

[waffles, 2017/05/16 00:29:02]

Done.

+  int read = file.ReadAtCurrentPos(buffer, kCrx2FileHeaderMagicSize);

[Sorin Jianu, 2017/05/15 19:49:52]

Maybe rename read to num_bytes or something more suggestive.

[Sorin Jianu, 2017/05/15 19:49:52]

const?

[waffles, 2017/05/16 00:29:02]

Acknowledged.

[waffles, 2017/05/16 00:29:02]

Inlined it.

+  if (read != kCrx2FileHeaderMagicSize)
+    return Result::ERROR_HEADER_INVALID;
+  if (!strncmp(buffer, kCrxDiffFileHeaderMagic, kCrx2FileHeaderMagicSize))
+    diff = true;
+  else if (strncmp(buffer, kCrx2FileHeaderMagic, kCrx2FileHeaderMagicSize))
+    return Result::ERROR_HEADER_INVALID;
+  file_hash->Update(buffer, sizeof(buffer));
+
+  // Version number.
+  uint32_t version = ReadAndHashLittleEndianUInt32(&file, file_hash.get());

[Sorin Jianu, 2017/05/15 19:49:52]

const?

[waffles, 2017/05/16 00:29:02]

Done.

+  Result result;
+  if (allow_crx2_ && (version == 2 || (diff && version == 0)))
+    result = VerifyCrx2(&file, file_hash.get());
+  else if (version == 3)
+    result = VerifyCrx3(&file, file_hash.get());
+  else
+    return Result::ERROR_HEADER_INVALID;
+  if (result != Result::OK_FULL)
+    return result;
+
+  // Finalize file hash.
+  uint8_t final_hash[crypto::kSHA256Length];

[Sorin Jianu, 2017/05/15 19:49:52]

init

[waffles, 2017/05/16 00:29:02]

Done.

+  file_hash->Finish(final_hash, sizeof(final_hash));
+  if (!expected_hash_.empty()) {
+    if (expected_hash_.size() != crypto::kSHA256Length)
+      return Result::ERROR_EXPECTED_HASH_INVALID;
+    if (!crypto::SecureMemEqual(final_hash, expected_hash_.data(),
+                                crypto::kSHA256Length))
+      return Result::ERROR_FILE_HASH_FAILED;

[Sorin Jianu, 2017/05/15 19:49:52]

It would be nice if the entire if can be rewritten just as:
if (!crypto::SecureMemEqual(final_hash, expected_hash_.data(),
                                 crypto::kSHA256Length))
       return Result::ERROR_FILE_HASH_FAILED;

[waffles, 2017/05/16 00:29:02]

Acknowledged. Unfortunately we have to tolerate the case that no expected_hash_
was passed, even in the new format.

+  }
+  return diff ? Result::OK_DELTA : Result::OK_FULL;
+}
+
+CrxVerifier::Result CrxVerifier::VerifyCrx3(base::File* file,
+                                            crypto::SecureHash* hash) const {
+  // Crx3 files are not yet supported - treat this as a malformed header.
+  return Result::ERROR_HEADER_INVALID;
+}
+
+CrxVerifier::Result CrxVerifier::VerifyCrx2(base::File* file,
+                                            crypto::SecureHash* hash) const {
+  DCHECK(!require_publisher_proof_);
+  uint32_t key_size = ReadAndHashLittleEndianUInt32(file, hash);

[Sorin Jianu, 2017/05/15 19:49:52]

some const here and below.

[waffles, 2017/05/16 00:29:02]

Done.

+  if (key_size > kMaxPublicKeySize)
+    return Result::ERROR_HEADER_INVALID;
+  uint32_t sig_size = ReadAndHashLittleEndianUInt32(file, hash);
+  if (sig_size > kMaxSignatureSize)
+    return Result::ERROR_HEADER_INVALID;
+  std::vector<uint8_t> key(key_size);
+  if (ReadAndHashBuffer(key.data(), key_size, file, hash) !=
+      static_cast<int>(key_size))
+    return Result::ERROR_HEADER_INVALID;
+  for (const auto& expected_hash : key_hashes_) {
+    // In practice we expect zero or one key_hashes_ for CRX2 files.
+    std::vector<uint8_t> hash(crypto::kSHA256Length);
+    std::unique_ptr<crypto::SecureHash> sha256(

[Sorin Jianu, 2017/05/15 19:49:52]

prefer = syntax

[waffles, 2017/05/16 00:29:02]

Done.

+        crypto::SecureHash::Create(crypto::SecureHash::SHA256));
+    sha256->Update(key.data(), key.size());
+    sha256->Finish(hash.data(), hash.size());
+    if (hash != expected_hash)
+      return Result::ERROR_REQUIRED_PROOF_MISSING;
+  }
+
+  std::vector<uint8_t> sig(sig_size);
+  if (ReadAndHashBuffer(sig.data(), sig_size, file, hash) !=
+      static_cast<int>(sig_size))
+    return Result::ERROR_HEADER_INVALID;
+  crypto::SignatureVerifier verifier;
+  if (!verifier.VerifyInit(crypto::SignatureVerifier::RSA_PKCS1_SHA1,
+                           sig.data(), sig.size(), key.data(), key.size())) {
+    return Result::ERROR_SIGNATURE_INITIALIZATION_FAILED;
+  }
+
+  // Read the rest of the file.
+  uint8_t buffer[1 << 12] = {};
+  size_t len;

[Sorin Jianu, 2017/05/15 19:49:52]

init

[waffles, 2017/05/16 00:29:02]

Done.

+  while ((len = ReadAndHashBuffer(buffer, arraysize(buffer), file, hash)) > 0)
+    verifier.VerifyUpdate(buffer, len);
+  if (!verifier.VerifyFinal())
+    return Result::ERROR_SIGNATURE_VERIFICATION_FAILED;
+
+  std::string public_key_bytes =

[Sorin Jianu, 2017/05/15 19:49:52]

can you use a ctor or initializer instead of initializing with a copy ctor from
a temp object?

[Sorin Jianu, 2017/05/15 19:49:52]

const?

[waffles, 2017/05/16 00:29:02]

Done.

[waffles, 2017/05/16 00:29:02]

Done.

+      std::string(reinterpret_cast<char*>(&key.front()), key.size());
+  if (public_key_)
+    base::Base64Encode(public_key_bytes, public_key_);
+  if (crx_id_)
+    *crx_id_ = id_util::GenerateId(public_key_bytes);
+  return Result::OK_FULL;
+}
+
+}  // namespace crx_file