Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(39)

Issue 1887553002: Make sure binding security checks don't pass if the frame is remote. (Closed)

Created:
3 years, 4 months ago by dcheng
Modified:
3 years, 4 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, esprehn, jam, nasko+codewatch_chromium.org, ncarter (slow), site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make sure binding security checks don't pass if the frame is remote. Blink assumes that remote frames will always fail the security origin check. Unfortunately, reality is not that simple. There are several instances where this assumption fails to hold. For example: 1. Navigate to a.com. 2. a.com opens a new window. 3. Navigate the new window to b.com via the omnibox. 4. Click a link to c.com in both windows. Because browser-initiated navigations go cross-process but renderer-initiated navigations do not [1], the two c.com windows will end up in different renderer processes. Both windows have the same origin but see each other as RemoteFrames. This means that SecurityOrigin's canAccess check will pass… but this ends up violating many assumptions in Blink that passing the security check implies a local frame. [1] https://www.chromium.org/developers/design-documents/process-models#Caveats BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/f23b8e77a83a5aafabf64acf723cf2ac02c5cf0e Cr-Commit-Position: refs/heads/master@{#387087}

Patch Set 1 #

Patch Set 2 : . #

Total comments: 11

Patch Set 3 : address comments / add more comments #

Total comments: 4

Patch Set 4 : Test fixes for --site-per-process and more periods. #

Total comments: 2

Patch Set 5 : . #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -3 lines) Patch
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 4 2 chunks +71 lines, -0 lines 2 comments Download
M third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp View 1 2 3 2 chunks +10 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/frame/DOMWindow.cpp View 1 2 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 39 (15 generated)
dcheng
Here's one possible mitigation. One thing I'm worried about is perf: I believe we try ...
3 years, 4 months ago (2016-04-13 04:27:36 UTC) #8
haraken
LGTM https://codereview.chromium.org/1887553002/diff/20001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (left): https://codereview.chromium.org/1887553002/diff/20001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp#oldcode53 third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:53: if (isOriginAccessibleFromDOMWindow(targetFrameOrigin, accessingWindow)) isOriginAccessibleFromDOMWindow has already been heavy, ...
3 years, 4 months ago (2016-04-13 04:43:20 UTC) #10
Yuki
I'm not sure if this is the right fix or not. If the both subframes ...
3 years, 4 months ago (2016-04-13 05:48:53 UTC) #11
dcheng
On 2016/04/13 at 05:48:53, yukishiino wrote: > I'm not sure if this is the right ...
3 years, 4 months ago (2016-04-13 06:18:09 UTC) #12
dcheng
Also, it looks like this is failing an assert so I need to figure out ...
3 years, 4 months ago (2016-04-13 06:18:31 UTC) #13
Charlie Reis
Thanks for the quick fix! Some nits on the test. Also, it looks like we're ...
3 years, 4 months ago (2016-04-13 06:18:33 UTC) #14
dcheng
https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_host/render_frame_host_manager_browsertest.cc File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_host/render_frame_host_manager_browsertest.cc#newcode2622 content/browser/frame_host/render_frame_host_manager_browsertest.cc:2622: // When two frames are same-origin but cross-process; they ...
3 years, 4 months ago (2016-04-13 06:33:30 UTC) #15
dcheng
https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_host/render_frame_host_manager_browsertest.cc File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_host/render_frame_host_manager_browsertest.cc#newcode2653 content/browser/frame_host/render_frame_host_manager_browsertest.cc:2653: // Clear the opener. On 2016/04/13 at 06:18:33, Charlie ...
3 years, 4 months ago (2016-04-13 06:33:49 UTC) #16
esprehn
Doesn't this break the web if those are RemoteFrames? I'm pretty sure you can reach ...
3 years, 4 months ago (2016-04-13 06:45:55 UTC) #18
Yuki
Thanks for the explanation and adding comments. Since this is a workaround to avoid crashes ...
3 years, 4 months ago (2016-04-13 07:08:12 UTC) #19
dcheng
On 2016/04/13 at 06:45:55, esprehn wrote: > Doesn't this break the web if those are ...
3 years, 4 months ago (2016-04-13 07:59:57 UTC) #20
dcheng
On 2016/04/13 at 07:59:57, dcheng wrote: > On 2016/04/13 at 06:45:55, esprehn wrote: > > ...
3 years, 4 months ago (2016-04-13 08:02:58 UTC) #21
Charlie Reis
On 2016/04/13 06:45:55, esprehn wrote: > Doesn't this break the web if those are RemoteFrames? ...
3 years, 4 months ago (2016-04-13 16:45:57 UTC) #22
dcheng
> > Why are there RemoteFrame instances when OOPIF is disabled? > Daniel's answer is ...
3 years, 4 months ago (2016-04-13 17:13:01 UTC) #23
Charlie Reis
On 2016/04/13 17:13:01, dcheng wrote: > FWIW, if you can think of a more concise ...
3 years, 4 months ago (2016-04-13 17:43:25 UTC) #24
dcheng
> The subframe case is intuitive, but Elliot's right that we don't have RemoteFrames for ...
3 years, 4 months ago (2016-04-13 18:07:04 UTC) #25
dcheng
Updated the comment: in fact, we don't currently replicate the frame tree in default Chrome ...
3 years, 4 months ago (2016-04-13 18:21:59 UTC) #28
Charlie Reis
Thanks! content/ and CL description LGTM.
3 years, 4 months ago (2016-04-13 18:25:28 UTC) #29
ncarter (slow)
lgtm https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_host/render_frame_host_manager_browsertest.cc File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_host/render_frame_host_manager_browsertest.cc#newcode2688 content/browser/frame_host/render_frame_host_manager_browsertest.cc:2688: } The test looks good. There is some ...
3 years, 4 months ago (2016-04-13 18:29:17 UTC) #31
esprehn
lgtm
3 years, 4 months ago (2016-04-13 19:42:23 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887553002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887553002/60001
3 years, 4 months ago (2016-04-13 19:43:38 UTC) #35
dcheng
https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_host/render_frame_host_manager_browsertest.cc File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_host/render_frame_host_manager_browsertest.cc#newcode2688 content/browser/frame_host/render_frame_host_manager_browsertest.cc:2688: } On 2016/04/13 at 18:29:17, ncarter wrote: > The ...
3 years, 4 months ago (2016-04-13 19:44:58 UTC) #36
commit-bot: I haz the power
Committed patchset #5 (id:60001)
3 years, 4 months ago (2016-04-13 21:08:30 UTC) #37
commit-bot: I haz the power
3 years, 4 months ago (2016-04-13 21:11:33 UTC) #39
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/f23b8e77a83a5aafabf64acf723cf2ac02c5cf0e
Cr-Commit-Position: refs/heads/master@{#387087}

Powered by Google App Engine
This is Rietveld 408576698