[omnibox] Fix UAF bug in TemplateURLParser

This CL takes a snapshot of SearchTermsData (just like
HistoryURLProvider does) within TemplateURLParser, so we eliminate a
source of UAF bugs, where the TemplateURL parsing outlives the original
SearchTermsData.

This bug happens during Chrome shutdown.

Bug: 1278322
Change-Id: I439d9d3193bcaa7ef57ec0a046d057c32ef6fb76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3403242
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#961536}

components/omnibox/browser/history_url_provider.cc

@@ -275,86 +275,6 @@ GURL ConvertToHostOnly(const history::HistoryMatch& match,
 
 }  // namespace
 
-// -----------------------------------------------------------------
-// SearchTermsDataSnapshot
-
-// Implementation of SearchTermsData that takes a snapshot of another
-// SearchTermsData by copying all the responses to the different getters into
-// member strings, then returning those strings when its own getters are called.
-// This will typically be constructed on the UI thread from
-// UIThreadSearchTermsData but is subsequently safe to use on any thread.
-class SearchTermsDataSnapshot : public SearchTermsData {
- public:
-  explicit SearchTermsDataSnapshot(const SearchTermsData* search_terms_data);
-  ~SearchTermsDataSnapshot() override;
-  SearchTermsDataSnapshot(const SearchTermsDataSnapshot&) = delete;
-  SearchTermsDataSnapshot& operator=(const SearchTermsDataSnapshot&) = delete;
-
-  std::string GoogleBaseURLValue() const override;
-  std::string GetApplicationLocale() const override;
-  std::u16string GetRlzParameterValue(bool from_app_list) const override;
-  std::string GetSearchClient() const override;
-  std::string GoogleImageSearchSource() const override;
-
-  // Estimates dynamic memory usage.
-  // See base/trace_event/memory_usage_estimator.h for more info.
-  size_t EstimateMemoryUsage() const override;
-
- private:
-  std::string google_base_url_value_;
-  std::string application_locale_;
-  std::u16string rlz_parameter_value_;
-  std::string search_client_;
-  std::string google_image_search_source_;
-};
-
-SearchTermsDataSnapshot::SearchTermsDataSnapshot(
-    const SearchTermsData* search_terms_data) {
-  if (search_terms_data) {
-    google_base_url_value_ = search_terms_data->GoogleBaseURLValue();
-    application_locale_ = search_terms_data->GetApplicationLocale();
-    rlz_parameter_value_ = search_terms_data->GetRlzParameterValue(false);
-    search_client_ = search_terms_data->GetSearchClient();
-    google_image_search_source_ = search_terms_data->GoogleImageSearchSource();
-  }
-}
-
-SearchTermsDataSnapshot::~SearchTermsDataSnapshot() {
-}
-
-std::string SearchTermsDataSnapshot::GoogleBaseURLValue() const {
-  return google_base_url_value_;
-}
-
-std::string SearchTermsDataSnapshot::GetApplicationLocale() const {
-  return application_locale_;
-}
-
-std::u16string SearchTermsDataSnapshot::GetRlzParameterValue(
-    bool from_app_list) const {
-  return rlz_parameter_value_;
-}
-
-std::string SearchTermsDataSnapshot::GetSearchClient() const {
-  return search_client_;
-}
-
-std::string SearchTermsDataSnapshot::GoogleImageSearchSource() const {
-  return google_image_search_source_;
-}
-
-size_t SearchTermsDataSnapshot::EstimateMemoryUsage() const {
-  size_t res = 0;
-
-  res += base::trace_event::EstimateMemoryUsage(google_base_url_value_);
-  res += base::trace_event::EstimateMemoryUsage(application_locale_);
-  res += base::trace_event::EstimateMemoryUsage(rlz_parameter_value_);
-  res += base::trace_event::EstimateMemoryUsage(search_client_);
-  res += base::trace_event::EstimateMemoryUsage(google_image_search_source_);
-
-  return res;
-}
-
 // -----------------------------------------------------------------
 // HistoryURLProvider
 
@@ -463,7 +383,7 @@ HistoryURLProviderParams::HistoryURLProviderParams(
           default_search_provider
               ? new TemplateURL(default_search_provider->data())
               : nullptr),
