OOPIF: content settings for images and scripts in OOPIFs don't respect file paths and might crash |
|||||||||||||||
Issue description
What steps will reproduce the problem?
(0) Create a file /tmp/foo.html containing
<img src="http://www.asdf.com/89asdf.gif">
(1) Start Chrome with --site-per-process
(2) Go to chrome://settings/content. Click "Manager exceptions..." and add a rule for "file:///tmp/foo.html" -> "Block"
(3) Open a new tab and go to file:///tmp/foo.html
Observe that the image is blocked, confirming that content settings took effect.
(4) Now open devtools and execute:
var f=document.createElement("iframe");
f.src="http://www.asdf.com";
document.body.appendChild(f);
What is the expected output?
The images in the iframe don't load, respecting the fact that the top frame is at a URL that's blacklisted in content settings. This is what happens without --site-per-process.
What do you see instead?
The images in the iframe actually load.
The fact that this doesn't work is a known problem; this bug just documents the specifics. The reason is in GetOriginOrURL in content_settings_observer.cc:
GURL GetOriginOrURL(const WebFrame* frame) {
WebString top_origin = frame->top()->getSecurityOrigin().toString();
// The |top_origin| is unique ("null") e.g., for file:// URLs. Use the
// document URL as the primary URL in those cases.
// TODO(alexmos): This is broken for --site-per-process, since top() can be a
// WebRemoteFrame which does not have a document(), and the WebRemoteFrame's
// URL is not replicated.
if (top_origin == "null")
return frame->top()->document().url();
return blink::WebStringToGURL(top_origin);
}
It's probably difficult to find a way to fix the file path-based matching. We've hit this problem before for plugins and popups, and for popups we ended up making a change to always use the origin, see https://codereview.chromium.org/1168913002/ and discussion at https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/JPKPFfZpVHE/discussion. I'm curious if anything prevents us from doing the same here.
Note that there's actually no crash when this is invoked from an OOPIF with a top frame that's at a file URL, as in that case, the top_origin actually shows up as "file://" rather than "null". This is because we don't replicate the m_blockLocalAccessFromLocalOrigin bit from the SecurityOrigin, which is what makes SecurityOrigin::toString() return "null" for a file URL. This is also a known problem - we also don't replicate some other things in the origin, such as document.domain, because we haven't seen a good reason to do so. We could fix this if we wanted to, but that won't help with the core issue of not having the top frame's URL above.
However, there's a crash if you do the above from a sandboxed main frame, or from a data URL, where the origin will actually serialize as "null". For example, with --site-per-process, going to
data:text/html,<iframe src='http://www.asdf.com'></iframe>
while having at least one image content settings rule defined crashes in
#8 0x7f4fc060be13 (anonymous namespace)::GetOriginOrURL()
#9 0x7f4fc060a076 (anonymous namespace)::GetContentSettingFromRules()
#10 0x7f4fc06099c2 ContentSettingsObserver::allowImage()
#11 0x7f4fad869edd blink::FrameLoaderClientImpl::allowImage()
#12 0x7f4fa32bf0a1 blink::FrameFetchContext::allowImage()
#13 0x7f4fa30b77c5 blink::ResourceFetcher::shouldDeferImageLoad()
#14 0x7f4fa30b771d blink::ResourceFetcher::resourceNeedsLoad()
#15 0x7f4fa30baee6 blink::ResourceFetcher::requestResource()
#16 0x7f4fa308e698 blink::ImageResource::fetch()
,
Sep 12 2016
Users experienced this crash on the following builds: Mac Beta 54.0.2840.16 - 0.30 CPM, 6 reports, 4 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Sep 13 2016
[adding a few folks from related discussion threads] Now that we're getting crashes in GetOriginOrURL() in our trial due to this, we need to take some action here. Two things we could do: 1) Remove GetOriginOrURL altogether, and always use the top frame's origin as primary_url for checking content settings here. This would break support for file path exceptions for images, scripts, and autoplay. 2) Fall back to document().url() if the top-level frame is local. This won't help support file path exceptions in OOPIF scenarios, but it will stop the crash and kick the can down the road until we can fix issue 621724 and convert content settings to use origins instead of GURLs. Thoughts? I'd strongly prefer to do 1), because: - we've done this for plugins (see r345104) and nobody noticed - issue 621724 will eventually do it anyway for all content settings (hopefully!) - supporting file path exceptions with OOPIFs is not really possible today (see discussion links in original description), since we don't know a remote frame's URL. Given that --isolate-extensions is now on by default, I'd prefer us to have the same behavior with and without OOPIFs. - mkwst@ was in favor of killing support for file URLs in content settings altogether, and nobody seemed to object too strongly :) But that does mean that the file path example in the original description won't work anymore. We've got some UMA data for ContentSettings.ExceptionScheme, which suggests that file: exceptions are rare (<1%) but I don't know if those numbers are considered negligible enough to kill it. Further, there's no data AFAICT on how many of those exceptions have a scheme-wide file:///* exception vs. an explicit path (we can continue to support the former but not the latter). lshang@: any opinions on this?
,
Sep 13 2016
> mkwst@ was in favor of killing support for file URLs in content settings altogether, and nobody seemed to object too strongly :) Note that I'm in favor of killing support for `file:` URLs in content settings by removing the ability to do anything in `file:` URLs that requires a content setting. I'm not as excited about removing content settings in a way that fails open. :)
,
Sep 16 2016
Users experienced this crash on the following builds: Mac Dev 55.0.2859.0 - 0.25 CPM, 1 reports, 1 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Sep 20 2016
> We've got some UMA data for ContentSettings.ExceptionScheme, which suggests that file: exceptions are rare (<1%) but I don't know if those numbers are considered negligible enough to kill it. Further, there's no data AFAICT on how many of those exceptions have a scheme-wide file:///* exception vs. an explicit path (we can continue to support the former but not the latter). lshang@: any opinions on this? Yeah that metric is added mainly to see how many file exceptions are used now, and currently it only tells what kind of schemes an exception has. But we can use ContentSettingsPattern.PatternParts.path (https://cs.chromium.org/chromium/src/components/content_settings/core/common/content_settings_pattern.h?sq=package:chromium&l=101) to see if a file exception has an explicit path or just wildcard. That's not hard to get.
,
Sep 20 2016
#6: Thanks, I'll see if I can add an UMA stat for this. In the meantime, I plan to land a short-term fix to avoid the crash in GetOriginOrURL in OOPIF modes.
,
Sep 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78aed40a1509c6cab064f9684e9653b37daff826 commit 78aed40a1509c6cab064f9684e9653b37daff826 Author: alexmos <alexmos@chromium.org> Date: Tue Sep 20 23:52:04 2016 Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames. The intent of the fallback to top()->document().url() in GetOriginOrURL() is to support file path matching for content setting exceptions in pages loaded from a file: scheme. This fallback can cause crashes in OOPIF modes when the top frame is remote, even in cases that have nothing to do with file: exceptions, such as a sandboxed main frame (which has a "null" origin) embedding an OOPIF. Longer-term, local and remote frames should be treated the same way for content settings exceptions for the file: scheme; and content settings will be refactored to be based on origins rather than GURLs in issue 621724. In the short term though, avoid the crash by falling back to document->url() only for local top frames. This shouldn't actually affect file exceptions, as --isolate-extensions, which is the only OOPIF mode currently enabled by default on trunk, won't put subframes inside file: pages into a separate process. BUG=628759, 466297 Review-Url: https://codereview.chromium.org/2354083002 Cr-Commit-Position: refs/heads/master@{#419899} [modify] https://crrev.com/78aed40a1509c6cab064f9684e9653b37daff826/chrome/renderer/content_settings_observer.cc
,
Sep 21 2016
Requesting to merge r419899 to beta to mitigate the GetOriginOrURL crashes there (these crashes are now happening more frequently on ChromeOS - see issue 644301).
,
Sep 21 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47261887fbe5c3826a830500a3623fc396d4ec73 commit 47261887fbe5c3826a830500a3623fc396d4ec73 Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Sep 21 23:58:30 2016 Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames. The intent of the fallback to top()->document().url() in GetOriginOrURL() is to support file path matching for content setting exceptions in pages loaded from a file: scheme. This fallback can cause crashes in OOPIF modes when the top frame is remote, even in cases that have nothing to do with file: exceptions, such as a sandboxed main frame (which has a "null" origin) embedding an OOPIF. Longer-term, local and remote frames should be treated the same way for content settings exceptions for the file: scheme; and content settings will be refactored to be based on origins rather than GURLs in issue 621724. In the short term though, avoid the crash by falling back to document->url() only for local top frames. This shouldn't actually affect file exceptions, as --isolate-extensions, which is the only OOPIF mode currently enabled by default on trunk, won't put subframes inside file: pages into a separate process. BUG=628759, 466297 Review-Url: https://codereview.chromium.org/2354083002 Cr-Commit-Position: refs/heads/master@{#419899} (cherry picked from commit 78aed40a1509c6cab064f9684e9653b37daff826) Review URL: https://codereview.chromium.org/2360903002 . Cr-Commit-Position: refs/branch-heads/2840@{#481} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/47261887fbe5c3826a830500a3623fc396d4ec73/chrome/renderer/content_settings_observer.cc
,
Sep 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37df8891dff743a4a693575058ce22ca22adbe2a commit 37df8891dff743a4a693575058ce22ca22adbe2a Author: alexmos <alexmos@chromium.org> Date: Tue Sep 27 17:27:08 2016 Add metrics for paths in file scheme exceptions in content settings. These will tell how often file scheme exceptions have a non-empty path vs. a wildcard path, and which content settings types use them. BUG=628759, 621724 Review-Url: https://codereview.chromium.org/2367683002 Cr-Commit-Position: refs/heads/master@{#421248} [modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/browser/host_content_settings_map.cc [modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern.cc [modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern.h [modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern_unittest.cc [modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/tools/metrics/histograms/histograms.xml
,
Sep 27 2016
Requesting to merge r421248 to M54. This is a simple change to add metrics for content settings exceptions for file URLs, with no behavior change. Merging it will give us data from stable much sooner.
,
Sep 28 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 28 2016
Please merge your change to M54 (branch: 2840) ASAP so that we could take this for next Beta Release.
,
Sep 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a60ed7ff6c56075b5b3ae018d527f397c2869ca9 commit a60ed7ff6c56075b5b3ae018d527f397c2869ca9 Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Sep 28 18:33:22 2016 Add metrics for paths in file scheme exceptions in content settings. These will tell how often file scheme exceptions have a non-empty path vs. a wildcard path, and which content settings types use them. BUG=628759, 621724 Review-Url: https://codereview.chromium.org/2367683002 Cr-Commit-Position: refs/heads/master@{#421248} (cherry picked from commit 37df8891dff743a4a693575058ce22ca22adbe2a) Review URL: https://codereview.chromium.org/2375183002 . Cr-Commit-Position: refs/branch-heads/2840@{#566} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/browser/host_content_settings_map.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.h [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern_unittest.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/tools/metrics/histograms/histograms.xml
,
Oct 9 2016
Users experienced this crash on the following builds: Mac Canary 56.0.2884.0 - 0.25 CPM, 1 reports, 1 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47261887fbe5c3826a830500a3623fc396d4ec73 commit 47261887fbe5c3826a830500a3623fc396d4ec73 Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Sep 21 23:58:30 2016 Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames. The intent of the fallback to top()->document().url() in GetOriginOrURL() is to support file path matching for content setting exceptions in pages loaded from a file: scheme. This fallback can cause crashes in OOPIF modes when the top frame is remote, even in cases that have nothing to do with file: exceptions, such as a sandboxed main frame (which has a "null" origin) embedding an OOPIF. Longer-term, local and remote frames should be treated the same way for content settings exceptions for the file: scheme; and content settings will be refactored to be based on origins rather than GURLs in issue 621724. In the short term though, avoid the crash by falling back to document->url() only for local top frames. This shouldn't actually affect file exceptions, as --isolate-extensions, which is the only OOPIF mode currently enabled by default on trunk, won't put subframes inside file: pages into a separate process. BUG=628759, 466297 Review-Url: https://codereview.chromium.org/2354083002 Cr-Commit-Position: refs/heads/master@{#419899} (cherry picked from commit 78aed40a1509c6cab064f9684e9653b37daff826) Review URL: https://codereview.chromium.org/2360903002 . Cr-Commit-Position: refs/branch-heads/2840@{#481} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/47261887fbe5c3826a830500a3623fc396d4ec73/chrome/renderer/content_settings_observer.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a60ed7ff6c56075b5b3ae018d527f397c2869ca9 commit a60ed7ff6c56075b5b3ae018d527f397c2869ca9 Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Sep 28 18:33:22 2016 Add metrics for paths in file scheme exceptions in content settings. These will tell how often file scheme exceptions have a non-empty path vs. a wildcard path, and which content settings types use them. BUG=628759, 621724 Review-Url: https://codereview.chromium.org/2367683002 Cr-Commit-Position: refs/heads/master@{#421248} (cherry picked from commit 37df8891dff743a4a693575058ce22ca22adbe2a) Review URL: https://codereview.chromium.org/2375183002 . Cr-Commit-Position: refs/branch-heads/2840@{#566} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/browser/host_content_settings_map.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.h [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern_unittest.cc [modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/tools/metrics/histograms/histograms.xml
,
Nov 20 2016
Users experienced this crash on the following builds: Mac Canary 57.0.2925.0 - 0.37 CPM, 1 reports, 1 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Dec 5 2016
,
Feb 7 2017
Users experienced this crash on the following builds: Linux Dev 58.0.3000.4 - 0.58 CPM, 1 reports, 1 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
May 31 2017
Users experienced this crash on the following builds: Mac Canary 61.0.3115.0 - 0.53 CPM, 1 reports, 1 clients (signature blink::WebString::WebString) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by alex...@chromium.org
, Sep 12 2016