Always verify Variations payloads.

On Android and iOS platforms, we do not currently pin TLS
connections, so verifying Variations payloads is important
to ensure that the data has not been tampered with in transit
from Google to the device.

Such verification of these payloads on mobile devices
previously added 25ms to process startup; modern mobile devices
are much quicker and we do not anticipate such an impact but we
will assess on the perf bots.

Unfortunately we can't delete the SignatureVerificationEnabled()
call entirely because it's still needed in order to disable
signature verification for unit tests. This would be hard to solve
because unit tests would need to simulate the real signing
procedure of the variations servers, using their private key.
This may be worth further investigation in future (e.g. using
a different key pair for unit tests) as it would allow removal
of quite a bit of production code.

Change-Id: I9cb89750e100bf7204140920583db8bd0fa0f41a
Bug: 1078056
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2181564
Commit-Queue: Adrian Taylor <adetaylor@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770744}

android_webview/browser/aw_feature_list_creator.cc

@@ -165,7 +165,8 @@ void AwFeatureListCreator::SetUpFieldTrials() {
   client_ = std::make_unique<AwVariationsServiceClient>();
   auto seed_store = std::make_unique<variations::VariationsSeedStore>(
       local_state_.get(), /*initial_seed=*/std::move(seed),
-      /*on_initial_seed_stored=*/base::DoNothing());
+      /*on_initial_seed_stored=*/base::DoNothing(),
+      /*signature_verification_enabled=*/true);
 
   // We set the seed fetch time to when the service downloaded the seed rather
   // than base::Time::Now() because we want to compute seed freshness based on

components/variations/service/variations_field_trial_creator_unittest.cc

@@ -487,7 +487,8 @@ TEST_F(FieldTrialCreatorTest, SetupFieldTrials_LoadsCountryOnFirstRun) {
   // the interaction between these two classes is what's being tested.
   auto seed_store = std::make_unique<VariationsSeedStore>(
       &prefs_, std::move(initial_seed),
-      /*on_initial_seed_stored=*/base::DoNothing());
+      /*on_initial_seed_stored=*/base::DoNothing(),
+      /*signature_verification_enabled=*/false);
   VariationsFieldTrialCreator field_trial_creator(
       &prefs_, &variations_service_client, std::move(seed_store),
       UIStringOverrider());

components/variations/service/variations_service.cc

@@ -377,7 +377,8 @@ VariationsService::VariationsService(
                            std::make_unique<VariationsSeedStore>(
                                local_state,
                                MaybeImportFirstRunSeed(local_state),
-                               base::BindOnce(&OnInitialSeedStored)),
+                               base::BindOnce(&OnInitialSeedStored),
+                               /*signature_verification_enabled=*/true),
                            ui_string_overrider),
       last_request_was_http_retry_(false) {
   DCHECK(client_);
@@ -699,15 +700,13 @@ bool VariationsService::StoreSeed(const std::string& seed_data,
                                   const std::string& country_code,
                                   base::Time date_fetched,
                                   bool is_delta_compressed,
-                                  bool is_gzip_compressed,
-                                  bool fetched_insecurely) {
+                                  bool is_gzip_compressed) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   std::unique_ptr<VariationsSeed> seed(new VariationsSeed);
   if (!field_trial_creator_.seed_store()->StoreSeedData(
           seed_data, seed_signature, country_code, date_fetched,
-          is_delta_compressed, is_gzip_compressed, fetched_insecurely,
-          seed.get())) {
+          is_delta_compressed, is_gzip_compressed, seed.get())) {
     return false;
   }
 
