Expand CRX verifier to verify 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
index 21a053cec1d502cda88802761e193bb9bf793fe8..0c9e021e9de816a9f8054d5e1d6ff9a921850efb 100644
--- a/components/crx_file/crx_verifier.cc
+++ b/components/crx_file/crx_verifier.cc
@@ -5,13 +5,20 @@
 #include "components/crx_file/crx_verifier.h"
 
 #include <cstring>
+#include <iterator>
 #include <memory>
+#include <set>
+#include <utility>
 
 #include "base/base64.h"
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "components/crx_file/crx2_file.h"
+#include "components/crx_file/crx3.pb.h"
 #include "components/crx_file/id_util.h"
 #include "crypto/secure_hash.h"
 #include "crypto/secure_util.h"
@@ -22,12 +29,28 @@ namespace crx_file {
 
 namespace {
 
-// The maximum size the crx2 parser will tolerate for a public key.
+// The maximum size the Crx2 parser will tolerate for a public key.
 constexpr uint32_t kMaxPublicKeySize = 1 << 16;
 
-// The maximum size the crx2 parser will tolerate for a signature.
+// The maximum size the Crx2 parser will tolerate for a signature.
 constexpr uint32_t kMaxSignatureSize = 1 << 16;
 
+// The maximum size the Crx3 parser will tolerate for a header.
+constexpr uint32_t kMaxHeaderSize = 1 << 18;
+
+// The context for Crx3 signing, encoded in UTF8.
+constexpr unsigned char kSignatureContext[] = u8"CRX3 SignedData";
+
+// The SHA256 hash of the "ecdsa_2017_public" Crx3 key.
+constexpr uint8_t kPublisherKeyHash[] = {
+    0x61, 0xf7, 0xf2, 0xa6, 0xbf, 0xcf, 0x74, 0xcd, 0x0b, 0xc1, 0xfe,
+    0x24, 0x97, 0xcc, 0x9b, 0x04, 0x25, 0x4c, 0x65, 0x8f, 0x79, 0xf2,
+    0x14, 0x53, 0x92, 0x86, 0x7e, 0xa8, 0x36, 0x63, 0x67, 0xcf};
+
+using VerifierCollection =
+    std::vector<std::unique_ptr<crypto::SignatureVerifier>>;
+using RepeatedProof = google::protobuf::RepeatedPtrField<AsymmetricKeyProof>;
+
 int ReadAndHashBuffer(uint8_t* buffer,
                       int length,
                       base::File* file,
@@ -48,14 +71,124 @@ uint32_t ReadAndHashLittleEndianUInt32(base::File* file,
   return buffer[3] << 24 | buffer[2] << 16 | buffer[1] << 8 | buffer[0];
 }
 
+bool ReadHashAndVerifyArchive(base::File* file,

[Devlin, 2017/05/23 20:32:40]

add a function comment

[waffles, 2017/05/23 23:05:13]

Done.

+                              crypto::SecureHash* hash,
+                              const VerifierCollection& verifiers) {
+  uint8_t buffer[1 << 12] = {};

[Devlin, 2017/05/23 20:32:40]

Comment why 1 << 12?

[waffles, 2017/05/23 23:05:13]

I don't know what to write in the comment. This constant was inherited from the
previous code, which predated my contributions to this project. Do you have a
recommendation for a different constant?

[Devlin, 2017/05/24 17:10:54]

Apparently this was added 8 years ago here:
https://codereview.chromium.org/126014

I still have no idea where the 1 << 12 came from, but it seems to have been
working, so I guess it's fine.

+  size_t len = 0;
+  while ((len = ReadAndHashBuffer(buffer, arraysize(buffer), file, hash)) > 0) {
+    for (auto& verifier : verifiers)
+      verifier->VerifyUpdate(buffer, len);
+  }
+  for (auto& verifier : verifiers) {
+    if (!verifier->VerifyFinal())
+      return false;
+  }
+  return true;
+}
+
+// The remaining contents of a Crx3 file are [header-size][header][archive].
+// [header] is an encoded protocol buffer and contains both a signed and
+// unsigned section. The unsigned section contains a set of key/signature pairs,
+// and the signed section is the encoding of another protocol buffer. All
+// signatures cover [prefix][signed-header-size][signed-header][archive].
 VerifierResult VerifyCrx3(
     base::File* file,
     crypto::SecureHash* hash,
     const std::vector<std::vector<uint8_t>>& required_key_hashes,
     std::string* public_key,
-    std::string* crx_id) {
-  // Crx3 files are not yet supported - treat this as a malformed header.
-  return VerifierResult::ERROR_HEADER_INVALID;
+    std::string* crx_id,
+    bool require_publisher_key) {
+  // Parse [header-size] and [header].
+  const uint32_t header_size = ReadAndHashLittleEndianUInt32(file, hash);
+  if (header_size > kMaxHeaderSize)
+    return VerifierResult::ERROR_HEADER_INVALID;
+  std::vector<uint8_t> header_bytes(header_size);
+  if (ReadAndHashBuffer(header_bytes.data(), header_size, file, hash) !=
+      static_cast<int>(header_size))

[Devlin, 2017/05/23 20:32:40]

any risk of overflow?

[waffles, 2017/05/23 23:05:13]

I don't think we can compile Chrome on any machine where int is less than 19
bits. Let me know if you disagree.

[Devlin, 2017/05/24 17:10:54]

Ah, missed the check on line 105.  I wonder if this would be clearer with
something like:
if (header_size > kMaxHeaderSize)
  return VerifierResult::ERROR_HEADER_INVALID;
// This cast is safe because of the range check above. 
int header_size_int = static_cast<int>(header_size);

That would also save us from doing the cast in ReadAndHashBuffer().

[waffles, 2017/05/24 17:53:07]

I added a comment. (I hate introducing names... >_>) It would be nice to instead
write a static_assert that kMaxHeaderSize can fit in an "int", but I'm not sure
how to safely write that assert.
(The cast in RAHB is on the buffer, not the length, but maybe I misunderstand
what you mean.)

+    return VerifierResult::ERROR_HEADER_INVALID;
+  CrxFileHeader header;
+  if (!header.ParseFromArray(header_bytes.data(), header_size))
+    return VerifierResult::ERROR_HEADER_INVALID;
+
+  // Parse [signed-header].
+  const std::string& signed_header_data_str = header.signed_header_data();
+  SignedData signed_header_data;
+  if (!signed_header_data.ParseFromString(signed_header_data_str))
+    return VerifierResult::ERROR_HEADER_INVALID;
+  const std::string& crx_id_encoded = signed_header_data.crx_id();
+  const std::string declared_crx_id = id_util::GenerateIdFromHex(
+      base::HexEncode(crx_id_encoded.data(), crx_id_encoded.size()));
+
+  // Create a little-endian representation of [signed-header-size].
+  const int signed_header_size = signed_header_data_str.size();
+  const uint8_t header_size_octets[] = {
+      signed_header_size, signed_header_size >> 8, signed_header_size >> 16,
+      signed_header_size >> 24};
+
+  // Create a set of all required key hashes.
+  std::set<std::vector<uint8_t>> required_key_set(required_key_hashes.begin(),
+                                                  required_key_hashes.end());
+  if (require_publisher_key) {
+    const std::vector<uint8_t> publisher_key(std::begin(kPublisherKeyHash),
+                                             std::end(kPublisherKeyHash));
+    required_key_set.insert(publisher_key);

[Devlin, 2017/05/23 20:32:40]

use emplace?
required_key_set.emplace(std::begin(kPublisherKeyHash),
std::end(kPublisherKeyHash));

Or use std::move(), if you prefer.

[waffles, 2017/05/23 23:05:13]

Thanks, done.

+  }
+  std::string public_key_bytes;
+  VerifierCollection verifiers;
+  const std::vector<std::pair<std::function<const RepeatedProof&()>,

[Devlin, 2017/05/23 20:32:40]

std::function is banned in Chromium.  Can you use base::Callback<> here?

https://chromium-cpp.appspot.com

[waffles, 2017/05/23 23:05:13]

Hacked on this for a while - does this look better, or do you still prefer the
use of Callback?

[Devlin, 2017/05/24 17:10:54]

This is fine.  I think base::Callback is mildly more readable, but probably not
worth the overhead.

[waffles, 2017/05/24 17:53:07]

Great. Yeah, I started to use base::Callback, but I had to have a static_cast to
the full function signature anyways (because the function is overloaded), and at
that point I decided there was no need to wrap in callbacks, since I needed the
other less-readable lines anyways...

+                              crypto::SignatureVerifier::SignatureAlgorithm>>
+      proof_types = {std::make_pair(
+                         [&header]() -> const RepeatedProof& {
+                           return header.sha256_with_rsa();
+                         },
+                         crypto::SignatureVerifier::RSA_PKCS1_SHA256),
+                     std::make_pair(
+                         [&header]() -> const RepeatedProof& {
+                           return header.sha256_with_ecdsa();
+                         },
+                         crypto::SignatureVerifier::ECDSA_SHA256)};
+
+  // Initialize all verifiers and update them with
+  // [prefix][signed-header-size][signed-header].
+  // Clear any elements of required_key_set that are encountered, and watch for
+  // the developer key.
+  for (const auto& proof_type : proof_types) {
+    for (const auto& proof : proof_type.first()) {
+      const std::string& key = proof.public_key();
+      const std::string& sig = proof.signature();
+      if (id_util::GenerateId(key) == declared_crx_id)
+        public_key_bytes = key;
+      std::vector<uint8_t> key_hash(crypto::kSHA256Length);
+      crypto::SHA256HashString(key, key_hash.data(), key_hash.size());
+      required_key_set.erase(key_hash);
+      std::unique_ptr<crypto::SignatureVerifier> v =

[Devlin, 2017/05/23 20:32:40]

optional: you could use auto here since base::MakeUnique<T> is defined to return
a std::unique_ptr<T>.

[waffles, 2017/05/23 23:05:13]

Done.

+          base::MakeUnique<crypto::SignatureVerifier>();
+      static_assert(sizeof(unsigned char) == sizeof(uint8_t),
+                    "Unsupported char size.");
+      if (!v->VerifyInit(
+              proof_type.second, reinterpret_cast<const uint8_t*>(sig.data()),
+              sig.size(), reinterpret_cast<const uint8_t*>(key.data()),
+              key.size()))
+        return VerifierResult::ERROR_SIGNATURE_INITIALIZATION_FAILED;
+      v->VerifyUpdate(kSignatureContext, arraysize(kSignatureContext));
+      v->VerifyUpdate(header_size_octets, arraysize(header_size_octets));
+      v->VerifyUpdate(
+          reinterpret_cast<const uint8_t*>(signed_header_data_str.data()),
+          signed_header_data_str.size());
+      verifiers.push_back(std::move(v));

[Devlin, 2017/05/23 20:32:40]

can we reserve() verifiers?

[waffles, 2017/05/23 23:05:13]

Done.

+    }
+  }
+  if (public_key_bytes.empty() || !required_key_set.empty())
+    return VerifierResult::ERROR_REQUIRED_PROOF_MISSING;
+
+  // Update and finalize the verifiers with [archive].
+  if (!ReadHashAndVerifyArchive(file, hash, verifiers))
+    return VerifierResult::ERROR_SIGNATURE_VERIFICATION_FAILED;
+
+  base::Base64Encode(public_key_bytes, public_key);
+  *crx_id = declared_crx_id;
+  return VerifierResult::OK_FULL;
 }
 
 VerifierResult VerifyCrx2(
@@ -75,7 +208,7 @@ VerifierResult VerifyCrx2(
       static_cast<int>(key_size))
     return VerifierResult::ERROR_HEADER_INVALID;
   for (const auto& expected_hash : required_key_hashes) {
-    // In practice we expect zero or one key_hashes_ for CRX2 files.
+    // 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 =
         crypto::SecureHash::Create(crypto::SecureHash::SHA256);
@@ -89,25 +222,20 @@ VerifierResult VerifyCrx2(
   if (ReadAndHashBuffer(sig.data(), sig_size, file, hash) !=
       static_cast<int>(sig_size))
     return VerifierResult::ERROR_HEADER_INVALID;
-  crypto::SignatureVerifier verifier;
-  if (!verifier.VerifyInit(crypto::SignatureVerifier::RSA_PKCS1_SHA1,
-                           sig.data(), sig.size(), key.data(), key.size())) {
+  std::vector<std::unique_ptr<crypto::SignatureVerifier>> verifiers;
+  verifiers.push_back(base::MakeUnique<crypto::SignatureVerifier>());
+  if (!verifiers[0]->VerifyInit(crypto::SignatureVerifier::RSA_PKCS1_SHA1,
+                                sig.data(), sig.size(), key.data(),
+                                key.size())) {
     return VerifierResult::ERROR_SIGNATURE_INITIALIZATION_FAILED;
   }
 
-  // Read the rest of the file.
-  uint8_t buffer[1 << 12] = {};
-  size_t len = 0;
-  while ((len = ReadAndHashBuffer(buffer, arraysize(buffer), file, hash)) > 0)
-    verifier.VerifyUpdate(buffer, len);
-  if (!verifier.VerifyFinal())
+  if (!ReadHashAndVerifyArchive(file, hash, verifiers))
     return VerifierResult::ERROR_SIGNATURE_VERIFICATION_FAILED;
 
   const std::string public_key_bytes(key.begin(), key.end());
-  if (public_key)
-    base::Base64Encode(public_key_bytes, public_key);
-  if (crx_id)
-    *crx_id = id_util::GenerateId(public_key_bytes);
+  base::Base64Encode(public_key_bytes, public_key);
+  *crx_id = id_util::GenerateId(public_key_bytes);
   return VerifierResult::OK_FULL;
 }
 
@@ -120,6 +248,8 @@ VerifierResult Verify(
     const std::vector<uint8_t>& required_file_hash,
     std::string* public_key,
     std::string* crx_id) {
+  std::string public_key_local;
+  std::string crx_id_local;
   base::File file(crx_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
   if (!file.IsValid())
     return VerifierResult::ERROR_FILE_NOT_READABLE;
@@ -145,11 +275,12 @@ VerifierResult Verify(
   VerifierResult result;
   if (format == VerifierFormat::CRX2_OR_CRX3 &&
       (version == 2 || (diff && version == 0)))
-    result = VerifyCrx2(&file, file_hash.get(), required_key_hashes, public_key,
-                        crx_id);
+    result = VerifyCrx2(&file, file_hash.get(), required_key_hashes,
+                        &public_key_local, &crx_id_local);
   else if (version == 3)
-    result = VerifyCrx3(&file, file_hash.get(), required_key_hashes, public_key,
-                        crx_id);
+    result = VerifyCrx3(&file, file_hash.get(), required_key_hashes,
+                        &public_key_local, &crx_id_local,
+                        format == VerifierFormat::CRX3_WITH_PUBLISHER_PROOF);
   else
     result = VerifierResult::ERROR_HEADER_INVALID;
   if (result != VerifierResult::OK_FULL)
@@ -165,6 +296,12 @@ VerifierResult Verify(
                                 crypto::kSHA256Length))
       return VerifierResult::ERROR_FILE_HASH_FAILED;
   }
+
+  // All is well. Set the out-params and return.
+  if (public_key)
+    *public_key = public_key_local;
+  if (crx_id)
+    *crx_id = crx_id_local;
   return diff ? VerifierResult::OK_DELTA : VerifierResult::OK_FULL;
 }