-      search_terms_data(new SearchTermsDataSnapshot(search_terms_data)),
+      search_terms_data(SearchTermsData::MakeSnapshot(search_terms_data)),
       allow_deleting_browser_history(allow_deleting_browser_history) {}
 
 HistoryURLProviderParams::~HistoryURLProviderParams() {

components/search_engines/search_terms_data.cc

@@ -3,13 +3,105 @@
 // found in the LICENSE file.
 
 #include "components/search_engines/search_terms_data.h"
+#include <memory>
 
 #include "base/check.h"
+#include "base/trace_event/memory_usage_estimator.h"
+#include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "components/google/core/common/google_util.h"
 #include "components/lens/lens_features.h"
 #include "url/gurl.h"
 
+namespace {
+
+// -----------------------------------------------------------------
+// SearchTermsDataSnapshot
+
+// Implementation of SearchTermsData that takes a snapshot of another
+// SearchTermsData by copying all the responses to the different getters into
+// member strings, then returning those strings when its own getters are called.
+// This will typically be constructed on the UI thread from
+// UIThreadSearchTermsData but is subsequently safe to use on any thread.
+class SearchTermsDataSnapshot : public SearchTermsData {
+ public:
+  explicit SearchTermsDataSnapshot(const SearchTermsData* search_terms_data);
+  ~SearchTermsDataSnapshot() override;
+  SearchTermsDataSnapshot(const SearchTermsDataSnapshot&) = delete;
+  SearchTermsDataSnapshot& operator=(const SearchTermsDataSnapshot&) = delete;
+
+  std::string GoogleBaseURLValue() const override;
+  std::string GetApplicationLocale() const override;
+  std::u16string GetRlzParameterValue(bool from_app_list) const override;
+  std::string GetSearchClient() const override;
+  std::string GoogleImageSearchSource() const override;
+
+  // Estimates dynamic memory usage.
+  // See base/trace_event/memory_usage_estimator.h for more info.
+  size_t EstimateMemoryUsage() const override;
+
+ private:
+  std::string google_base_url_value_;
+  std::string application_locale_;
+  std::u16string rlz_parameter_value_;
+  std::string search_client_;
+  std::string google_image_search_source_;
+};
+
+SearchTermsDataSnapshot::SearchTermsDataSnapshot(
+    const SearchTermsData* search_terms_data) {
+  if (search_terms_data) {
+    google_base_url_value_ = search_terms_data->GoogleBaseURLValue();
+    application_locale_ = search_terms_data->GetApplicationLocale();
+    rlz_parameter_value_ = search_terms_data->GetRlzParameterValue(false);
+    search_client_ = search_terms_data->GetSearchClient();
+    google_image_search_source_ = search_terms_data->GoogleImageSearchSource();
+  }
+}
+
+SearchTermsDataSnapshot::~SearchTermsDataSnapshot() = default;
+
+std::string SearchTermsDataSnapshot::GoogleBaseURLValue() const {
+  return google_base_url_value_;
+}
+
+std::string SearchTermsDataSnapshot::GetApplicationLocale() const {
+  return application_locale_;
+}
+
+std::u16string SearchTermsDataSnapshot::GetRlzParameterValue(
+    bool from_app_list) const {
+  return rlz_parameter_value_;
+}
+
+std::string SearchTermsDataSnapshot::GetSearchClient() const {
+  return search_client_;
+}
+
+std::string SearchTermsDataSnapshot::GoogleImageSearchSource() const {
+  return google_image_search_source_;
+}
+
+size_t SearchTermsDataSnapshot::EstimateMemoryUsage() const {
+  size_t res = 0;
+
+  res += base::trace_event::EstimateMemoryUsage(google_base_url_value_);
+  res += base::trace_event::EstimateMemoryUsage(application_locale_);
+  res += base::trace_event::EstimateMemoryUsage(rlz_parameter_value_);
+  res += base::trace_event::EstimateMemoryUsage(search_client_);
+  res += base::trace_event::EstimateMemoryUsage(google_image_search_source_);
+
+  return res;
+}
+
+}  // namespace
+
+// static
+std::unique_ptr<SearchTermsData> SearchTermsData::MakeSnapshot(
+    const SearchTermsData* original_data) {
+  return std::make_unique<SearchTermsDataSnapshot>(original_data);
+}
+
 SearchTermsData::SearchTermsData() = default;
 
 SearchTermsData::~SearchTermsData() = default;

components/search_engines/search_terms_data.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SEARCH_ENGINES_SEARCH_TERMS_DATA_H_
 #define COMPONENTS_SEARCH_ENGINES_SEARCH_TERMS_DATA_H_
 
+#include <memory>
 #include <string>
 
 #include "base/compiler_specific.h"
@@ -13,6 +14,12 @@
 // only be accessed on the UI thread.
 class SearchTermsData {
  public:
+  // Utility function that takes a snapshot of a different SearchTermsData
+  // instance. This is used to access SearchTermsData off the UI thread, or to
+  // copy the SearchTermsData for lifetime reasons.
+  static std::unique_ptr<SearchTermsData> MakeSnapshot(
+      const SearchTermsData* original_data);
+
   SearchTermsData();
 
   SearchTermsData(const SearchTermsData&) = delete;

components/search_engines/template_url_parser.cc

@@ -11,7 +11,6 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
-#include "base/memory/raw_ptr.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -111,7 +110,7 @@ class SafeTemplateURLParser {
       const SearchTermsData* search_terms_data,
       const TemplateURLParser::ParameterFilter& parameter_filter,
       TemplateURLParser::ParseCallback callback)
-      : search_terms_data_(search_terms_data),
+      : search_terms_data_(SearchTermsData::MakeSnapshot(search_terms_data)),
         parameter_filter_(parameter_filter),
         callback_(std::move(callback)) {}
 
@@ -158,7 +157,10 @@ class SafeTemplateURLParser {
   // at least one element, if only the empty string.
   std::vector<std::string> namespaces_;
 
-  raw_ptr<const SearchTermsData> search_terms_data_;
+  // We have to own our own snapshot, because the parse request may outlive the
+  // originally provided SearchTermsData lifetime.
+  std::unique_ptr<SearchTermsData> search_terms_data_;
+
   TemplateURLParser::ParameterFilter parameter_filter_;
   TemplateURLParser::ParseCallback callback_;
 };