From f89efb408bbd1329e7f06bbac2e78293896b4604 Mon Sep 17 00:00:00 2001
From: Vincent Scheib <scheib@chromium.org>
Date: Wed, 01 Jul 2026 16:52:15 -0700
Subject: [PATCH] Sanitize bidi and control characters in PWA file extensions

PWA file extension strings in the manifest can contain unsanitized
bidirectional control characters or other control characters.
This CL implements a three-tier hybrid enforcement strategy to
prevent UI spoofing:

1. Parse-time DevTools Gate: Rejects manifest file handler
   extensions containing U_FORMAT_CHAR or U_CONTROL_CHAR during
   Blink manifest parsing, logging a DevTools console error.
2. Browser-side Zero-Trust IPC Validation: Hardens the trusted
   browser process to reject manifests containing these characters in
   file handler extensions, preventing exploitation even by
   compromised renderers.
3. Display-time Hardening: Sanitizes extensions before UI rendering
   in web_app_utils.cc by actively filtering out U_FORMAT_CHAR
   and U_CONTROL_CHAR.

Tests:
- WebAppUtilsTest.
  TransformFileExtensionsForDisplay_StripsBidiControls
  (covers LRM, RLO, SOH, DEL, APC, and empty string)
- ManifestParserTest.FileHandlerParseRules
  (covers format and control characters)
- ManifestBrowserTest.
  BadMessage_FileHandlersAcceptExtensionInvalidFormatCharacter
- ManifestBrowserTest.
  BadMessage_FileHandlersAcceptExtensionInvalidControlCharacter

BUG=520490618
BUG=513996595
TAG=agy
CONV=d87700cf-7987-4cf2-bb6b-56ac7798e179

Change-Id: I77f874f6dbac094c522eed822486978cdff90a0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8012267
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Luke Deng <lukedeng@google.com>
Commit-Queue: Vincent Scheib <scheib@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1655824}
---
diff --git a/chrome/browser/web_applications/web_app_utils.cc b/chrome/browser/web_applications/web_app_utils.cc
index f2ccb0ce..5a8bf2c8 100644
--- a/chrome/browser/web_applications/web_app_utils.cc
+++ b/chrome/browser/web_applications/web_app_utils.cc
@@ -68,6 +68,8 @@
 #include "third_party/blink/public/common/safe_url_pattern.h"
 #include "third_party/blink/public/mojom/manifest/display_mode.mojom-shared.h"
 #include "third_party/blink/public/mojom/manifest/manifest.mojom-shared.h"
+#include "third_party/icu/source/common/unicode/uchar.h"
+#include "third_party/icu/source/common/unicode/utf16.h"
 #include "third_party/liburlpattern/options.h"
 #include "third_party/liburlpattern/part.h"
 #include "third_party/liburlpattern/pattern.h"
@@ -426,7 +428,33 @@
   std::ranges::transform(
       extensions, std::back_inserter(extensions_for_display),
       [](const std::string& extension) {
-        return base::UTF8ToUTF16(base::ToUpperASCII(extension.substr(1)));
+        if (extension.empty()) {
+          return std::u16string();
+        }
+        std::u16string ext_u16 =
+            base::UTF8ToUTF16(base::ToUpperASCII(extension.substr(1)));
+        std::u16string sanitized;
+        sanitized.reserve(ext_u16.size());
+        for (size_t i = 0; i < ext_u16.length();) {
+          UChar32 c;
+          U16_NEXT(ext_u16, i, ext_u16.length(), c);
+          // TODO(crbug.com/530303003): This check for control and format
+          // characters is duplicated across manifest parsing, IPC validation,
+          // and PWA display. Consider consolidating it into a shared helper in
+          // //base/strings/string_util.h.
+          if (!base::IsUnicodeControl(c) && u_charType(c) != U_FORMAT_CHAR) {
+            if (c <= 0xFFFF) {
+              // Safe to cast UChar32 to char16_t here because the guard ensures
+              // c is within the BMP (<= 0xFFFF), and Unicode code points are
+              // non-negative.
+              sanitized.push_back(static_cast<char16_t>(c));
+            } else {
+              sanitized.push_back(U16_LEAD(c));
+              sanitized.push_back(U16_TRAIL(c));
+            }
+          }
+        }
+        return sanitized;
       });
   return extensions_for_display;
 }
