New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 615910 link

Starred by 1 user
Status: Fixed
Owner:
█ mkwst@chromium.org
Buried. Ping if important.
Closed: Jun 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug-Security

Blocking:
issue 617584



Sign in to add a comment

upgrade-insecure-requests is not upgrading iframe sources

Reported by tollm...@gmail.com, May 30 2016 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2751.0 Safari/537.36

Steps to reproduce the problem:
1. Generate a page with the following CSP header: `default-src https:; upgrade-insecure-requests`
2. Generate a simple page with an HTTP iframe source that can also accept an HTTPS request. I've attached the HTML I used to test for this condition. Ensure that this page is protected with the CSP header from step 1.
3. Remove, and possibly flush, any HSTS headers set for the test domain.

What is the expected behavior?
`upgrade-insecure-requests` should upgrade the "http://domain.com/iframe-src" source to "https://domain.com/iframe-src" and successfully frame the resource.

What went wrong?
The "http://domain.com/iframe-src" resource is blocked by the CSP policy and the following error is displayed in the console:

Refused to frame 'http://domain.com/iframe-src' because it violates the following Content Security Policy directive: "default-src https:". Note that 'frame-src' was not explicitly set, so 'default-src' is used as a fallback.

Did this work before? No 

Chrome version: 53.0.2751.0  Channel: canary
OS Version: OS X 10.11.4
Flash Version: Shockwave Flash 22.0 r0

I've tested in Chrome Stable and Canary and get the same results. I get the expected behavior in Firefox Stable and Nightly.

Note that I added an `img` tag `src` in my test as well to verify that there wasn't something in my test that caused UIR to break. When using an HTTP `img` `src`, it resource is successfully upgraded, suggesting that upgrades are working on the page, just not for iframes.
iframe-uir-test.html
152 bytes View Download

Comment 1 by mkwst@chromium.org, May 31 2016

Components: Blink>SecurityFeature
Labels: -Restrict-View-SecurityTeam
Owner: mkwst@chromium.org
Status: Started (was: Unconfirmed)
It looks like we're doing the upgrade after checking CSP, which is unfortunate. I'll poke at it.

Removing view restrictions again, as we're failing closed.

Comment 2 by mkwst@chromium.org, May 31 2016 