@@ -810,19 +809,12 @@ void VariationsService::OnSimpleLoaderCompleteOrRedirect(
   scoped_refptr<net::HttpResponseHeaders> headers;
 
   int response_code = -1;
-  // We use the final URL HTTPS/HTTP value to pass to StoreSeed, since
-  // signature validation should be forced on for any HTTP fetch, including
-  // redirects from HTTPS to HTTP. We default to false since we can't get this
-  // value (nor will it be used) in the redirect case.
-  bool final_url_was_https = false;
 
   // Variations seed fetches should not follow redirects, so if this request was
   // redirected, keep the default values for |net_error| and |is_success| (treat
   // it as a net::ERR_INVALID_REDIRECT), and the fetch will be cancelled when
   // pending_seed_request is reset.
   if (!was_redirect) {
-    final_url_was_https =
-        pending_seed_request_->GetFinalURL().SchemeIs(url::kHttpsScheme);
     const network::mojom::URLResponseHead* response_info =
         pending_seed_request_->ResponseInfo();
     if (response_info && response_info->headers) {
@@ -919,7 +911,7 @@ void VariationsService::OnSimpleLoaderCompleteOrRedirect(
   const std::string country_code = GetHeaderValue(headers.get(), "X-Country");
   const bool store_success =
       StoreSeed(*response_body, signature, country_code, response_date,
-                is_delta_compressed, is_gzip_compressed, !final_url_was_https);
+                is_delta_compressed, is_gzip_compressed);
   if (!store_success && is_delta_compressed) {
     disable_deltas_for_next_request_ = true;
     // |request_scheduler_| will be null during unit tests.

components/variations/service/variations_service.h

@@ -235,8 +235,7 @@ class VariationsService
                          const std::string& country_code,
                          base::Time date_fetched,
                          bool is_delta_compressed,
-                         bool is_gzip_compressed,
-                         bool fetched_insecurely);
+                         bool is_gzip_compressed);
 
   // Create an entropy provider based on low entropy. This is used to create
   // trials for studies that should only depend on low entropy, such as studies

components/variations/service/variations_service_unittest.cc

@@ -136,8 +136,7 @@ class TestVariationsService : public VariationsService {
         fetch_attempted_(false),
         seed_stored_(false),
         delta_compressed_seed_(false),
-        gzip_compressed_seed_(false),
-        insecurely_fetched_seed_(false) {
+        gzip_compressed_seed_(false) {
     interception_url_ =
         GetVariationsServerURL(use_secure_url ? USE_HTTPS : USE_HTTP);
     set_variations_server_url(interception_url_);
@@ -160,7 +159,6 @@ class TestVariationsService : public VariationsService {
   const std::string& stored_country() const { return stored_country_; }
   bool delta_compressed_seed() const { return delta_compressed_seed_; }
   bool gzip_compressed_seed() const { return gzip_compressed_seed_; }
-  bool insecurely_fetched_seed() const { return insecurely_fetched_seed_; }
 
   bool CallMaybeRetryOverHTTP() { return CallMaybeRetryOverHTTPForTesting(); }
 
@@ -188,14 +186,12 @@ class TestVariationsService : public VariationsService {
                  const std::string& country_code,
                  base::Time date_fetched,
                  bool is_delta_compressed,
-                 bool is_gzip_compressed,
-                 bool fetched_insecurely) override {
+                 bool is_gzip_compressed) override {
     seed_stored_ = true;
     stored_seed_data_ = seed_data;
     stored_country_ = country_code;
     delta_compressed_seed_ = is_delta_compressed;
     gzip_compressed_seed_ = is_gzip_compressed;
-    insecurely_fetched_seed_ = fetched_insecurely;
     RecordSuccessfulFetch();
     return true;
   }
@@ -223,7 +219,6 @@ class TestVariationsService : public VariationsService {
   std::string stored_country_;
   bool delta_compressed_seed_;
   bool gzip_compressed_seed_;
-  bool insecurely_fetched_seed_;
 
   DISALLOW_COPY_AND_ASSIGN(TestVariationsService);
 };
@@ -944,43 +939,6 @@ TEST_F(VariationsServiceTest, FieldTrialCreatorInitializedCorrectly) {
   service.GetClientFilterableStateForVersionCalledForTesting();
 }
 
-TEST_F(VariationsServiceTest, InsecurelyFetchedSetWhenHTTP) {
-  std::string serialized_seed = SerializeSeed(CreateTestSeed());
-  VariationsService::EnableFetchForTesting();
-  TestVariationsService service(
-      std::make_unique<web_resource::TestRequestAllowedNotifier>(
-          &prefs_, network_tracker_),
-      &prefs_, GetMetricsStateManager(), false);
-  service.set_intercepts_fetch(false);
-  service.test_url_loader_factory()->AddResponse(
-      service.interception_url().spec(), serialized_seed);
-  base::HistogramTester histogram_tester;
-  // Note: We call DoFetchFromURL() here instead of DoActualFetch() since the
-  // latter doesn't pass true to |http_retry|.
-  service.DoFetchFromURL(service.interception_url(), true);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(service.insecurely_fetched_seed());
-  histogram_tester.ExpectUniqueSample(
-      "Variations.SeedFetchResponseOrErrorCode.HTTP", net::HTTP_OK, 1);
-}
-
-TEST_F(VariationsServiceTest, InsecurelyFetchedNotSetWhenHTTPS) {
-  std::string serialized_seed = SerializeSeed(CreateTestSeed());
-  TestVariationsService service(
-      std::make_unique<web_resource::TestRequestAllowedNotifier>(
-          &prefs_, network_tracker_),
-      &prefs_, GetMetricsStateManager(), true);
-  VariationsService::EnableFetchForTesting();
-  service.set_intercepts_fetch(false);
-  service.test_url_loader_factory()->AddResponse(
-      service.interception_url().spec(), serialized_seed);
-  base::HistogramTester histogram_tester;
-  service.DoActualFetch();
-  EXPECT_FALSE(service.insecurely_fetched_seed());
-  histogram_tester.ExpectUniqueSample("Variations.SeedFetchResponseOrErrorCode",
-                                      net::HTTP_OK, 1);
-}
-
 TEST_F(VariationsServiceTest, RetryOverHTTPIfURLisSet) {
   TestVariationsService service(
       std::make_unique<web_resource::TestRequestAllowedNotifier>(

components/variations/variations_seed_store.cc

@@ -112,22 +112,23 @@ UpdateSeedDateResult GetSeedDateChangeState(
 }  // namespace
 
 VariationsSeedStore::VariationsSeedStore(PrefService* local_state)
-    : VariationsSeedStore(local_state, nullptr, base::DoNothing()) {}
+    : VariationsSeedStore(local_state, nullptr, base::DoNothing(), true) {}
 
 VariationsSeedStore::VariationsSeedStore(
     PrefService* local_state,
     std::unique_ptr<SeedResponse> initial_seed,
-    base::OnceCallback<void()> on_initial_seed_stored)
+    base::OnceCallback<void()> on_initial_seed_stored,
+    bool signature_verification_enabled)
     : local_state_(local_state),
-      on_initial_seed_stored_(std::move(on_initial_seed_stored)) {
+      on_initial_seed_stored_(std::move(on_initial_seed_stored)),
+      signature_verification_enabled_(signature_verification_enabled) {
 #if defined(OS_ANDROID)
   if (initial_seed)
     ImportInitialSeed(std::move(initial_seed));
 #endif  // OS_ANDROID
 }
 
-VariationsSeedStore::~VariationsSeedStore() {
-}
+VariationsSeedStore::~VariationsSeedStore() = default;
 
 bool VariationsSeedStore::LoadSeed(VariationsSeed* seed,
                                    std::string* seed_data,
@@ -149,7 +150,6 @@ bool VariationsSeedStore::StoreSeedData(
     const base::Time& date_fetched,
     bool is_delta_compressed,
     bool is_gzip_compressed,
-    bool fetched_insecurely,
     VariationsSeed* parsed_seed) {
   UMA_HISTOGRAM_COUNTS_1000("Variations.StoreSeed.DataSize",
                             data.length() / 1024);
@@ -181,8 +181,7 @@ bool VariationsSeedStore::StoreSeedData(
 
   if (!is_delta_compressed) {
     return StoreSeedDataNoDelta(ungzipped_data, base64_seed_signature,
-                                country_code, date_fetched, fetched_insecurely,
-                                parsed_seed);
+                                country_code, date_fetched, parsed_seed);
   }
 
   // If the data is delta compressed, first decode it.
@@ -200,9 +199,9 @@ bool VariationsSeedStore::StoreSeedData(
     return false;
   }
 
-  const bool result = StoreSeedDataNoDelta(
-      updated_seed_data, base64_seed_signature, country_code, date_fetched,
-      fetched_insecurely, parsed_seed);
+  const bool result =
+      StoreSeedDataNoDelta(updated_seed_data, base64_seed_signature,
+                           country_code, date_fetched, parsed_seed);
   if (!result)
     RecordStoreSeedResult(StoreSeedResult::FAILED_DELTA_STORE);
   return result;
@@ -237,9 +236,9 @@ bool VariationsSeedStore::StoreSafeSeed(
     const ClientFilterableState& client_state,
     base::Time seed_fetch_time) {
   std::string base64_seed_data;
-  StoreSeedResult result = VerifyAndCompressSeedData(
-      seed_data, base64_seed_signature, false /* fetched_insecurely */,
-      SeedType::SAFE, &base64_seed_data, nullptr);
+  StoreSeedResult result =
+      VerifyAndCompressSeedData(seed_data, base64_seed_signature,
+                                SeedType::SAFE, &base64_seed_data, nullptr);
   UMA_HISTOGRAM_ENUMERATION("Variations.SafeMode.StoreSafeSeed.Result", result,
                             StoreSeedResult::ENUM_SIZE);
   if (result != StoreSeedResult::SUCCESS)
@@ -378,17 +377,6 @@ void VariationsSeedStore::RegisterPrefs(PrefRegistrySimple* registry) {
                                std::string());
 }
 
-bool VariationsSeedStore::SignatureVerificationEnabled() {
-#if defined(OS_IOS) || defined(OS_ANDROID)
-  // Signature verification is disabled on mobile platforms for now, since it
-  // adds about ~15ms to the startup time on mobile (vs. a couple ms on
-  // desktop).
-  return false;
-#else
-  return true;
-#endif
-}
-
 void VariationsSeedStore::ClearPrefs(SeedType seed_type) {
   if (seed_type == SeedType::LATEST) {
     local_state_->ClearPref(prefs::kVariationsCompressedSeed);
@@ -428,7 +416,7 @@ void VariationsSeedStore::ImportInitialSeed(
 
   if (!StoreSeedData(initial_seed->data, initial_seed->signature,
                      initial_seed->country, date, false,
-                     initial_seed->is_gzip_compressed, false, nullptr)) {
+                     initial_seed->is_gzip_compressed, nullptr)) {
     RecordFirstRunSeedImportResult(FirstRunSeedImportResult::FAIL_STORE_FAILED);
     LOG(WARNING) << "First run variations seed is invalid.";
     return;
@@ -449,7 +437,7 @@ LoadSeedResult VariationsSeedStore::LoadSeedImpl(
   *base64_seed_signature = local_state_->GetString(
       seed_type == SeedType::LATEST ? prefs::kVariationsSeedSignature
                                     : prefs::kVariationsSafeSeedSignature);
-  if (SignatureVerificationEnabled()) {
+  if (signature_verification_enabled_) {
     const VerifySignatureResult result =
         VerifySeedSignature(*seed_data, *base64_seed_signature);
     if (seed_type == SeedType::LATEST) {
@@ -509,13 +497,12 @@ bool VariationsSeedStore::StoreSeedDataNoDelta(
     const std::string& base64_seed_signature,
     const std::string& country_code,
     const base::Time& date_fetched,
-    bool fetched_insecurely,
     VariationsSeed* parsed_seed) {
   std::string base64_seed_data;
   VariationsSeed seed;
-  StoreSeedResult result = VerifyAndCompressSeedData(
-      seed_data, base64_seed_signature, fetched_insecurely, SeedType::LATEST,
-      &base64_seed_data, &seed);
+  StoreSeedResult result =
+      VerifyAndCompressSeedData(seed_data, base64_seed_signature,
+                                SeedType::LATEST, &base64_seed_data, &seed);
   if (result != StoreSeedResult::SUCCESS) {
     RecordStoreSeedResult(result);
     return false;
@@ -556,7 +543,6 @@ bool VariationsSeedStore::StoreSeedDataNoDelta(
 StoreSeedResult VariationsSeedStore::VerifyAndCompressSeedData(
     const std::string& seed_data,
     const std::string& base64_seed_signature,
-    bool fetched_insecurely,
     SeedType seed_type,
     std::string* base64_seed_data,
     VariationsSeed* parsed_seed) {
@@ -568,7 +554,7 @@ StoreSeedResult VariationsSeedStore::VerifyAndCompressSeedData(
   if (!seed.ParseFromString(seed_data))
     return StoreSeedResult::FAILED_PARSE;
 
-  if (SignatureVerificationEnabled() || fetched_insecurely) {
+  if (signature_verification_enabled_) {
     const VerifySignatureResult result =
         VerifySeedSignature(seed_data, base64_seed_signature);
     switch (seed_type) {

components/variations/variations_seed_store.h

@@ -29,14 +29,18 @@ class VariationsSeed;
 // seed from Local State.
 class VariationsSeedStore {
  public:
+  // Standard constructor. Enables signature verification.
   explicit VariationsSeedStore(PrefService* local_state);
   // |initial_seed| may be null. If not null, then it will be stored in this
   // seed store. This is used by Android Chrome to supply the first run seed,
   // and by Android WebView to supply the seed on every run.
   // |on_initial_seed_stored| will be called the first time a seed is stored.
+  // |signature_verification_enabled| can be used in unit tests to disable
+  // signature checks on the seed.
   VariationsSeedStore(PrefService* local_state,
                       std::unique_ptr<SeedResponse> initial_seed,
-                      base::OnceCallback<void()> on_initial_seed_stored);
+                      base::OnceCallback<void()> on_initial_seed_stored,
+                      bool signature_verification_enabled);
   virtual ~VariationsSeedStore();
 
   // Loads the variations seed data from local state into |seed|, as well as the
@@ -65,7 +69,6 @@ class VariationsSeedStore {
                      const base::Time& date_fetched,
                      bool is_delta_compressed,
                      bool is_gzip_compressed,
-                     bool fetched_insecurely,
                      VariationsSeed* parsed_seed) WARN_UNUSED_RESULT;
 
   // Loads the safe variations seed data from local state into |seed| and
@@ -120,10 +123,6 @@ class VariationsSeedStore {
   PrefService* local_state() { return local_state_; }
   const PrefService* local_state() const { return local_state_; }
 
- protected:
-  // Whether signature verification is enabled. Overridable for tests.
-  virtual bool SignatureVerificationEnabled();
-
  private:
   FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, VerifySeedSignature);
   FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, ApplyDeltaPatch);
@@ -179,21 +178,17 @@ class VariationsSeedStore {
                             const std::string& base64_seed_signature,
                             const std::string& country_code,
                             const base::Time& date_fetched,
-                            bool fetched_insecurely,
                             VariationsSeed* parsed_seed) WARN_UNUSED_RESULT;
 
   // Validates the |seed_data|, comparing it (if enabled) against the provided
   // cryptographic signature. Returns the result of the operation. On success,
   // fills |base64_seed_data| with the compressed and base64-encoded seed data;
   // and if |parsed_seed| is non-null, fills it with the parsed seed data.
-  // |fetched_insecurely| indicates whether |seed_data| was just fetched from
-  // the server over an insecure channel (i.e. over HTTP rathern than HTTPS).
   // |seed_type| specifies whether |seed_data| is for the safe seed (vs. the
   // regular/normal seed).
   StoreSeedResult VerifyAndCompressSeedData(
       const std::string& seed_data,
       const std::string& base64_seed_signature,
-      bool fetched_insecurely,
       SeedType seed_type,
       std::string* base64_seed_data,
       VariationsSeed* parsed_seed) WARN_UNUSED_RESULT;
@@ -212,6 +207,9 @@ class VariationsSeedStore {
 
   base::OnceCallback<void()> on_initial_seed_stored_;
 
+  // Whether to validate signatures on the seed. Always on except in tests.
+  bool signature_verification_enabled_ = true;
+
   DISALLOW_COPY_AND_ASSIGN(VariationsSeedStore);
 };
 

components/variations/variations_seed_store_unittest.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/base64.h"
+#include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/time/time.h"
@@ -51,16 +52,17 @@ constexpr char kIdenticalToSafeSeedSentinel[] = "safe_seed_content";
 class TestVariationsSeedStore : public VariationsSeedStore {
  public:
   explicit TestVariationsSeedStore(PrefService* local_state)
-      : VariationsSeedStore(local_state) {}
-  ~TestVariationsSeedStore() override {}
+      : VariationsSeedStore(local_state,
+                            nullptr,
+                            base::DoNothing(),
+                            /*signature_verification_enabled=*/false) {}
+  ~TestVariationsSeedStore() override = default;
 
   bool StoreSeedForTesting(const std::string& seed_data) {
     return StoreSeedData(seed_data, std::string(), std::string(),
-                         base::Time::Now(), false, false, false, nullptr);
+                         base::Time::Now(), false, false, nullptr);
   }
 
-  bool SignatureVerificationEnabled() override { return false; }
-
  private:
   DISALLOW_COPY_AND_ASSIGN(TestVariationsSeedStore);
 };
@@ -72,9 +74,7 @@ class SignatureVerifyingVariationsSeedStore : public VariationsSeedStore {
  public:
   explicit SignatureVerifyingVariationsSeedStore(PrefService* local_state)
       : VariationsSeedStore(local_state) {}
-  ~SignatureVerifyingVariationsSeedStore() override {}
-
-  bool SignatureVerificationEnabled() override { return true; }
+  ~SignatureVerifyingVariationsSeedStore() override = default;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SignatureVerifyingVariationsSeedStore);
@@ -348,7 +348,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_ParsedSeed) {
   VariationsSeed parsed_seed;
   EXPECT_TRUE(seed_store.StoreSeedData(serialized_seed, std::string(),
                                        std::string(), base::Time::Now(), false,
-                                       false, false, &parsed_seed));
+                                       false, &parsed_seed));
   EXPECT_EQ(serialized_seed, SerializeSeed(parsed_seed));
 }
 
@@ -360,13 +360,13 @@ TEST(VariationsSeedStoreTest, StoreSeedData_CountryCode) {
   // Test with a valid header value.
   std::string seed = SerializeSeed(CreateTestSeed());
   EXPECT_TRUE(seed_store.StoreSeedData(seed, std::string(), "test_country",
-                                       base::Time::Now(), false, false, false,
+                                       base::Time::Now(), false, false,
                                        nullptr));
   EXPECT_EQ("test_country", prefs.GetString(prefs::kVariationsCountry));
 
   // Test with no country code specified - which should preserve the old value.
   EXPECT_TRUE(seed_store.StoreSeedData(seed, std::string(), std::string(),
-                                       base::Time::Now(), false, false, false,
+                                       base::Time::Now(), false, false,
                                        nullptr));
   EXPECT_EQ("test_country", prefs.GetString(prefs::kVariationsCountry));
 }
@@ -384,7 +384,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_GzippedSeed) {
   VariationsSeed parsed_seed;
   EXPECT_TRUE(seed_store.StoreSeedData(compressed_seed, std::string(),
                                        std::string(), base::Time::Now(), false,
-                                       true, false, &parsed_seed));
+                                       true, &parsed_seed));
   EXPECT_EQ(serialized_seed, SerializeSeed(parsed_seed));
 }
 
@@ -920,7 +920,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_GzippedEmptySeed) {
   VariationsSeed parsed_seed;
   EXPECT_FALSE(seed_store.StoreSeedData(compressed_seed, std::string(),
                                         std::string(), base::Time::Now(), false,
-                                        true, false, &parsed_seed));
+                                        true, &parsed_seed));
 }
 
 TEST(VariationsSeedStoreTest, VerifySeedSignature) {