diff --git a/content/browser/manifest/manifest_manager_host.cc b/content/browser/manifest/manifest_manager_host.cc
index ad89aab3..0bf0cd58 100644
--- a/content/browser/manifest/manifest_manager_host.cc
+++ b/content/browser/manifest/manifest_manager_host.cc
@@ -26,6 +26,8 @@
 #include "third_party/blink/public/common/security/protocol_handler_security_level.h"
 #include "third_party/blink/public/mojom/manifest/manifest.mojom.h"
 #include "third_party/blink/public/mojom/manifest/manifest_manager.mojom.h"
+#include "third_party/icu/source/common/unicode/uchar.h"
+#include "third_party/icu/source/common/unicode/utf16.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -93,6 +95,22 @@
       if (!document_origin.IsSameOriginWith(file_handler->action)) {
         return "Manifest file_handlers must be same-origin with the document.";
       }
+      for (const auto& [mime_type, extensions] : file_handler->accept) {
+        for (const auto& extension : extensions) {
+          for (size_t i = 0; i < extension.length();) {
+            UChar32 c;
+            U16_NEXT(extension, i, extension.length(), c);
+            // TODO(crbug.com/530303003): This check for control and format
+            // characters is duplicated across manifest parsing, IPC validation,
+            // and PWA display. Consider consolidating it into a shared helper
+            // in //base/strings/string_util.h.
+            if (base::IsUnicodeControl(c) || u_charType(c) == U_FORMAT_CHAR) {
+              return "Manifest file_handlers accept extension contains invalid "
+                     "control or format characters.";
+            }
+          }
+        }
+      }
     }
 
     for (const auto& protocol_handler : manifest.protocol_handlers) {
diff --git a/third_party/blink/renderer/modules/manifest/manifest_parser.cc b/third_party/blink/renderer/modules/manifest/manifest_parser.cc
index 870efde..893a0b2 100644
--- a/third_party/blink/renderer/modules/manifest/manifest_parser.cc
+++ b/third_party/blink/renderer/modules/manifest/manifest_parser.cc
@@ -4,6 +4,8 @@
 
 #include "third_party/blink/renderer/modules/manifest/manifest_parser.h"
 
+#include <unicode/uchar.h>
+
 #include <cstddef>
 #include <memory>
 #include <optional>
@@ -17,6 +19,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
+#include "base/strings/string_util.h"
 #include "net/base/mime_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "services/device/public/mojom/screen_orientation_lock_types.mojom-blink.h"
@@ -48,6 +51,7 @@
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/weborigin/security_origin.h"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
+#include "third_party/blink/renderer/platform/wtf/text/code_point_iterator.h"
 #include "third_party/blink/renderer/platform/wtf/text/strcat.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_impl.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_utf8_adaptor.h"
@@ -1821,6 +1825,19 @@
     return false;
   }
 
+  // TODO(crbug.com/530303003): This check for control and format characters is
+  // duplicated across manifest parsing, IPC validation, and PWA display.
+  // Consider consolidating it into a shared helper in
+  // //base/strings/string_util.h.
+  for (UChar32 c : *output) {
+    if (base::IsUnicodeControl(c) || u_charType(c) == U_FORMAT_CHAR) {
+      AddErrorInfo(
+          "property 'accept' file extension ignored, contains invalid "
+          "control or format characters.");
+      return false;
+    }
+  }
+
   return true;
 }
 
diff --git a/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py b/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
index 20a6cb7..1a279700 100755
--- a/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
+++ b/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
@@ -278,6 +278,7 @@
             'base::ClampFloor',
             'base::ClampRound',
             'base::IsTypeInRangeForNumericType',
+            'base::IsUnicodeControl',
             'base::IsValueInRangeForNumericType',
             'base::IsValueNegative',
             'base::MakeStrictNum',
diff --git a/chrome/browser/web_applications/web_app_utils_unittest.cc b/chrome/browser/web_applications/web_app_utils_unittest.cc
index 7cae1bd..50659495 100644
--- a/chrome/browser/web_applications/web_app_utils_unittest.cc
+++ b/chrome/browser/web_applications/web_app_utils_unittest.cc
@@ -115,6 +115,23 @@
 #endif
 }
 
