Add blacklist checks for javascript:// ulrs

Investigation in https://bugs.chromium.org/p/chromium/issues/detail?id=913334 showed that blacklists (URLBlacklist and URLWhitelist policies) are applied only to “normal” urls, not ones with “javascript” scheme. This is because this URLs (and some more, like “chrome://crash”) are so-called Renderer Debug URLs, they don't need network fetch or something like that, and they are just passed into renderer. So to make blacklists work for such URLs we have to make additional check. Bug: 913334 Change-Id: I109cf27640d641fc35f449ef0bfd622d19a6c27b Reviewed-on: https://chromium-review.googlesource.com/c/1436372Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Sergey Poromov <poromov@chromium.org> Commit-Queue: Oleg Davydov <burunduk@chromium.org> Cr-Commit-Position: refs/heads/master@{#629135}

Add blacklist checks for javascript:// ulrs
Investigation in https://bugs.chromium.org/p/chromium/issues/detail?id=913334 showed that blacklists (URLBlacklist and URLWhitelist policies) are applied only to “normal” urls, not ones with “javascript” scheme. This is because this URLs (and some more, like “chrome://crash”) are so-called Renderer Debug URLs, they don't need network fetch or something like that, and they are just passed into renderer. So to make blacklists work for such URLs we have to make additional check. Bug: 913334 Change-Id: I109cf27640d641fc35f449ef0bfd622d19a6c27b Reviewed-on: https://chromium-review.googlesource.com/c/1436372Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Sergey Poromov <poromov@chromium.org> Commit-Queue: Oleg Davydov <burunduk@chromium.org> Cr-Commit-Position: refs/heads/master@{#629135}
2cc0167b · Oleg Davydov · Commit Bot · 6864b393 · 2cc0167b · 2cc0167b
Commit 2cc0167b authored Feb 05, 2019 by Oleg Davydov Committed by Commit Bot Feb 05, 2019
7 changed files
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -5453,3 +5453,14 @@ bool ChromeContentBrowserClient::IsBuiltinComponent(
  return false;
 #endif
 }
+
+bool ChromeContentBrowserClient::IsRendererDebugURLBlacklisted(
+    const GURL& url,
+    content::BrowserContext* context) {
+  PolicyBlacklistService* service =
+      PolicyBlacklistFactory::GetForBrowserContext(context);
+
+  using URLBlacklistState = policy::URLBlacklist::URLBlacklistState;
+  URLBlacklistState blacklist_state = service->GetURLBlacklistState(url);
+  return blacklist_state == URLBlacklistState::URL_IN_BLACKLIST;
+}
--- a/chrome/browser/chrome_content_browser_client.h
+++ b/chrome/browser/chrome_content_browser_client.h
@@ -551,6 +551,9 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
  bool IsBuiltinComponent(content::BrowserContext* browser_context,
                          const url::Origin& origin) override;

+  bool IsRendererDebugURLBlacklisted(const GURL& url,
+                                     content::BrowserContext* context) override;
+
  // Determines the committed previews state for the passed in params.
  static content::PreviewsState DetermineCommittedPreviewsForURL(
      const GURL& url,

--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -6432,6 +6432,82 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, WebUsbAllowDevicesForUrls) {
  EXPECT_FALSE(context->HasDevicePermission(kTestUrl, kTestUrl, *device_info));
 }

