Expand URL whitelisting for anonymizer

This adds whitelisting to URL redaction for URLs of the format
chrome://*/crisper.js and also chrome-extension://<first-party-id>/*.js
as long as there are also no query parameters appended to those.

There is also a constant added for the list of 1st party extension IDs.
Extra plumbing of those values were required in order to keep with
allowed dependencies.

Bug: 979779
Test: component_unittests pass
Change-Id: I151ddb3d3791a57b6f394867bf244e945febc321
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699699
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: J Kardatzke <jkardatzke@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678328}

chrome/browser/chromeos/policy/system_log_uploader.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "components/feedback/anonymizer_tool.h"
 #include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/user_manager/user_manager.h"
@@ -130,7 +131,8 @@ std::string ReadAndAnonymizeLogFile(feedback::AnonymizerTool* anonymizer,
 // as pairs (file name, data) and returns. Called on blocking thread.
 std::unique_ptr<SystemLogUploader::SystemLogs> ReadFiles() {
   auto system_logs = std::make_unique<SystemLogUploader::SystemLogs>();
-  feedback::AnonymizerTool anonymizer;
+  feedback::AnonymizerTool anonymizer(
+      extension_misc::kBuiltInFirstPartyExtensionIds);
   for (const char* file_path : kSystemLogFileNames) {
     if (!base::PathExists(base::FilePath(file_path)))
       continue;

chrome/browser/extensions/api/feedback_private/chrome_feedback_private_delegate.cc

@@ -200,8 +200,12 @@ void ChromeFeedbackPrivateDelegate::FetchExtraLogs(
   constexpr bool scrub = true;
 
   if (system_logs::ContainsIwlwifiLogs(feedback_data->sys_info())) {
+    // TODO (jkardatzke): Modify this so that we are using the same instance of
+    // the anonymizer for the rest of the logs.
+    // We can pass null for the 1st party IDs since we are just anonymizing
+    // wifi data here.
     system_logs::SystemLogsFetcher* fetcher =
-        new system_logs::SystemLogsFetcher(scrub);
+        new system_logs::SystemLogsFetcher(scrub, nullptr);
     fetcher->AddSource(std::make_unique<system_logs::IwlwifiDumpLogSource>());
     fetcher->Fetch(base::BindOnce(&OnFetchedExtraLogs, feedback_data,
                                   std::move(callback)));

chrome/browser/feedback/system_logs/about_system_logs_fetcher.cc

@@ -21,7 +21,8 @@ namespace system_logs {
 
 SystemLogsFetcher* BuildAboutSystemLogsFetcher() {
   const bool scrub_data = false;
-  SystemLogsFetcher* fetcher = new SystemLogsFetcher(scrub_data);
+  // We aren't anonymizing, so we can pass null for the 1st party IDs.
+  SystemLogsFetcher* fetcher = new SystemLogsFetcher(scrub_data, nullptr);
 
   fetcher->AddSource(std::make_unique<ChromeInternalLogSource>());
   fetcher->AddSource(std::make_unique<MemoryDetailsLogSource>());

chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h"
 #include "chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h"
 #include "chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "components/feedback/system_logs/system_logs_fetcher.h"
 
 #if defined(OS_CHROMEOS)
@@ -23,7 +24,8 @@ namespace system_logs {
 
 SystemLogsFetcher* BuildChromeSystemLogsFetcher() {
   const bool scrub_data = true;
-  SystemLogsFetcher* fetcher = new SystemLogsFetcher(scrub_data);
+  SystemLogsFetcher* fetcher = new SystemLogsFetcher(
+      scrub_data, extension_misc::kBuiltInFirstPartyExtensionIds);
 
   fetcher->AddSource(std::make_unique<ChromeInternalLogSource>());
   fetcher->AddSource(std::make_unique<CrashIdsSource>());

chrome/common/extensions/extension_constants.cc

@@ -45,6 +45,50 @@ const char kInAppPaymentsSupportAppId[] = "nmmhkkegccagdldgiimedpiccmgmieda";
 const char kMediaRouterStableExtensionId[] = "pkedcjkdefgpdelpbcmbmeomcjbeemfm";
 const char kCloudReportingExtensionId[] = "oempjldejiginopiohodkdoklcjklbaa";
 
+const char* const kBuiltInFirstPartyExtensionIds[] = {
+    kCalculatorAppId,
+    kCalendarAppId,
+    kChromeRemoteDesktopAppId,
+    kCloudPrintAppId,
+    kDataSaverExtensionId,
+    kDocsOfflineExtensionId,
+    kDriveHostedAppId,
+    kEnterpriseWebStoreAppId,
+    kGmailAppId,
+    kGoogleDocAppId,
+    kGoogleMapsAppId,
+    kGooglePhotosAppId,
+    kGooglePlayBooksAppId,
+    kGooglePlayMoviesAppId,
+    kGooglePlayMusicAppId,
+    kGooglePlusAppId,
+    kGoogleSheetsAppId,
+    kGoogleSlidesAppId,
+    kHTermAppId,
+    kHTermDevAppId,
+    kIdentityApiUiAppId,
+    kCroshBuiltinAppId,
+    kTextEditorAppId,
+    kInAppPaymentsSupportAppId,
+    kMediaRouterStableExtensionId,
+    kCloudReportingExtensionId,
+#if defined(OS_CHROMEOS)
+    kAssessmentAssistantExtensionId,
+    kAutoclickExtensionId,
+    kSelectToSpeakExtensionId,
+    kSwitchAccessExtensionId,
+    kFirstRunDialogId,
+    kEspeakSpeechSynthesisExtensionId,
+    kGoogleSpeechSynthesisExtensionId,
+    kWallpaperManagerId,
+    kZipArchiverExtensionId,
+    kChromeCameraAppId,
+    kChromeCameraAppDevId,
+    kKioskNextHomeAppId,
+#endif        // defined(OS_CHROMEOS)
+    nullptr,  // Null-terminated array.
+};
+
 #if defined(OS_CHROMEOS)
 const char kAssessmentAssistantExtensionId[] =
     "gndmhdcefbhlchkhipcnnbkcmicncehk";
@@ -76,7 +120,8 @@ const char kChromeCameraAppId[] = "hfhhnacclhffhdffklopdkcgdhifgngh";
 const char kChromeCameraAppDevId[] = "flgnmkgjffmkephdokeeliiopbjaafpm";
 const char kChromeCameraAppPath[] = "chromeos/camera";
 const char kKioskNextHomeAppId[] = "nbaolgedfgoedkjbfmpediclncanmpbc";
-#endif
+
+#endif  // defined(CHROME_OS)
 
 const char kAppStateNotInstalled[] = "not_installed";
 const char kAppStateInstalled[] = "installed";

chrome/common/extensions/extension_constants.h

@@ -105,6 +105,9 @@ extern const char kMediaRouterStableExtensionId[];
 // The extension id of the Chrome Reporting extension.
 extern const char kCloudReportingExtensionId[];
 
+// A list of all the first party extension IDs, last entry is null.
+extern const char* const kBuiltInFirstPartyExtensionIds[];
+
 // The buckets used for app launches.
 enum AppLaunchBucket {
   // Launch from NTP apps section while maximized.

components/feedback/anonymizer_tool.cc

@@ -353,8 +353,9 @@ bool FindAndConsumeAndGetSkipped(re2::StringPiece* input,
 
 }  // namespace
 
-AnonymizerTool::AnonymizerTool()
-    : custom_patterns_with_context_(base::size(kCustomPatternsWithContext)),
+AnonymizerTool::AnonymizerTool(const char* const* first_party_extension_ids)
+    : first_party_extension_ids_(first_party_extension_ids),
+      custom_patterns_with_context_(base::size(kCustomPatternsWithContext)),
       custom_patterns_without_context_(
           base::size(kCustomPatternsWithoutContext)) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
@@ -477,11 +478,49 @@ std::string AnonymizerTool::AnonymizeCustomPatternWithContext(
   return result;
 }
 
-bool WhitelistMatchedId(re2::StringPiece matched_id) {
-  bool is_safe_chrome_resource =
-      matched_id.starts_with("chrome://resources/") &&
-      !matched_id.contains("?");
-  return is_safe_chrome_resource;
+// This takes a |url| argument and returns true if the URL is whitelisted and
+// does NOT need to be redacted, returns false otherwise.
+bool IsUrlWhitelisted(re2::StringPiece url,
+                      const char* const* first_party_extension_ids) {
+  // We do not whitelist anything with a query parameter.
+  if (url.contains("?"))
+    return false;
+
+  // Check for whitelisting of chrome:// URLs.
+  if (url.starts_with("chrome://")) {
+    // We allow everything in chrome://resources/.
+    if (url.starts_with("chrome://resources/"))
+      return true;
+
+    // We allow chrome://*/crisper.js.
+    if (url.ends_with("/crisper.js"))
+      return true;
+
+    return false;
+  }
+
+  // If the whitelist is null, then don't check it.
+  if (!first_party_extension_ids)
+    return false;
+
+  // Whitelist URLs of the format chrome-extension://<first-party-id>/*.js
+  if (!url.starts_with("chrome-extension://"))
+    return false;
+
+  // These must end with a .js extension.
+  if (!url.ends_with(".js"))
+    return false;
+
+  int i = 0;
+  const char* test_id = first_party_extension_ids[i];
+  const re2::StringPiece url_sub =
+      url.substr(sizeof("chrome-extension://") - 1);
+  while (test_id) {
+    if (url_sub.starts_with(test_id))
+      return true;
+    test_id = first_party_extension_ids[++i];
+  }
+  return false;
 }
 
 std::string AnonymizerTool::AnonymizeCustomPatternWithoutContext(
@@ -499,7 +538,7 @@ std::string AnonymizerTool::AnonymizeCustomPatternWithoutContext(
   re2::StringPiece skipped;
   re2::StringPiece matched_id;
   while (FindAndConsumeAndGetSkipped(&text, *re, &skipped, &matched_id)) {
-    if (WhitelistMatchedId(matched_id)) {
+    if (IsUrlWhitelisted(matched_id, first_party_extension_ids_)) {
       skipped.AppendToString(&result);
       matched_id.AppendToString(&result);
       continue;
@@ -527,8 +566,10 @@ std::string AnonymizerTool::AnonymizeCustomPatternWithoutContext(
 }
 
 AnonymizerToolContainer::AnonymizerToolContainer(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : anonymizer_(new AnonymizerTool), task_runner_(task_runner) {}
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    const char* const* first_party_extension_ids)
+    : anonymizer_(new AnonymizerTool(first_party_extension_ids)),
+      task_runner_(task_runner) {}
 
 AnonymizerToolContainer::~AnonymizerToolContainer() {
   task_runner_->DeleteSoon(FROM_HERE, std::move(anonymizer_));

components/feedback/anonymizer_tool.h

@@ -32,7 +32,10 @@ struct CustomPatternWithoutContext {
 
 class AnonymizerTool {
  public:
-  AnonymizerTool();
+  // |first_party_extension_ids| is a null terminated array of all the 1st
+  // party extension IDs whose URLs won't be redacted. It is OK to pass null for
+  // that value if it's OK to redact those URLs or they won't be present.
+  AnonymizerTool(const char* const* first_party_extension_ids);
   ~AnonymizerTool();
 
   // Returns an anonymized version of |input|. PII-sensitive data (such as MAC
@@ -57,6 +60,10 @@ class AnonymizerTool {
       const CustomPatternWithoutContext& pattern,
       std::map<std::string, std::string>* identifier_space);
 
+  // Null-terminated list of first party extension IDs. We need to have this
+  // passed into us because we can't refer to the code where these are defined.
+  const char* const* first_party_extension_ids_;  // Not owned.
+
   // Map of MAC addresses discovered in anonymized strings to anonymized
   // representations. 11:22:33:44:55:66 gets anonymized to 11:22:33:00:00:01,
   // where the first three bytes represent the manufacturer. The last three
@@ -88,7 +95,8 @@ class AnonymizerToolContainer
     : public base::RefCountedThreadSafe<AnonymizerToolContainer> {
  public:
   explicit AnonymizerToolContainer(
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      const char* const* first_party_extension_ids);
 
   // Returns a pointer to the instance of this anonymier. May only be called
   // on |task_runner_|.

components/feedback/anonymizer_tool_unittest.cc

@@ -10,6 +10,9 @@
 
 namespace feedback {
 
+const char kFakeFirstPartyID[] = "nkoccljplnhpfnfiajclkommnmllphnl";
+const char* const kFakeFirstPartyExtensionIDs[] = {kFakeFirstPartyID, nullptr};
+
 class AnonymizerToolTest : public testing::Test {
  protected:
   std::string AnonymizeMACAddresses(const std::string& input) {
@@ -35,7 +38,7 @@ class AnonymizerToolTest : public testing::Test {
                                                             space);
   }
 
-  AnonymizerTool anonymizer_;
+  AnonymizerTool anonymizer_{kFakeFirstPartyExtensionIDs};
 };
 
 TEST_F(AnonymizerToolTest, Anonymize) {
@@ -347,8 +350,16 @@ TEST_F(AnonymizerToolTest, AnonymizeChunk) {
        "aa:aa:aa:00:00:01"},
       {"chrome://resources/foo",  // Secure chrome resource, whitelisted.
        "chrome://resources/foo"},
+      {"chrome://settings/crisper.js",  // Whitelisted settings URLs.
+       "chrome://settings/crisper.js"},
+      // Whitelisted first party extension.
+      {"chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/foobar.js",
+       "chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/foobar.js"},
       {"chrome://resources/f?user=bar",  // Potentially PII in parameter.
-       "<URL: 2>"}};
+       "<URL: 2>"},
+      {"chrome-extension://nkoccljplnhpfnfiajclkommnmllphnl/foobar.js?bar=x",
+       "<URL: 3>"},  // Potentially PII in parameter.
+  };
   std::string anon_input;
   std::string anon_output;
   for (const auto& s : data) {

components/feedback/system_logs/system_logs_fetcher.cc

@@ -46,7 +46,9 @@ void Anonymize(feedback::AnonymizerTool* anonymizer,
 
 }  // namespace
 
-SystemLogsFetcher::SystemLogsFetcher(bool scrub_data)
+SystemLogsFetcher::SystemLogsFetcher(
+    bool scrub_data,
+    const char* const first_party_extension_ids[])
     : response_(std::make_unique<SystemLogsResponse>()),
       num_pending_requests_(0),
       task_runner_for_anonymizer_(base::CreateSequencedTaskRunnerWithTraits(
@@ -55,7 +57,8 @@ SystemLogsFetcher::SystemLogsFetcher(bool scrub_data)
            base::TaskPriority::USER_VISIBLE,
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})) {
   if (scrub_data)
-    anonymizer_ = std::make_unique<feedback::AnonymizerTool>();
+    anonymizer_ =
+        std::make_unique<feedback::AnonymizerTool>(first_party_extension_ids);
 }
 
 SystemLogsFetcher::~SystemLogsFetcher() {

components/feedback/system_logs/system_logs_fetcher.h

@@ -43,8 +43,12 @@ using SysLogsFetcherCallback =
 // };
 class SystemLogsFetcher {
  public:
-  // If scrub_data is true, logs will be anonymized.
-  explicit SystemLogsFetcher(bool scrub_data);
+  // If |scrub_data| is true, logs will be anonymized.
+  // |first_party_extension_ids| is a null terminated array of all the 1st
+  // party extension IDs whose URLs won't be redacted. It is OK to pass null for
+  // that value if it's OK to redact those URLs or they won't be present.
+  explicit SystemLogsFetcher(bool scrub_data,
+                             const char* const first_party_extension_ids[]);
   ~SystemLogsFetcher();
 
   // Adds a source to use when fetching.

extensions/browser/api/feedback_private/log_source_access_manager.cc

@@ -81,7 +81,8 @@ LogSourceAccessManager::LogSourceAccessManager(content::BrowserContext* context)
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
       anonymizer_container_(
           base::MakeRefCounted<feedback::AnonymizerToolContainer>(
-              task_runner_for_anonymizer_)),
+              task_runner_for_anonymizer_,
+              /* first_party_extension_ids= */ nullptr)),
       weak_factory_(this) {}
 
 LogSourceAccessManager::~LogSourceAccessManager() {}

extensions/shell/browser/system_logs/shell_system_logs_fetcher.cc

@@ -12,7 +12,9 @@ namespace system_logs {
 SystemLogsFetcher* BuildShellSystemLogsFetcher(
     content::BrowserContext* browser_context) {
   // Deletes itself after Fetch() is completes.
-  SystemLogsFetcher* fetcher = new SystemLogsFetcher(true /* scrub_data */);
+  SystemLogsFetcher* fetcher =
+      new SystemLogsFetcher(/* scrub_data= */ true,
+                            /* first_party_extension_ids= */ nullptr);
   fetcher->AddSource(std::make_unique<BasicLogSource>(browser_context));
   return fetcher;
 }