+TEST_F(WebAppUtilsTest, TransformFileExtensionsForDisplay_StripsBidiControls) {
+  std::set<std::string> extensions = {
+      ".aa\xE2\x80\x8E",  // LRM (Format)
+      ".bb\xE2\x80\xAE",  // RLO (Format)
+      ".cc\x01",          // SOH (Control)
+      ".dd\x7F",          // DEL (Control)
+      ".ee\xC2\x9F",      // APC (Control)
+      ".bat",             // Safe
+      ""                  // Empty
+  };
+  std::vector<std::u16string> transformed =
+      TransformFileExtensionsForDisplay(extensions);
+
+  EXPECT_THAT(transformed, ::testing::UnorderedElementsAre(
+                               u"", u"AA", u"BB", u"CC", u"DD", u"EE", u"BAT"));
+}
+
 TEST_F(WebAppUtilsTest, AreWebAppsUserInstallable) {
   Profile* regular_profile = profile();
 
diff --git a/content/browser/manifest/manifest_browsertest.cc b/content/browser/manifest/manifest_browsertest.cc
index 2022229..a4485c1 100644
--- a/content/browser/manifest/manifest_browsertest.cc
+++ b/content/browser/manifest/manifest_browsertest.cc
@@ -1245,6 +1245,76 @@
           "Manifest file_handlers must be same-origin with the document."));
 }
 
+IN_PROC_BROWSER_TEST_F(
+    ManifestBrowserTest,
+    BadMessage_FileHandlersAcceptExtensionInvalidFormatCharacter) {
+  const GURL test_url =
+      embedded_test_server()->GetURL("/manifest/empty-manifest.html");
+  ASSERT_TRUE(NavigateToURL(shell(), test_url));
+
+  ManifestManagerHost* host = ManifestManagerHost::GetOrCreateForPage(
+      shell()->web_contents()->GetPrimaryPage());
+
+  mojo::FakeMessageDispatchContext fake_dispatch_context;
+  auto bad_manifest = blink::mojom::Manifest::New();
+  bad_manifest->start_url = test_url;
+  bad_manifest->id = test_url;
+  bad_manifest->scope = embedded_test_server()->GetURL("/manifest/");
+
+  blink::mojom::ManifestFileHandlerPtr file_handler =
+      blink::mojom::ManifestFileHandler::New();
+  file_handler->action = test_url;
+
+  std::vector<std::u16string> extensions;
+  extensions.push_back(u".png\u202E");
+  file_handler->accept[u"image/png"] = std::move(extensions);
+
+  bad_manifest->file_handlers.push_back(std::move(file_handler));
+
+  mojo::test::BadMessageObserver bad_message_observer;
+  host->ValidateAndMaybeOverrideManifestForTesting(
+      blink::mojom::ManifestRequestResult::kSuccess, std::move(bad_manifest));
+  EXPECT_THAT(bad_message_observer.WaitForBadMessage(),
+              ::testing::StartsWith(
+                  "Manifest file_handlers accept extension contains invalid "
+                  "control or format characters."));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    ManifestBrowserTest,
+    BadMessage_FileHandlersAcceptExtensionInvalidControlCharacter) {
+  const GURL test_url =
+      embedded_test_server()->GetURL("/manifest/empty-manifest.html");
+  ASSERT_TRUE(NavigateToURL(shell(), test_url));
+
+  ManifestManagerHost* host = ManifestManagerHost::GetOrCreateForPage(
+      shell()->web_contents()->GetPrimaryPage());
+
+  mojo::FakeMessageDispatchContext fake_dispatch_context;
+  auto bad_manifest = blink::mojom::Manifest::New();
+  bad_manifest->start_url = test_url;
+  bad_manifest->id = test_url;
+  bad_manifest->scope = embedded_test_server()->GetURL("/manifest/");
+
+  blink::mojom::ManifestFileHandlerPtr file_handler =
+      blink::mojom::ManifestFileHandler::New();
+  file_handler->action = test_url;
+
+  std::vector<std::u16string> extensions;
+  extensions.push_back(u".png\u0001");
+  file_handler->accept[u"image/png"] = std::move(extensions);
+
+  bad_manifest->file_handlers.push_back(std::move(file_handler));
+
+  mojo::test::BadMessageObserver bad_message_observer;
+  host->ValidateAndMaybeOverrideManifestForTesting(
+      blink::mojom::ManifestRequestResult::kSuccess, std::move(bad_manifest));
+  EXPECT_THAT(bad_message_observer.WaitForBadMessage(),
+              ::testing::StartsWith(
+                  "Manifest file_handlers accept extension contains invalid "
+                  "control or format characters."));
+}
+
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest,
                        BadMessage_ProtocolHandlersActionCrossOrigin) {
   const GURL test_url =
diff --git a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
index 488fbdcf..3e0076d 100644
--- a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
+++ b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
@@ -3040,6 +3040,62 @@
     ASSERT_EQ(0u, file_handlers.size());
   }
 