+// Handler for embedded http-server, returns a small page with javascript
+// variable and a link to increment it. It's for JavascriptBlacklistable test.
+std::unique_ptr<net::test_server::HttpResponse> JSIncrementerPageHandler(
+    const net::test_server::HttpRequest& request) {
+  if (request.relative_url != "/test.html") {
+    return std::unique_ptr<net::test_server::HttpResponse>();
+  }
+
+  std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
+      new net::test_server::BasicHttpResponse());
+  http_response->set_code(net::HTTP_OK);
+  http_response->set_content(
+      "<head><script type=\"text/javascript\">\n"
+      "<!--\n"
+      "var value = 1;"
+      "var increment = function() {"
+      "  value = value + 1;"
+      "};\n"
+      "//-->\n"
+      "</script></head><body>"
+      "<a id='link' href=\"javascript:increment();\">click</a>"
+      "</body>");
+  http_response->set_content_type("text/html");
+  return http_response;
+}
+
+// Fetch value from page generated by JSIncrementerPageHandler.
+int JSIncrementerFetch(content::WebContents* contents) {
+  int result;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractInt(
+      contents, "domAutomationController.send(value);", &result));
+  return result;
+}
+
+// Tests that javascript-links are handled properly according to blacklist
+// settings, bug 913334.
+IN_PROC_BROWSER_TEST_F(PolicyTest, JavascriptBlacklistable) {
+  embedded_test_server()->RegisterRequestHandler(
+      base::BindRepeating(&JSIncrementerPageHandler));
+  ASSERT_TRUE(embedded_test_server()->Start());
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/test.html"));
+
+  EXPECT_EQ(JSIncrementerFetch(contents), 1);
+
+  // Without blacklist policy value is incremented properly.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL("javascript:increment()"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+
+  EXPECT_EQ(JSIncrementerFetch(contents), 2);
+
+  // Create and apply a policy.
+  base::ListValue blacklist;
+  blacklist.AppendString("javascript://*");
+  PolicyMap policies;
+  policies.Set(key::kURLBlacklist, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
+               POLICY_SOURCE_CLOUD, blacklist.CreateDeepCopy(), nullptr);
+  UpdateProviderPolicy(policies);
+  FlushBlacklistPolicy();
+
+  // After applying policy javascript url's don't work any more, value leaves
+  // unchanged.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL("javascript:increment()"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+  EXPECT_EQ(JSIncrementerFetch(contents), 2);
+
+  // But in-page links still work even if they are javascript-links.
+  EXPECT_TRUE(content::ExecuteScript(
+      contents, "document.getElementById('link').click();"));
+  EXPECT_EQ(JSIncrementerFetch(contents), 3);
+}
+
 // Similar to PolicyTest but sets the WebAppInstallForceList policy before the
 // browser is started.
 class WebAppInstallForceListPolicyTest : public PolicyTest {

--- a/components/policy/content/policy_blacklist_service.h
+++ b/components/policy/content/policy_blacklist_service.h
@@ -21,8 +21,7 @@ class URLChecker;
 // PolicyBlacklistService and PolicyBlacklistFactory provide a way for
 // us to access URLBlacklistManager, a policy block list service based on
 // the Preference Service. The URLBlacklistManager responds to permission
-// changes and is per-Profile. The PolicyBlacklistNavigationThrottle accesses
-// this service to determine what we should block.
+// changes and is per-Profile.
 class PolicyBlacklistService : public KeyedService {
 public:
  using CheckSafeSearchCallback = base::OnceCallback<void(bool is_safe)>;

--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -2673,6 +2673,14 @@ void NavigationControllerImpl::NavigateWithoutEntry(
  // URLs, unlike the cases below where we clear it if the navigation doesn't
  // proceed.
  if (IsRendererDebugURL(params.url)) {
+    // Renderer-debug URLs won't go through NavigationThrottlers so we have to
+    // check them explicitly. See bug 913334.
+    if (GetContentClient()->browser()->IsRendererDebugURLBlacklisted(
+            params.url, browser_context_)) {
+      DiscardPendingEntry(false);
+      return;
+    }
+
    HandleRendererDebugURL(node, params.url);
    return;
  }

--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -920,4 +920,10 @@ bool ContentBrowserClient::IsBuiltinComponent(BrowserContext* browser_context,
  return false;
 }

+bool ContentBrowserClient::IsRendererDebugURLBlacklisted(
+    const GURL& url,
+    BrowserContext* context) {
+  return false;
+}
+
 }  // namespace content
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -1448,6 +1448,12 @@ class CONTENT_EXPORT ContentBrowserClient {
  // to native code, and as such whether its log messages should be recorded.
  virtual bool IsBuiltinComponent(BrowserContext* browser_context,
                                  const url::Origin& origin);
+
+  // Returns whether given |url| has to be blocked. It's used only for renderer
+  // debug URLs, as other requests are handled via NavigationThrottlers and
+  // blacklist policies are applied there.
+  virtual bool IsRendererDebugURLBlacklisted(const GURL& url,
+                                             BrowserContext* context);
 };

 }  // namespace content