From 75d339759c73444cd7ff04c0e31af01aeca5f500 Mon Sep 17 00:00:00 2001
From: Dibyajyoti Pal <[email protected]>
Date: Wed, 20 May 2026 18:21:12 -0700
Subject: [PATCH] [PWA] Enforce custom icon hash validations during admin-installs if provided

This CL enforces custom icon hash validations during admin-installs if provided as part of the WebAppInstallForceList enterprise policy.
This makes the custom icon being provided more secure.

Problems:
1. The custom icon hash specified in the policy entry dictionary was never extracted or validated, resulting in an integrity bypass.
2. During a placeholder installation, FetchCustomIcon performed a top-  level navigation of the shared WebContents to the custom icon URL. A compromised icon origin could respond with HTML/JS, executing arbitrary scripts inside the background renderer process associated with the WebAppCommandManager.

Fixes:
1. Modify WebAppPolicyManager to extract and store the 'hash' key from custom icon policy entries in ExternalInstallOptions.
2. Implement a secure icon-downloading helper 'FetchAndVerifyCustomIcon' inside web_app_install_utils.cc. This downloads raw icon bytes directly within the browser process via network::SimpleURLLoader (thus executing no script), computes the SHA256 hash of the raw bytes, verifies it against the expected hash (if provided in the policy), and then decodes it safely out-of-process using ImageDecoder.
3. Update InstallPlaceholderJob and ExternalAppResolutionCommand to fetch custom icons securely using FetchAndVerifyCustomIcon instead of performing a top-level navigation of WebContents.
4. Do not override manifest icons in WebAppPolicyManager. Instead, securely fetch the custom icon and apply it directly to the installation metadata before finalizing the install.

Unit tests are updated to mock URLLoader responses with valid mock PNG
payloads matching their respective expected SHA256 hashes.

Bug: 513484193
Include-Ci-Only-Tests: chromium.mac:mac15-x64-rel-tests|browser_tests
Change-Id: I4673dad972f54aa9536f63277794f4e2d8f3d7f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7856177
Reviewed-by: Nate Chapin <[email protected]>
Reviewed-by: Marijn Kruisselbrink <[email protected]>
Commit-Queue: Dibyajyoti Pal <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1633966}
---

diff --git a/chrome/browser/web_applications/BUILD.gn b/chrome/browser/web_applications/BUILD.gn
index 821efed8..283cb673 100644
--- a/chrome/browser/web_applications/BUILD.gn
+++ b/chrome/browser/web_applications/BUILD.gn
@@ -92,6 +92,8 @@
     "commands/web_app_uninstall_command.h",
     "commands/web_install_from_url_command.cc",
     "commands/web_install_from_url_command.h",
+    "custom_icon_fetcher.cc",
+    "custom_icon_fetcher.h",
     "daily_metrics_helper.cc",
     "daily_metrics_helper.h",
     "extension_status_utils.h",
@@ -581,6 +583,7 @@
     "//chrome/browser/browsing_data:constants",
     "//chrome/browser/content_settings:content_settings_factory",
     "//chrome/browser/favicon",
+    "//chrome/browser/image_decoder",
     "//chrome/browser/metrics",
     "//chrome/browser/profiles:profile",
     "//chrome/browser/profiles:profile_manager",
diff --git a/chrome/browser/web_applications/commands/external_app_resolution_command.cc b/chrome/browser/web_applications/commands/external_app_resolution_command.cc
index 09edfce9..a9ba7642 100644
--- a/chrome/browser/web_applications/commands/external_app_resolution_command.cc
+++ b/chrome/browser/web_applications/commands/external_app_resolution_command.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/commands/web_app_command.h"
 #include "chrome/browser/web_applications/commands/web_app_uninstall_command.h"
+#include "chrome/browser/web_applications/custom_icon_fetcher.h"
 #include "chrome/browser/web_applications/external_install_options.h"
 #include "chrome/browser/web_applications/externally_managed_app_manager.h"
 #include "chrome/browser/web_applications/jobs/finalize_install_job.h"