(For clarity: I'm pretty sure that the frame _would_ be upgraded if you dropped the `frame-src https:` bit. The ordering of the CSP check is the issue here, not the UIR behavior in general.)

Comment 3 by mkwst@chromium.org, May 31 2016 

https://codereview.chromium.org/2022083002 is up for review.

Comment 4 by tollm...@gmail.com, May 31 2016 

> I'm pretty sure that the frame _would_ be upgraded if you dropped the `frame-src https:` bit

I can confirm that this is true. With a CSP header of `upgrade-insecure-requests`, the iframe is properly upgraded.

I agree that my report is a mischaracterization of the issue. Thanks for sorting that out!

Comment 5 by mea...@chromium.org, May 31 2016

Labels: Security_Impact-Stable Security_Severity-Low

Comment 6 by mkwst@chromium.org, Jun 6 2016 

Sorry this has taken so long; after an iteration or three, I think the patch should be in shape to land early this week.

Comment 7 by mkwst@chromium.org, Jun 7 2016

Blocking: 617584
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 8 2016 

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

commit 3f3e725e6479c711e5e13e59e8d011ee89992119
Author: mkwst <mkwst@chromium.org>
Date: Wed Jun 08 21:34:51 2016

Move 'frame-src' CSP checks into FrameFetchContext.

Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
ends up being too early in the navigation cycle to handle the results
of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
loading code in the last ~3 years that we can pretty easily move this
check into 'FrameFetchContext::canRequest' alongside the rest of CSP.

BUG= 615910 

Review-Url: https://codereview.chromium.org/2022083002
Cr-Commit-Position: refs/heads/master@{#398685}

[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt
[add] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/LayoutTests/http/tests/security/upgrade-insecure-requests/iframe-upgrade-csp.https.html
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/DocumentLoader.h
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[add] https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119/third_party/WebKit/Source/web/tests/data/iframe_redirect_blocked.html

Comment 9 by mkwst@chromium.org, Jun 9 2016

Status: Fixed (was: Started)
Should be fixed in the next canary.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 13 2016 

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

commit 13632cf2d8145ab0785011bc4e1c7254914395fc
Author: tommycli <tommycli@chromium.org>
Date: Mon Jun 13 21:54:19 2016

Revert of Move 'frame-src' CSP checks into FrameFetchContext. (patchset #5 id:100001 of https://codereview.chromium.org/2022083002/ )

Reason for revert:
Speculative revert for breaking Dr. Memory:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20Browser%20%28DrMemory%20full%29%20%284%29?numbuilds=200

First breaking build:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20Browser%20%28DrMemory%20full%29%20%284%29/builds/8028

Original issue's description:
> Move 'frame-src' CSP checks into FrameFetchContext.
>
> Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
> ends up being too early in the navigation cycle to handle the results
> of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
> loading code in the last ~3 years that we can pretty easily move this
> check into 'FrameFetchContext::canRequest' alongside the rest of CSP.
>
> BUG= 615910 
>
> Committed: https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119
> Cr-Commit-Position: refs/heads/master@{#398685}

TBR=jialiul@chromium.org,alexmos@chromium.org,creis@chromium.org,dcheng@chromium.org,japhet@chromium.org,jochen@chromium.org,lukasza@chromium.org,nasko@chromium.org,nparker@chromium.org,yoav@yoav.ws,mkwst@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 615910 

Review-Url: https://codereview.chromium.org/2068443002
Cr-Commit-Position: refs/heads/master@{#399561}

[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt
[delete] https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e/third_party/WebKit/LayoutTests/http/tests/security/upgrade-insecure-requests/iframe-upgrade-csp.https.html
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/DocumentLoader.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[delete] https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e/third_party/WebKit/Source/web/tests/data/iframe_redirect_blocked.html
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 15 2016 

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

commit 13632cf2d8145ab0785011bc4e1c7254914395fc
Author: tommycli <tommycli@chromium.org>
Date: Mon Jun 13 21:54:19 2016

Revert of Move 'frame-src' CSP checks into FrameFetchContext. (patchset #5 id:100001 of https://codereview.chromium.org/2022083002/ )

Reason for revert:
Speculative revert for breaking Dr. Memory:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20Browser%20%28DrMemory%20full%29%20%284%29?numbuilds=200

First breaking build:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20Browser%20%28DrMemory%20full%29%20%284%29/builds/8028

Original issue's description:
> Move 'frame-src' CSP checks into FrameFetchContext.
>
> Currently, we're doing 'frame-src' checks inside 'FrameLoader', which
> ends up being too early in the navigation cycle to handle the results
> of 'upgrade-insecure-requests'. Happily, we've refactored enough of the
> loading code in the last ~3 years that we can pretty easily move this
> check into 'FrameFetchContext::canRequest' alongside the rest of CSP.
>
> BUG= 615910 
>
> Committed: https://crrev.com/3f3e725e6479c711e5e13e59e8d011ee89992119
> Cr-Commit-Position: refs/heads/master@{#398685}

TBR=jialiul@chromium.org,alexmos@chromium.org,creis@chromium.org,dcheng@chromium.org,japhet@chromium.org,jochen@chromium.org,lukasza@chromium.org,nasko@chromium.org,nparker@chromium.org,yoav@yoav.ws,mkwst@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 615910 

Review-Url: https://codereview.chromium.org/2068443002
Cr-Commit-Position: refs/heads/master@{#399561}

[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt
[delete] https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e/third_party/WebKit/LayoutTests/http/tests/security/upgrade-insecure-requests/iframe-upgrade-csp.https.html
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/DocumentLoader.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/13632cf2d8145ab0785011bc4e1c7254914395fc/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[delete] https://crrev.com/3bba4159d276c9513d8ba25d742ceb32a0c8075e/third_party/WebKit/Source/web/tests/data/iframe_redirect_blocked.html
Project Member

Comment 12 by sheriffbot@chromium.org, Oct 1 2016 

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 13 by sheriffbot@chromium.org, Oct 1 2016

Labels: Restrict-View-SecurityNotify
Project Member

Comment 14 by sheriffbot@chromium.org, Oct 2 2016

Labels: -Restrict-View-SecurityNotify
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 15 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic
►

Sign in to add a comment