+  // Extensions that contain format characters are invalid.
+  {
+    auto& manifest = ParseManifest(
+        R"({
+          "file_handlers": [
+            {
+              "name": "name",
+              "action": "/files",
+              "accept": {
+                "image/png": [
+                  ".png\u202E"
+                ]
+              }
+            }
+          ]
+        })");
+    auto& file_handlers = manifest->file_handlers;
+
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'accept' file extension ignored, contains invalid "
+        "control or format characters.",
+        errors()[0]);
+    EXPECT_EQ("FileHandler ignored. Property 'accept' is invalid.",
+              errors()[1]);
+    ASSERT_EQ(0u, file_handlers.size());
+  }
+
+  // Extensions that contain control characters are invalid.
+  {
+    auto& manifest = ParseManifest(
+        R"({
+          "file_handlers": [
+            {
+              "name": "name",
+              "action": "/files",
+              "accept": {
+                "image/png": [
+                  ".png\u0001"
+                ]
+              }
+            }
+          ]
+        })");
+    auto& file_handlers = manifest->file_handlers;
+
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'accept' file extension ignored, contains invalid "
+        "control or format characters.",
+        errors()[0]);
+    EXPECT_EQ("FileHandler ignored. Property 'accept' is invalid.",
+              errors()[1]);
+    ASSERT_EQ(0u, file_handlers.size());
+  }
+
   // Invalid MIME types and those with parameters are stripped.
   {
     auto& manifest = ParseManifest(

diff --git a/chrome/browser/web_applications/web_app_utils_unittest.cc b/chrome/browser/web_applications/web_app_utils_unittest.cc
index 7cae1bd..50659495 100644
--- a/chrome/browser/web_applications/web_app_utils_unittest.cc
+++ b/chrome/browser/web_applications/web_app_utils_unittest.cc
@@ -115,6 +115,23 @@
 #endif
 }
 
+TEST_F(WebAppUtilsTest, TransformFileExtensionsForDisplay_StripsBidiControls) {
+  std::set<std::string> extensions = {
+      ".aa\xE2\x80\x8E",  // LRM (Format)
+      ".bb\xE2\x80\xAE",  // RLO (Format)
+      ".cc\x01",          // SOH (Control)
+      ".dd\x7F",          // DEL (Control)
+      ".ee\xC2\x9F",      // APC (Control)
+      ".bat",             // Safe
+      ""                  // Empty
+  };
+  std::vector<std::u16string> transformed =
+      TransformFileExtensionsForDisplay(extensions);
+
+  EXPECT_THAT(transformed, ::testing::UnorderedElementsAre(
+                               u"", u"AA", u"BB", u"CC", u"DD", u"EE", u"BAT"));
+}
+
 TEST_F(WebAppUtilsTest, AreWebAppsUserInstallable) {
   Profile* regular_profile = profile();
diff --git a/content/browser/manifest/manifest_browsertest.cc b/content/browser/manifest/manifest_browsertest.cc
index 2022229..a4485c1 100644
--- a/content/browser/manifest/manifest_browsertest.cc
+++ b/content/browser/manifest/manifest_browsertest.cc
@@ -1245,6 +1245,76 @@
           "Manifest file_handlers must be same-origin with the document."));
 }
 