@@ -436,6 +437,52 @@
 
 void ExternalAppResolutionCommand::UpdateInfoWithParamsAndUpgradeLock(
     bool icon_download_failed) {
+  if (install_options_.override_icon_url) {
+    custom_icon_fetcher_ = std::make_unique<CustomIconFetcher>(
+        &profile_.get(), install_options_.override_icon_url.value(),
+        install_options_.override_icon_hash);
+    custom_icon_fetcher_->StartRequest(base::BindOnce(
+        &ExternalAppResolutionCommand::OnCustomIconDecodedPopulateBitmaps,
+        weak_ptr_factory_.GetWeakPtr()));
+    return;
+  }
+
+  ContinueUpdateInfoWithParamsAndUpgradeLock(icon_download_failed);
+}
+
+void ExternalAppResolutionCommand::OnCustomIconDecodedPopulateBitmaps(
+    std::optional<SkBitmap> bitmap) {
+  custom_icon_fetcher_.reset();
+  bool icon_download_failed = true;
+
+  // Populate the manifest and trusted icons from the downloaded custom icon
+  // urls.
+  if (bitmap) {
+    CHECK(!bitmap->drawsNothing());
+    web_app_info_->icon_bitmaps.any.clear();
+    web_app_info_->icon_bitmaps.maskable.clear();
+    web_app_info_->icon_bitmaps.monochrome.clear();
+    web_app_info_->trusted_icon_bitmaps.any.clear();
+    web_app_info_->trusted_icon_bitmaps.maskable.clear();
+    web_app_info_->trusted_icon_bitmaps.monochrome.clear();
+
+    IconsMap icons_map;
+    icons_map.emplace(install_options_.override_icon_url.value(),
+                      std::vector<SkBitmap>{bitmap.value()});
+    PopulateProductIcons(web_app_info_.get(), &icons_map);
+
+    apps::IconInfo trusted_bitmap;
+    trusted_bitmap.url = install_options_.override_icon_url.value();
+    web_app_info_->trusted_icons = {trusted_bitmap};
+    PopulateTrustedIconBitmaps(*web_app_info_.get(), icons_map);
+    icon_download_failed = false;
+  }
+
+  ContinueUpdateInfoWithParamsAndUpgradeLock(icon_download_failed);
+}
+
+void ExternalAppResolutionCommand::ContinueUpdateInfoWithParamsAndUpgradeLock(
+    bool icon_download_failed) {
   // TODO(b/300878868): Reject installation if the manifest id provided in the
   // WebAppInstallForceList does not match the final manifest id.
   app_id_ = GenerateAppIdFromManifestId(web_app_info_->manifest_id());
@@ -476,8 +523,8 @@
   finalize_options.add_to_desktop = install_params_->add_to_desktop;
   finalize_options.add_to_quick_launch_bar =
       install_params_->add_to_quick_launch_bar;
-  // TODO(crbug.com/379136842): This is likely too 'permissive' of a check, and
-  // different more restrictive filter should likely be used instead.
+  // TODO(crbug.com/379136842): This is likely too 'permissive' of a check,
+  // and different more restrictive filter should likely be used instead.
   if (apps_lock_->registrar().AppMatches(
           app_id_, WebAppFilter::IsAppSurfaceableToUser())) {
     // If an installation is triggered for the same app but with a
@@ -650,17 +697,17 @@
 
 void ExternalAppResolutionCommand::OnPlaceHolderAppLockAcquired() {
   CHECK(apps_lock_);
-  // This is the entry point for the placeholder installation path. It is called
-  // after the initial URL load has failed and we have acquired a lock for the
-  // placeholder app ID.
+  // This is the entry point for the placeholder installation path. It is
+  // called after the initial URL load has failed and we have acquired a lock
+  // for the placeholder app ID.
   CHECK(apps_lock_->IsGranted());
   if (on_lock_upgraded_callback_for_testing_) {
     std::move(on_lock_upgraded_callback_for_testing_).Run();
   }
 
   // TODO(b/300878868): Use the manifest id specified in the
-  // `WebAppInstallForceList` to generate the placeholder app id. This is needed
-  // to make sure an in-place installation can be done.
+  // `WebAppInstallForceList` to generate the placeholder app id. This is
+  // needed to make sure an in-place installation can be done.
   install_placeholder_job_.emplace(
       &profile_.get(),
       *GetMutableDebugValue().EnsureDict("install_placeholder_job"),
@@ -693,8 +740,8 @@
 
 void ExternalAppResolutionCommand::InstallFromInfo() {
   // This is the entry point for the offline installation path. It is called
-  // when the initial URL load fails and we are not installing a placeholder, or
-  // when `only_use_app_info_factory` is true.
+  // when the initial URL load fails and we are not installing a placeholder,
+  // or when `only_use_app_info_factory` is true.
   install_params_ = ConvertExternalInstallOptionsToParams(install_options_);
   CHECK(install_params_.has_value());
 
@@ -719,8 +766,8 @@
                std::move(install_params_->additional_search_terms));
   web_app_info_->install_url = install_params_->install_url;
 
-  // External installs are considered trusted, all manifest icons can be used as
-  // trusted ones.
+  // External installs are considered trusted, all manifest icons can be used
+  // as trusted ones.
   web_app_info_->trusted_icons = web_app_info_->manifest_icons;
   web_app_info_->trusted_icon_bitmaps = web_app_info_->icon_bitmaps;
 
diff --git a/chrome/browser/web_applications/commands/external_app_resolution_command.h b/chrome/browser/web_applications/commands/external_app_resolution_command.h
index 106fd4e..535c67a5 100644
--- a/chrome/browser/web_applications/commands/external_app_resolution_command.h
+++ b/chrome/browser/web_applications/commands/external_app_resolution_command.h
@@ -45,6 +45,7 @@
 
 namespace web_app {
 
+class CustomIconFetcher;
 class FinalizeInstallJob;
 class InstallPlaceholderJob;
 class WebAppDataRetriever;
@@ -122,6 +123,8 @@
       DownloadedIconsHttpResults icons_http_results);
 
   void UpdateInfoWithParamsAndUpgradeLock(bool icon_download_failed);
+  void OnCustomIconDecodedPopulateBitmaps(std::optional<SkBitmap> bitmap);
+  void ContinueUpdateInfoWithParamsAndUpgradeLock(bool icon_download_failed);
 
   void OnLockUpgradedFinalizeInstall(bool icon_download_failed);
   void OnInstallFinalized(const webapps::AppId& app_id,
@@ -196,6 +199,7 @@
   std::optional<InstallFromInfoJob> install_from_info_job_;
   std::optional<RemoveInstallSourceJob> remove_placeholder_job_;
   std::optional<FinalizeInstallJob> install_job_;
+  std::unique_ptr<CustomIconFetcher> custom_icon_fetcher_;
 

diff --git a/chrome/browser/web_applications/jobs/install_placeholder_job_unittest.cc b/chrome/browser/web_applications/jobs/install_placeholder_job_unittest.cc
index d1cc126..ad02cb9e 100644
--- a/chrome/browser/web_applications/jobs/install_placeholder_job_unittest.cc
+++ b/chrome/browser/web_applications/jobs/install_placeholder_job_unittest.cc
@@ -35,6 +35,7 @@
 #include "components/webapps/browser/web_contents/web_app_url_loader.h"
 #include "components/webapps/common/web_app_id.h"
 #include "net/http/http_status_code.h"
+#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -114,7 +115,8 @@
 
 class InstallPlaceholderJobTest : public WebAppTest {
  public:
-  static constexpr int kIconSize = 96;
+  InstallPlaceholderJobTest()
+      : WebAppTest(WebAppTest::WithTestUrlLoaderFactory()) {}
   const GURL kInstallUrl = GURL("https://example.com");
 
   void SetUp() override {
@@ -156,39 +158,26 @@
 }
 
 TEST_F(InstallPlaceholderJobTest, InstallPlaceholderWithOverrideIconUrl) {
+  data_decoder::test::InProcessDataDecoder data_decoder;
   ExternalInstallOptions options(kInstallUrl, mojom::UserDisplayMode::kBrowser,
                                  ExternalInstallSource::kExternalPolicy);
   const GURL icon_url("https://somedifferentoriginexample.com/test.png");
   options.override_icon_url = icon_url;
   base::test::TestFuture<webapps::InstallResultCode, webapps::AppId> future;
 
-  auto data_retriever =
-      std::make_unique<testing::StrictMock<MockDataRetriever>>();
   auto url_loader = std::make_unique<web_app::TestWebAppUrlLoader>();
-
-  SkBitmap bitmap;
-  std::vector<gfx::Size> icon_sizes(1, gfx::Size(kIconSize, kIconSize));
-  bitmap.allocN32Pixels(kIconSize, kIconSize);
-  bitmap.eraseColor(SK_ColorRED);
-  IconsMap icons = {{icon_url, {bitmap}}};
-  const IconUrlWithSize icon_metadata =
-      IconUrlWithSize::CreateForUnspecifiedSize(icon_url);
-  DownloadedIconsHttpResults http_result = {
-      {icon_metadata, net::HttpStatusCode::HTTP_OK}};
-  EXPECT_CALL(
-      *data_retriever,
-      GetIcons(testing::_, testing::ElementsAre(icon_metadata),
-               /*download_page_favicons=*/false, /*fail_all_if_any_fail=*/false,
-               base::test::IsNotNullCallback()))
-      .WillOnce(base::test::RunOnceCallback<4>(
-          IconsDownloadedResult::kCompleted, std::move(icons), http_result));
   url_loader->SetNextLoadUrlResult(kInstallUrl,
                                    webapps::WebAppUrlLoaderResult::kUrlLoaded);
-  url_loader->SetNextLoadUrlResult(icon_url,
-                                   webapps::WebAppUrlLoaderResult::kUrlLoaded);
+
+  std::string png_bytes =
+      "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A\x00\x00\x00\x0D\x49\x48\x44\x52\x00\x00"
+      "\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90\x77\x53\xDE\x00\x00\x00"
+      "\x0C\x49\x44\x41\x54\x78\x9C\x63\xF8\xCF\xC0\x00\x00\x03\x01\x01\x00\x18"
+      "\xDD\x8D\xB0\x00\x00\x00\x00\x49\x45\x4E\x44\xAE\x42\x60\x82";
+  profile_url_loader_factory().AddResponse(icon_url.spec(), png_bytes);
 
   auto command = std::make_unique<InstallPlaceholderJobWrapperCommand>(
-      profile(), options, future.GetCallback(), std::move(data_retriever),
+      profile(), options, future.GetCallback(), /*data_retriever=*/nullptr,
       std::move(url_loader));
   provider()->command_manager().ScheduleCommand(std::move(command));
diff --git a/chrome/browser/web_applications/policy/web_app_policy_manager_browsertest.cc b/chrome/browser/web_applications/policy/web_app_policy_manager_browsertest.cc
index 62ce1176..cf2ce9f 100644
--- a/chrome/browser/web_applications/policy/web_app_policy_manager_browsertest.cc
+++ b/chrome/browser/web_applications/policy/web_app_policy_manager_browsertest.cc
@@ -72,7 +72,8 @@
  public:
   static constexpr char kDefaultAppName[] = "Simple web app";
   static constexpr char kDefaultCustomName[] = "Custom name";
-  static constexpr char kDefaultCustomIconHash[] = "abcdef";
+  static constexpr char kDefaultCustomIconHash[] =
+      "7b1d9c8e582971ec50be86f32c7753cb6532a066bd1f9ab222c30a0a3fee429f";
 
   static constexpr char kInstallUrlSuffix[] =
       "/web_apps/install_url/install_url.html";
@@ -482,8 +483,19 @@
 
   EXPECT_EQ(kDefaultCustomName,
             base::UTF16ToASCII(manifest->name.value_or(std::u16string())));
-  ASSERT_EQ(1u, manifest->icons.size());
-  EXPECT_TRUE(manifest->icons[0].src.spec().ends_with(kCustomIconUrlSuffix));
+  ASSERT_EQ(2u, manifest->icons.size());
+  EXPECT_TRUE(manifest->icons[0].src.spec().ends_with("basic-48.png"));
+  EXPECT_TRUE(manifest->icons[1].src.spec().ends_with("basic-192.png"));
+
+  base::test::TestFuture<WebAppIconManager::WebAppBitmaps> disk_bitmaps;
+  provider().icon_manager().ReadAllIcons(GetAppId(),
+                                         disk_bitmaps.GetCallback());
+  ASSERT_TRUE(disk_bitmaps.Wait());
+  const OrderedSizeToBitmap& any_icons = disk_bitmaps.Get().trusted_icons.any;
+  ASSERT_THAT(any_icons, testing::Contains(testing::Pair(192, testing::_)));
+  EXPECT_THAT(
+      any_icons.at(192),
+      gfx::test::EqualsBitmap(gfx::test::CreateBitmap(192, SK_ColorBLUE)));
 }
 
 // This test suite verifies the WebAppInstallByUserEnabled policy behavior when
diff --git a/chrome/browser/web_applications/policy/web_app_policy_manager_unittest.cc b/chrome/browser/web_applications/policy/web_app_policy_manager_unittest.cc
index 896c93b..a9296a9 100644
--- a/chrome/browser/web_applications/policy/web_app_policy_manager_unittest.cc
+++ b/chrome/browser/web_applications/policy/web_app_policy_manager_unittest.cc
@@ -121,7 +121,8 @@
 const char kDefaultCustomAppName[] = "custom app name";
 constexpr char kDefaultCustomIconUrl[] = "https://windowed.example/icon.png";
 constexpr char kUnsecureIconUrl[] = "http://windowed.example/icon.png";
-constexpr char kDefaultCustomIconHash[] = "abcdef";
+constexpr char kDefaultCustomIconHash[] =
+    "567e3611094b537115a14c3a314014e8dfcb2b6e993ede8b2da85be93f200849";
 
 base::DictValue GetWindowedItem() {
   return base::DictValue()
@@ -182,15 +183,18 @@
       .Set(kCustomNameKey, std::move(name));
 }
 
-base::DictValue GetCustomAppIconItem(bool secure = true) {
+base::DictValue GetCustomAppIconItem(bool secure = true,
+                                     bool include_hash = true) {
+  base::DictValue custom_icon;
+  custom_icon.Set(kCustomIconURLKey,
+                  secure ? kDefaultCustomIconUrl : kUnsecureIconUrl);
+  if (include_hash) {
+    custom_icon.Set(kCustomIconHashKey, kDefaultCustomIconHash);
+  }
   return base::DictValue()
       .Set(kUrlKey, kWindowedUrl)
       .Set(kDefaultLaunchContainerKey, kDefaultLaunchContainerWindowValue)
-      .Set(kCustomIconKey,
-           base::DictValue()
-               .Set(kCustomIconURLKey,
-                    secure ? kDefaultCustomIconUrl : kUnsecureIconUrl)
-               .Set(kCustomIconHashKey, kDefaultCustomIconHash));
+      .Set(kCustomIconKey, std::move(custom_icon));
 }
 
 void SetWebAppSettingsListPref(Profile* profile, std::string_view pref) {
@@ -215,7 +219,8 @@
 
 class WebAppPolicyManagerTestBase : public WebAppTest {
  public:
-  WebAppPolicyManagerTestBase() = default;
+  WebAppPolicyManagerTestBase()
+      : WebAppTest(WebAppTest::WithTestUrlLoaderFactory()) {}
   WebAppPolicyManagerTestBase(const WebAppPolicyManagerTestBase&) = delete;
   WebAppPolicyManagerTestBase& operator=(const WebAppPolicyManagerTestBase&) =
       delete;
@@ -607,6 +612,13 @@
 }
 
 TEST_F(WebAppPolicyManagerTest, ForceInstallAppWithCustomAppIcon) {
+  std::string png_bytes =
+      "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A\x00\x00\x00\x0D\x49\x48\x44\x52\x00\x00"
+      "\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90\x77\x53\xDE\x00\x00\x00"
+      "\x0C\x49\x44\x41\x54\x78\x9C\x63\xF8\xCF\xC0\x00\x00\x03\x01\x01\x00\x18"
+      "\xDD\x8D\xB0\x00\x00\x00\x00\x49\x45\x4E\x44\xAE\x42\x60\x82";
+  profile_url_loader_factory().AddResponse(kDefaultCustomIconUrl, png_bytes);
+
   base::ListValue list;
   list.Append(GetCustomAppIconItem());
   profile()->GetPrefs()->SetList(prefs::kWebAppInstallForceList,
@@ -616,6 +628,23 @@
   EXPECT_NE(GetPolicyInstalledWindowedApp(), nullptr);
 }
 
+TEST_F(WebAppPolicyManagerTest, ForceInstallAppWithCustomAppIconNoHash) {
+  std::string png_bytes =
+      "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A\x00\x00\x00\x0D\x49\x48\x44\x52\x00\x00"
+      "\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90\x77\x53\xDE\x00\x00\x00"
+      "\x0C\x49\x44\x41\x54\x78\x9C\x63\xF8\xCF\xC0\x00\x00\x03\x01\x01\x00\x18"
+      "\xDD\x8D\xB0\x00\x00\x00\x00\x49\x45\x4E\x44\xAE\x42\x60\x82";
+  profile_url_loader_factory().AddResponse(kDefaultCustomIconUrl, png_bytes);
+
+  base::ListValue list;
+  list.Append(GetCustomAppIconItem(/*secure=*/true, /*include_hash=*/false));
+  profile()->GetPrefs()->SetList(prefs::kWebAppInstallForceList,
+                                 std::move(list));
+
+  WaitForAppsToSynchronize();
+  EXPECT_NE(GetPolicyInstalledWindowedApp(), nullptr);
+}
+
 // If the custom icon URL is not https, the icon should be ignored.
 TEST_F(WebAppPolicyManagerTest, ForceInstallAppWithUnsecureCustomAppIcon) {
   base::ListValue list;