New issue
Advanced search Search tips

Issue 914261 link

Starred by 1 user
Status: Fixed
Owner:
falken@chromium.org
Closed: Dec 14
Cc:
yhirano@chromium.org
toyoshim@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 911974



Sign in to add a comment

resource_loader should call SecurityPolicy::IsOriginAccessAllowed()

Project Member Reported by falken@chromium.org, Dec 12 
My CL at https://chromium-review.googlesource.com/c/chromium/src/+/1367331 tries to change CSS's same-origin check to depend entirely on FetchResponseType.

But many Chrome OS browser_tests fail because a stylesheet is unexpectedly considered cross-origin.

With the CL (and debug output):
From CanAccessRules():
denying: "chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj" access to "chrome://resources/css/action_link.css"

This happens because action_link.css is considered CORS-cross-origin by resource_loader.

Without the CL:
From CanAccessRules():
css_style_sheet.cc(351)] checking: "chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj" access to "chrome://resources/css/action_link.css"
security_origin.cc(399)] --> yes! seucrity policy
css_style_sheet.cc(354)] --> ALLOWED!

This happens CanAccessRules() calls document->GetSecurityOrigin()->CanReadContent(base_url) instead of looking at FetchResponseType. This returns true because 
SecurityPolicy::IsOriginAccessAllowed(this, target_origin.get()) returns true for the chrome-extension:// document to access to the target chrome:// resource.


It looks like ResourceLoader doesn't use the SecurityPolicy to set FetchResponseType.  It uses cors::CalculateResponseTainting which uses network::cors::CalculateResponseTainting which does a pure origin check.

Comment 1 by falken@chromium.org, Dec 12 

You can check this locally by patching in the CL and building Chrome OS browser_tests and running:

$ out/cros/browser_tests --gtest_filter="FileDisplay/FilesAppBrowserTest.Test/fileDisplayDriveOffline_DriveFs"

Comment 2 by falken@chromium.org, Dec 13 

Note to self.

When OOBCORS is off, the type is set to opaque in blink::ResourceLoader::DidReceiveResponse:
 response_with_type->SetType(response_tainting_);

When OOBCORS is on, the type is set to opaque in network::cors::CorsURLLoader::OnReceiveResponse:
  response_head_to_pass.response_type = response_tainting_;

Comment 3 by falken@chromium.org, Dec 13

Blocking: 911974
Owner: falken@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff9707c095926abd5470537a146abcddb979e192

commit ff9707c095926abd5470537a146abcddb979e192
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Dec 13 08:33:28 2018

Split network::cors::CalculateResponseTainting into Blink and network variants.

Previously, Blink was using network::cors::CalculateResponseTainting,
by converting SecurityOrigin to url::Origin. This discarded info about
origins that are whitelisted by SecurityPolicy. So the response type
was becoming opaque when an extension requested a URL that it should
have access to according to the SecurityPolicy.

Originally it was thought that url::Origin was sufficient because
Blink's CalculateCorsFlag() was supposed to check whether the
origin has access when doing a CORS request.

However, it turns out the SecurityPolicy whitelist is expected to allow
cross-origin access even in no-CORS request mode, so CalculateCorsFlag()
wasn't effective.

The solution is to split network::cors::CalculateResponseTainting into
blink::CalculateResponseTainting and
network::cors::CorsURLLoader::CalculateResponseTainting. Blink's
version uses SecurityOrigin to check for access.

Bug:  914261 
Change-Id: Iff56e3fc08f14b163d8fef425caeca42e1b6f3e0
Reviewed-on: https://chromium-review.googlesource.com/c/1375297
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616246}
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/cors/cors_url_loader.cc
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/cors/cors_url_loader.h
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/cors/cors_url_loader_unittest.cc
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/public/cpp/cors/cors.cc
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/public/cpp/cors/cors.h
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/services/network/public/cpp/cors/cors_unittest.cc
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/third_party/blink/renderer/platform/loader/cors/cors.cc
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/third_party/blink/renderer/platform/loader/cors/cors.h
[modify] https://crrev.com/ff9707c095926abd5470537a146abcddb979e192/third_party/blink/renderer/platform/loader/cors/cors_test.cc

Comment 5 by falken@chromium.org, Dec 14

Labels: M-73
Status: Fixed (was: Started)
Thanks yhirano for help.
►

Sign in to add a comment