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

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

Created:
4 years, 1 month ago by dcheng
Modified:
4 years, 1 month 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 ...
4 years, 1 month 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, ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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: > > ...
4 years, 1 month 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? ...
4 years, 1 month ago (2016-04-13 16:45:57 UTC) #22
dcheng
> > Why are there RemoteFrame instances when OOPIF is disabled? > Daniel's answer is ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month ago (2016-04-13 18:21:59 UTC) #28
Charlie Reis
Thanks! content/ and CL description LGTM.
4 years, 1 month 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 ...
4 years, 1 month ago (2016-04-13 18:29:17 UTC) #31
esprehn
lgtm
4 years, 1 month 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
4 years, 1 month 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 ...
4 years, 1 month ago (2016-04-13 19:44:58 UTC) #36
commit-bot: I haz the power
Committed patchset #5 (id:60001)
4 years, 1 month ago (2016-04-13 21:08:30 UTC) #37
commit-bot: I haz the power
4 years, 1 month 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