Allow a test publisher key for extension updates when

--apps-gallery-url is set.

Bug: 966017
Change-Id: Ibdd79784c4e2ed276cdaf78e1135665c7c6d0253
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1648743
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687233}

chrome/browser/extensions/crx_installer.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/lazy_instance.h"
@@ -35,6 +36,7 @@
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/grit/generated_resources.h"
@@ -170,7 +172,9 @@ CrxInstaller::~CrxInstaller() {
 void CrxInstaller::InstallCrx(const base::FilePath& source_file) {
   crx_file::VerifierFormat format =
       off_store_install_allow_reason_ == OffStoreInstallDisallowed
-          ? GetWebstoreVerifierFormat()
+          ? GetWebstoreVerifierFormat(
+                base::CommandLine::ForCurrentProcess()->HasSwitch(
+                    ::switches::kAppsGalleryURL))
           : GetExternalVerifierFormat();
   InstallCrxFile(CRXFileInfo(source_file, format));
 }

components/crx_file/BUILD.gn

@@ -49,6 +49,7 @@ bundle_data("unit_tests_bundle_data") {
     "//components/test/data/crx_file/valid.crx2",
     "//components/test/data/crx_file/valid_no_publisher.crx3",
     "//components/test/data/crx_file/valid_publisher.crx3",
+    "//components/test/data/crx_file/valid_test_publisher.crx3",
   ]
   outputs = [
     "{{bundle_resources_dir}}/" +

components/crx_file/crx_verifier.cc

@@ -38,12 +38,18 @@ constexpr uint32_t kMaxSignatureSize = 1 << 16;
 // The maximum size the Crx3 parser will tolerate for a header.
 constexpr uint32_t kMaxHeaderSize = 1 << 18;
 
-// The SHA256 hash of the "ecdsa_2017_public" Crx3 key.
+// The SHA256 hash of the DER SPKI "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};
 
+// The SHA256 hash of the DER SPKI "ecdsa_2017_public" Crx3 test key.
+constexpr uint8_t kPublisherTestKeyHash[] = {
+    0x6c, 0x46, 0x41, 0x3b, 0x00, 0xd0, 0xfa, 0x0e, 0x72, 0xc8, 0xd2,
+    0x5f, 0x64, 0xf3, 0xa6, 0x17, 0x03, 0x0d, 0xde, 0x21, 0x61, 0xbe,
+    0xb7, 0x95, 0x91, 0x95, 0x83, 0x68, 0x12, 0xe9, 0x78, 0x1e};
+
 using VerifierCollection =
     std::vector<std::unique_ptr<crypto::SignatureVerifier>>;
 using RepeatedProof = google::protobuf::RepeatedPtrField<AsymmetricKeyProof>;
@@ -98,7 +104,8 @@ VerifierResult VerifyCrx3(
     const std::vector<std::vector<uint8_t>>& required_key_hashes,
     std::string* public_key,
     std::string* crx_id,
-    bool require_publisher_key) {
+    bool require_publisher_key,
+    bool accept_publisher_test_key) {
   // Parse [header-size] and [header].
   const uint32_t header_size = ReadAndHashLittleEndianUInt32(file, hash);
   if (header_size > kMaxHeaderSize)
@@ -130,10 +137,6 @@ VerifierResult VerifyCrx3(
   // 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) {
-    required_key_set.emplace(std::begin(kPublisherKeyHash),
-                             std::end(kPublisherKeyHash));
-  }
 
   using ProofFetcher = const RepeatedProof& (CrxFileHeader::*)() const;
   ProofFetcher rsa = &CrxFileHeader::sha256_with_rsa;
@@ -149,6 +152,15 @@ VerifierResult VerifyCrx3(
           std::make_pair(rsa, crypto::SignatureVerifier::RSA_PKCS1_SHA256),
           std::make_pair(ecdsa, crypto::SignatureVerifier::ECDSA_SHA256)};
 
+  std::vector<uint8_t> publisher_key(std::begin(kPublisherKeyHash),
+                                     std::end(kPublisherKeyHash));
+  base::Optional<std::vector<uint8_t>> publisher_test_key;
+  if (accept_publisher_test_key) {
+    publisher_test_key.emplace(std::begin(kPublisherTestKeyHash),
+                               std::end(kPublisherTestKeyHash));
+  }
+  bool found_publisher_key = false;
+
   // 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
@@ -162,6 +174,10 @@ VerifierResult VerifyCrx3(
       std::vector<uint8_t> key_hash(crypto::kSHA256Length);
       crypto::SHA256HashString(key, key_hash.data(), key_hash.size());
       required_key_set.erase(key_hash);
+      DCHECK_EQ(accept_publisher_test_key, publisher_test_key.has_value());
+      found_publisher_key =
+          found_publisher_key || key_hash == publisher_key ||
+          (accept_publisher_test_key && key_hash == *publisher_test_key);
       auto v = std::make_unique<crypto::SignatureVerifier>();
       static_assert(sizeof(unsigned char) == sizeof(uint8_t),
                     "Unsupported char size.");
@@ -178,6 +194,9 @@ VerifierResult VerifyCrx3(
   if (public_key_bytes.empty() || !required_key_set.empty())
     return VerifierResult::ERROR_REQUIRED_PROOF_MISSING;
 
+  if (require_publisher_key && !found_publisher_key)
+    return VerifierResult::ERROR_REQUIRED_PROOF_MISSING;
+
   // Update and finalize the verifiers with [archive].
   if (!ReadHashAndVerifyArchive(file, hash, verifiers))
     return VerifierResult::ERROR_SIGNATURE_VERIFICATION_FAILED;
@@ -273,15 +292,20 @@ VerifierResult Verify(
                  << "' is in CRX2 format, which is deprecated and will not be "
                     "supported in M78+";
   if (format == VerifierFormat::CRX2_OR_CRX3 &&
-      (version == 2 || (diff && version == 0)))
+      (version == 2 || (diff && version == 0))) {
     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_local, &crx_id_local,
-                        format == VerifierFormat::CRX3_WITH_PUBLISHER_PROOF);
-  else
+  } else if (version == 3) {
+    bool require_publisher_key =
+        format == VerifierFormat::CRX3_WITH_PUBLISHER_PROOF ||
+        format == VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF;
+    result =
+        VerifyCrx3(&file, file_hash.get(), required_key_hashes,
+                   &public_key_local, &crx_id_local, require_publisher_key,
+                   format == VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF);
+  } else {
     result = VerifierResult::ERROR_HEADER_INVALID;
+  }
   if (result != VerifierResult::OK_FULL)
     return result;
 

components/crx_file/crx_verifier.h

@@ -16,9 +16,12 @@ class FilePath;
 namespace crx_file {
 
 enum class VerifierFormat {
-  CRX2_OR_CRX3,               // Accept Crx2 or Crx3.
-  CRX3,                       // Accept only Crx3.
-  CRX3_WITH_PUBLISHER_PROOF,  // Accept only Crx3 with a publisher proof.
+  CRX2_OR_CRX3,                    // Accept Crx2 or Crx3.
+  CRX3,                            // Accept only Crx3.
+  CRX3_WITH_TEST_PUBLISHER_PROOF,  // Accept only Crx3 with a test or production
+                                   // publisher proof.
+  CRX3_WITH_PUBLISHER_PROOF,       // Accept only Crx3 with a production
+                                   // publisher proof.
 };
 
 enum class VerifierResult {

components/crx_file/crx_verifier_unittest.cc

@@ -32,6 +32,17 @@ constexpr char kOjjKey[] =
     "mCsHBHFWbqtGhSN4YCAw3DYQzwdTcIVaIA8f2Uo4AZ4INKkrEPRL8o9mZDYtO2YHIQg8pMSRMa"
     "6AawBNYi9tZScnmgl5L1qE6z5oIwIDAQAB";
 
+constexpr char kJlnHash[] = "jlnmailbicnpnbggmhfebbomaddckncf";
+constexpr char kJlnKey[] =
+    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAtYd4M8wBjlPsc/wxS1/uXKMD6GtI7/"
+    "0GLxRe6UXTYtDk91u/"
+    "FdJRK9jHws+"
+    "FO6Yi3XcJGtMvuojiB1j4bdiYBfvRgfTD4b7krWtWM2udKPBtHI9ikAT5aom5Bda8rCPNyaqXC"
+    "6Ax+KTgQpeeJglYu7TTd/"
+    "AePyvlRHtCKNkcvRQLY0b6hccALqoTzyTueDX12c8Htg76syEPbz7hSIPPfq6KEGvuVSxWAejy"
+    "/y6EhwAdXRLpegul9KmL94OY1G6dpycUKwyKeXOcB6Qj5iKNcOqJAaSLxoOZby4G3cI1BcQpp/"
+    "3vYccJ4qouDMfaanLe8CvFlLp4VOn833aJ8PYpLQIDAQAB";
+
 }  // namespace
 
 namespace crx_file {
@@ -162,6 +173,15 @@ TEST_F(CrxVerifierTest, ChecksPinnedKey) {
   EXPECT_EQ(std::string(kOjjHash), crx_id);
   EXPECT_EQ(std::string(kOjjKey), public_key);
 
+  public_key = "UNSET";
+  crx_id = "UNSET";
+  EXPECT_EQ(VerifierResult::ERROR_REQUIRED_PROOF_MISSING,
+            Verify(TestFile("valid_test_publisher.crx3"),
+                   VerifierFormat::CRX3_WITH_PUBLISHER_PROOF, keys, hash,
+                   &public_key, &crx_id));
+  EXPECT_EQ("UNSET", crx_id);
+  EXPECT_EQ("UNSET", public_key);
+
   public_key = "UNSET";
   crx_id = "UNSET";
   EXPECT_EQ(VerifierResult::ERROR_REQUIRED_PROOF_MISSING,
@@ -172,6 +192,37 @@ TEST_F(CrxVerifierTest, ChecksPinnedKey) {
   EXPECT_EQ("UNSET", public_key);
 }
 
+TEST_F(CrxVerifierTest, ChecksPinnedKeyAcceptsTest) {
+  const std::vector<uint8_t> hash;
+  const std::vector<std::vector<uint8_t>> keys;
+  std::string public_key = "UNSET";
+  std::string crx_id = "UNSET";
+  EXPECT_EQ(VerifierResult::OK_FULL,
+            Verify(TestFile("valid_publisher.crx3"),
+                   VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF, keys, hash,
+                   &public_key, &crx_id));
+  EXPECT_EQ(std::string(kOjjHash), crx_id);
+  EXPECT_EQ(std::string(kOjjKey), public_key);
+
+  public_key = "UNSET";
+  crx_id = "UNSET";
+  EXPECT_EQ(VerifierResult::OK_FULL,
+            Verify(TestFile("valid_test_publisher.crx3"),
+                   VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF, keys, hash,
+                   &public_key, &crx_id));
+  EXPECT_EQ(std::string(kJlnHash), crx_id);
+  EXPECT_EQ(std::string(kJlnKey), public_key);
+
+  public_key = "UNSET";
+  crx_id = "UNSET";
+  EXPECT_EQ(VerifierResult::ERROR_REQUIRED_PROOF_MISSING,
+            Verify(TestFile("valid_no_publisher.crx3"),
+                   VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF, keys, hash,
+                   &public_key, &crx_id));
+  EXPECT_EQ("UNSET", crx_id);
+  EXPECT_EQ("UNSET", public_key);
+}
+
 TEST_F(CrxVerifierTest, NullptrSafe) {
   const std::vector<uint8_t> hash;
   const std::vector<std::vector<uint8_t>> keys;

extensions/browser/updater/extension_downloader.cc

@@ -954,7 +954,7 @@ void ExtensionDownloader::NotifyDelegateDownloadFinished(
   const std::set<int>& request_ids = fetch_data->request_ids;
   const crx_file::VerifierFormat required_format =
       extension_urls::IsWebstoreUpdateUrl(fetch_data->url)
-          ? GetWebstoreVerifierFormat()
+          ? GetWebstoreVerifierFormat(false)
           : crx_format_requirement_;
   delegate_->OnExtensionDownloadFinished(
       CRXFileInfo(id, crx_path, package_hash, required_format),

extensions/browser/updater/update_data_provider.cc

@@ -106,7 +106,7 @@ UpdateDataProvider::GetData(bool install_immediately,
     crx_component->requires_network_encryption = true;
     crx_component->crx_format_requirement =
         extension->from_webstore()
-            ? GetWebstoreVerifierFormat()
+            ? GetWebstoreVerifierFormat(false)
             : GetPolicyVerifierFormat(
                   extension_prefs->InsecureExtensionUpdatesEnabled());
     crx_component->installer = base::MakeRefCounted<ExtensionInstaller>(

extensions/common/verifier_formats.cc

@@ -2,12 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "extensions/common/verifier_formats.h"
+
 #include "components/crx_file/crx_verifier.h"
 
 namespace extensions {
 
-crx_file::VerifierFormat GetWebstoreVerifierFormat() {
-  return crx_file::VerifierFormat::CRX3_WITH_PUBLISHER_PROOF;
+crx_file::VerifierFormat GetWebstoreVerifierFormat(
+    bool test_publisher_enabled) {
+  return test_publisher_enabled
+             ? crx_file::VerifierFormat::CRX3_WITH_TEST_PUBLISHER_PROOF
+             : crx_file::VerifierFormat::CRX3_WITH_PUBLISHER_PROOF;
 }
 
 crx_file::VerifierFormat GetPolicyVerifierFormat(

extensions/common/verifier_formats.h

@@ -12,8 +12,9 @@ enum class VerifierFormat;
 namespace extensions {
 
 // Returns the default format requirement for installing an extension that
-// originates or updates from the Webstore.
-crx_file::VerifierFormat GetWebstoreVerifierFormat();
+// originates or updates from the Webstore. |test_publisher_enabled| indicates
+// whether items from a test instance of Webstore are permitted.
+crx_file::VerifierFormat GetWebstoreVerifierFormat(bool test_publisher_enabled);
 
 // Returns the default format requirement for installing an extension that
 // is force-installed by policy. |insecure_updates_enabled| indicates