+IN_PROC_BROWSER_TEST_F(
+    ManifestBrowserTest,
+    BadMessage_FileHandlersAcceptExtensionInvalidFormatCharacter) {
+  const GURL test_url =
+      embedded_test_server()->GetURL("/manifest/empty-manifest.html");
+  ASSERT_TRUE(NavigateToURL(shell(), test_url));
+
+  ManifestManagerHost* host = ManifestManagerHost::GetOrCreateForPage(
+      shell()->web_contents()->GetPrimaryPage());
+
+  mojo::FakeMessageDispatchContext fake_dispatch_context;
+  auto bad_manifest = blink::mojom::Manifest::New();
+  bad_manifest->start_url = test_url;
+  bad_manifest->id = test_url;
+  bad_manifest->scope = embedded_test_server()->GetURL("/manifest/");
+
+  blink::mojom::ManifestFileHandlerPtr file_handler =
+      blink::mojom::ManifestFileHandler::New();
+  file_handler->action = test_url;
+
+  std::vector<std::u16string> extensions;
+  extensions.push_back(u".png\u202E");
+  file_handler->accept[u"image/png"] = std::move(extensions);
+
+  bad_manifest->file_handlers.push_back(std::move(file_handler));
+
+  mojo::test::BadMessageObserver bad_message_observer;
+  host->ValidateAndMaybeOverrideManifestForTesting(
+      blink::mojom::ManifestRequestResult::kSuccess, std::move(bad_manifest));
+  EXPECT_THAT(bad_message_observer.WaitForBadMessage(),
+              ::testing::StartsWith(
+                  "Manifest file_handlers accept extension contains invalid "
+                  "control or format characters."));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    ManifestBrowserTest,
+    BadMessage_FileHandlersAcceptExtensionInvalidControlCharacter) {
+  const GURL test_url =
+      embedded_test_server()->GetURL("/manifest/empty-manifest.html");
+  ASSERT_TRUE(NavigateToURL(shell(), test_url));
+
+  ManifestManagerHost* host = ManifestManagerHost::GetOrCreateForPage(
+      shell()->web_contents()->GetPrimaryPage());
+
+  mojo::FakeMessageDispatchContext fake_dispatch_context;
+  auto bad_manifest = blink::mojom::Manifest::New();
+  bad_manifest->start_url = test_url;
+  bad_manifest->id = test_url;
+  bad_manifest->scope = embedded_test_server()->GetURL("/manifest/");
+
+  blink::mojom::ManifestFileHandlerPtr file_handler =
+      blink::mojom::ManifestFileHandler::New();
+  file_handler->action = test_url;
+
+  std::vector<std::u16string> extensions;
+  extensions.push_back(u".png\u0001");
+  file_handler->accept[u"image/png"] = std::move(extensions);
+
+  bad_manifest->file_handlers.push_back(std::move(file_handler));
+
+  mojo::test::BadMessageObserver bad_message_observer;
+  host->ValidateAndMaybeOverrideManifestForTesting(
+      blink::mojom::ManifestRequestResult::kSuccess, std::move(bad_manifest));
+  EXPECT_THAT(bad_message_observer.WaitForBadMessage(),
+              ::testing::StartsWith(
+                  "Manifest file_handlers accept extension contains invalid "
+                  "control or format characters."));
+}
+
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest,
                        BadMessage_ProtocolHandlersActionCrossOrigin) {
   const GURL test_url =
diff --git a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
index 488fbdcf..3e0076d 100644
--- a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
+++ b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
@@ -3040,6 +3040,62 @@
     ASSERT_EQ(0u, file_handlers.size());
   }
 
+  // Extensions that contain format characters are invalid.
+  {
+    auto& manifest = ParseManifest(
+        R"({
+          "file_handlers": [
+            {
+              "name": "name",
+              "action": "/files",
+              "accept": {
+                "image/png": [
+                  ".png\u202E"
+                ]
+              }
+            }
+          ]
+        })");
+    auto& file_handlers = manifest->file_handlers;
+
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'accept' file extension ignored, contains invalid "
+        "control or format characters.",
+        errors()[0]);
+    EXPECT_EQ("FileHandler ignored. Property 'accept' is invalid.",
+              errors()[1]);
+    ASSERT_EQ(0u, file_handlers.size());
+  }
+
+  // Extensions that contain control characters are invalid.
+  {
+    auto& manifest = ParseManifest(
+        R"({
+          "file_handlers": [
+            {
+              "name": "name",
+              "action": "/files",
+              "accept": {
+                "image/png": [
+                  ".png\u0001"
+                ]
+              }
+            }
+          ]
+        })");
+    auto& file_handlers = manifest->file_handlers;
+
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'accept' file extension ignored, contains invalid "
+        "control or format characters.",
+        errors()[0]);
+    EXPECT_EQ("FileHandler ignored. Property 'accept' is invalid.",
+              errors()[1]);
+    ASSERT_EQ(0u, file_handlers.size());
+  }
+
   // Invalid MIME types and those with parameters are stripped.
   {
     auto& manifest = ParseManifest(