Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887553002:
Make sure binding security checks don't pass if the frame is remote. (Closed)
|
Created:
4 years, 8 months ago by dcheng Modified:
4 years, 8 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. |
DescriptionMake 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
Messages
Total messages: 39 (15 generated)
Description was changed from ========== Make sure binding security checks don't pass if the frame is remote. BUG=601629 ========== to ========== Make sure binding security checks don't pass if the frame is remote. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Make sure binding security checks don't pass if the frame is remote. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this simply won't be true. One example: 1. [Window 1] navigates to a.com, which has a b.com subframe. 2. a.com in [Window 1] opens a new window, [Window 2]. 3. [Window 2] navigates to c.com, which also a b.com subframe. 4. The navigation in [Window 2] causes a process swap, so [Window 1] and [Window 2] are now in separate processes. 5. Without out-of-process iframes, all the frames in a page are in the same process. 6. Thus, b.com in [Window 1] cannot script b.com in [Window 2], even though they are same origin. Note that this has always been true in Chrome, so there's no behavior change. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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 never that simple. There are several instances where this simply won't be true. One example: 1. [Window 1] navigates to a.com, which has a b.com subframe. 2. a.com in [Window 1] opens a new window, [Window 2]. 3. [Window 2] navigates to c.com, which also a b.com subframe. 4. The navigation in [Window 2] causes a process swap, so [Window 1] and [Window 2] are now in separate processes. 5. Without out-of-process iframes, all the frames in a page are in the same process. 6. Thus, b.com in [Window 1] cannot script b.com in [Window 2], even though they are same origin. Note that this has always been true in Chrome, so there's no behavior change. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. [Window 1] opens [Window 2]. 2. [Window 2] is navigated cross-process (say a.com to b.com). 3. Note that this has always been true in Chrome, so there's no behavior change. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. [Window 1] opens [Window 2]. 2. [Window 2] is navigated cross-process (say a.com to b.com). 3. Note that this has always been true in Chrome, so there's no behavior change. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! Normally, they should be able to script each other, but since the frames are in two different processes, we need to fail the security checks to prevent bad casts from happening. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! Normally, they should be able to script each other, but since the frames are in two different processes, we need to fail the security checks to prevent bad casts from happening. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! Normally, they should be able to script each other… but since the frames are in two different processes, we need to fail the security checks to avoid violating the assumptions that the frames will be local. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
dcheng@chromium.org changed reviewers: + creis@chromium.org, haraken@chromium.org, yukishiino@chromium.org
Description was changed from ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! Normally, they should be able to script each other… but since the frames are in two different processes, we need to fail the security checks to avoid violating the assumptions that the frames will be local. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access check will fail… but this ends up violating many assumptions in blink that passing the security check implies a local frame. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Here's one possible mitigation. One thing I'm worried about is perf: I believe we try to avoid virtual calls as much as possible, and this adds an additional one to this code path. Some possible alternatives: - Add a bit to SecurityOrigin to indicate if it originated from a remote frame, and update canAccess() to check that bit. - Always give remote frames a unique origin. Might have complications in terms of origin replication and persisting things like isSecureContext(). - One that didn't work was setting a special security token for remote frames.
Description was changed from ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access check will fail… but this ends up violating many assumptions in blink that passing the security check implies a local frame. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access check will pass… but this ends up violating many assumptions in blink that passing the security check implies a local frame. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
LGTM https://codereview.chromium.org/1887553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (left): https://codereview.chromium.org/1887553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:53: if (isOriginAccessibleFromDOMWindow(targetFrameOrigin, accessingWindow)) isOriginAccessibleFromDOMWindow has already been heavy, so I don't think this change affects performance.
I'm not sure if this is the right fix or not. If the both subframes are b.com, then, should they be considered as the same origin? Why should we treat them as cross origin? Reading the spec, it seems saying nothing about the structure of frames or parent-child relationship. https://html.spec.whatwg.org/multipage/browsers.html#concept-origin ---- For Document objects, If the Document's address's scheme is a network scheme A copy of the Document's address's origin. Two origins, A and B, are said to be same origin if the following algorithm returns true: 2. If A and B are both tuple origins and their schemes, hosts, and port are identical, then return true. ---- Then, both of b.com subframes are same origin, aren't they?
On 2016/04/13 at 05:48:53, yukishiino wrote: > I'm not sure if this is the right fix or not. If the both subframes are b.com, then, should they be considered as the same origin? Why should we treat them as cross origin? > > Reading the spec, it seems saying nothing about the structure of frames or parent-child relationship. > > https://html.spec.whatwg.org/multipage/browsers.html#concept-origin > ---- > For Document objects, > If the Document's address's scheme is a network scheme > A copy of the Document's address's origin. > > Two origins, A and B, are said to be same origin if the following algorithm returns true: > 2. If A and B are both tuple origins and their schemes, hosts, and port are identical, then return true. > ---- > > Then, both of b.com subframes are same origin, aren't they? They are same origin... but due to the underlying details of how we allocate frames to processes, it's quite easy to end up with cases where we have same origin frames in a "unit of related browsing contexts" that are hosted in different processes. If we allow these frames to pass the security checks for each other, then they should be able to synchronously script each other and access properties like window.document, but they cannot, because the other frame is cross-process. Thus, we must fail the security checks and treat them as if they were cross-process. For what it's worth, this isn't a new problem: it's just that this was not exposed before, due to the swappedout:// abstraction we used to use. Rather than using an actual RemoteFrame and mirroring the entire frame tree structure, we would navigate a LocalFrame to a special URL called swappedout://. Since the frame tree wasn't replicated, it wasn't possible for these subframes to reach each other previously. We removed the swappedout:// abstraction in M49, switching to RemoteFrame, which exposed this bug.
Also, it looks like this is failing an assert so I need to figure out what's going on. I'll upload a new patch in a bit.
Thanks for the quick fix! Some nits on the test. Also, it looks like we're still crashing? https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2622: // When two frames are same-origin but cross-process; they should behave as if nit: Comma rather than semicolon. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2635: EXPECT_TRUE(orig_site_instance.get() != NULL); nit: nullptr https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2653: // Clear the opener. This block is unnecessary, given how the test works. It only matters when doing a window.open() (to about:blank) and then doing a renderer-initiated cross-site navigation. Instead, this test is doing a browser-initiated cross-site navigation on line 2662, which will go cross process whether the opener is null or not. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2661: // Do a cross-site navigation that winds up same-site nit: End with period, and mention that it ends up in a different process. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2667: // Now navigate the new tab to a different site. Looks like a stale comment?
https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2622: // When two frames are same-origin but cross-process; they should behave as if On 2016/04/13 at 06:18:32, Charlie Reis wrote: > nit: Comma rather than semicolon. Done. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2635: EXPECT_TRUE(orig_site_instance.get() != NULL); On 2016/04/13 at 06:18:33, Charlie Reis wrote: > nit: nullptr Changed this EXPECT_NE as well. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2661: // Do a cross-site navigation that winds up same-site On 2016/04/13 at 06:18:33, Charlie Reis wrote: > nit: End with period, and mention that it ends up in a different process. Done. https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2667: // Now navigate the new tab to a different site. On 2016/04/13 at 06:18:33, Charlie Reis wrote: > Looks like a stale comment? Done.
https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2653: // Clear the opener. On 2016/04/13 at 06:18:33, Charlie Reis wrote: > This block is unnecessary, given how the test works. > > It only matters when doing a window.open() (to about:blank) and then doing a renderer-initiated cross-site navigation. Instead, this test is doing a browser-initiated cross-site navigation on line 2662, which will go cross process whether the opener is null or not. Done.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Doesn't this break the web if those are RemoteFrames? I'm pretty sure you can reach across the frame tree of the opened window to get to the other b.com subframe. Why are there RemoteFrame instances when OOPIF is disabled?
Thanks for the explanation and adding comments. Since this is a workaround to avoid crashes and not a true fix (ideally we should allow the same-origin inter-frame communication), I think it'd be better to put TODO comments. LGTM.
On 2016/04/13 at 06:45:55, esprehn wrote: > Doesn't this break the web if those are RemoteFrames? I'm pretty sure you can reach across the frame tree of the opened window to get to the other b.com subframe. > Yes. But it's also worth noting that this case has never worked in Chrome. > Why are there RemoteFrame instances when OOPIF is disabled? There have always been instances where we swap processes on navigation. Until we developed OOPIF, we used a different idiom to implement it completely in the embedder, so WebKit/Blink had no knowledge of it. It used a placeholder page called swappedout:// to represent the concept of a remote frame. Blink provided some basic hooks that the embedder could override to make things appear to work: for example, to intercept things like postMessage and deliver it to the actual process hosting the window. ┌───────────── PROCESS 1 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│ a.com ││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ and after a cross-process navigation, it'd look like this: ┌───────────── PROCESS 1 ─────────────┐ ┌───────────── PROCESS 2 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│swappedout://││ ││swappedout://│------>│ b.com ││ │└─────────────┘ opens └─────────────┘│ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ └─────────────────────────────────────┘ Of particular note here: there was no frame tree replication between [PROCESS 1] and [PROCESS 2], so a.com would never see any subframes in b.com, and similarly, b.com would never see any subframes in a.com. When we added OOPIF, we ended up having two ways of representing a window that is in another process: the legacy swappedout:// idiom and the new RemoteFrame idiom. This led to quite a bit of complexity in the embedder to support both idioms. In M49, we removed the old swappedout:// code path and replaced it with the new RemoteFrame code path, so it now the cross-process navigation case looks like this: ┌───────────── PROCESS 1 ─────────────┐ ┌───────────── PROCESS 2 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│ b.com remote││ ││ a.com remote│------>│ b.com ││ │└─────────────┘ opens └─────────────┘│ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ └─────────────────────────────────────┘ Since OOPIF replicates frame trees, this means that subframes in a.com and b.com are now visible to each other, even across the process separation.
On 2016/04/13 at 07:59:57, dcheng wrote: > On 2016/04/13 at 06:45:55, esprehn wrote: > > Doesn't this break the web if those are RemoteFrames? I'm pretty sure you can reach across the frame tree of the opened window to get to the other b.com subframe. > > > > Yes. But it's also worth noting that this case has never worked in Chrome. > > > Why are there RemoteFrame instances when OOPIF is disabled? > > There have always been instances where we swap processes on navigation. Until we developed OOPIF, we used a different idiom to implement it completely in the embedder, so WebKit/Blink had no knowledge of it. It used a placeholder page called swappedout:// to represent the concept of a remote frame. Blink provided some basic hooks that the embedder could override to make things appear to work: for example, to intercept things like postMessage and deliver it to the actual process hosting the window. > > ┌───────────── PROCESS 1 ─────────────┐ > │┌─────────────┐ ┌─────────────┐│ > ││ a.com │------>│ a.com ││ > │└─────────────┘ opens └─────────────┘│ > └─────────────────────────────────────┘ > > and after a cross-process navigation, it'd look like this: > > ┌───────────── PROCESS 1 ─────────────┐ ┌───────────── PROCESS 2 ─────────────┐ > │┌─────────────┐ ┌─────────────┐│ │┌─────────────┐ ┌─────────────┐│ > ││ a.com │------>│swappedout://││ ││swappedout://│------>│ b.com ││ > │└─────────────┘ opens └─────────────┘│ │└─────────────┘ opens └─────────────┘│ > └─────────────────────────────────────┘ └─────────────────────────────────────┘ > > Of particular note here: there was no frame tree replication between [PROCESS 1] and [PROCESS 2], so a.com would never see any subframes in b.com, and similarly, b.com would never see any subframes in a.com. > > When we added OOPIF, we ended up having two ways of representing a window that is in another process: the legacy swappedout:// idiom and the new RemoteFrame idiom. This led to quite a bit of complexity in the embedder to support both idioms. In M49, we removed the old swappedout:// code path and replaced it with the new RemoteFrame code path, so it now the cross-process navigation case looks like this: > > ┌───────────── PROCESS 1 ─────────────┐ ┌───────────── PROCESS 2 ─────────────┐ > │┌─────────────┐ ┌─────────────┐│ │┌─────────────┐ ┌─────────────┐│ > ││ a.com │------>│ b.com remote││ ││ a.com remote│------>│ b.com ││ > │└─────────────┘ opens └─────────────┘│ │└─────────────┘ opens └─────────────┘│ > └─────────────────────────────────────┘ └─────────────────────────────────────┘ > > Since OOPIF replicates frame trees, this means that subframes in a.com and b.com are now visible to each other, even across the process separation. Rewrapped to make it slightly more readable in the deprecated UI: ┌───────────── PROCESS 1 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│ a.com ││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ and after a cross-process navigation, it'd look like this with the old way, using swappedout:// ┌───────────── PROCESS 1 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│swappedout://││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ ┌───────────── PROCESS 2 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││swappedout://│------>│ a.com ││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ New way, with RemoteFrame: ┌───────────── PROCESS 1 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││ a.com │------>│b.com remote ││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘ ┌───────────── PROCESS 2 ─────────────┐ │┌─────────────┐ ┌─────────────┐│ ││a.com remote │------>│ a.com ││ │└─────────────┘ opens └─────────────┘│ └─────────────────────────────────────┘
On 2016/04/13 06:45:55, esprehn wrote: > Doesn't this break the web if those are RemoteFrames? I'm pretty sure you can > reach across the frame tree of the opened window to get to the other b.com > subframe. > > Why are there RemoteFrame instances when OOPIF is disabled? Daniel's answer is correct, but this also applies to main frames, as the test shows. Since Chrome launched, we've made browser-initiated cross-site navigations go cross-process, but renderer-initiated cross-site navigations usually don't. This was to preserve compatibility in cases that a cross-site iframe opens a popup (e.g., OAuth), etc. It's explained in the following doc and paper: https://www.chromium.org/developers/design-documents/process-models http://www.charlesreis.com/research/publications/eurosys-2009.pdf As a result, you could have two windows on site A, go cross-process in the second one to site B (by typing in the omnibox), and then click a link in both to site C. They'll be same-site but different processes. Yes, this breaks a possible script relationship, but it's unlikely to matter in practice relative to the OAuth case. (We decided it was a good tradeoff because the user initiated the navigation to B, basically leaving the previous context behind. You can almost view that as creating a new unit of related browsing contexts / BrowsingInstance. Maybe we can formalize that concept.) As Daniel mentions, this wasn't a problem to Blink before RemoteFrames, because the origin of the cross-process window looked like swappedout://. With RemoteFrames, we replicate the origin and it looks like it should pass the check. Full --site-per-process mode would eliminate this possibility, but most realistic modes (even those with OOPIFs) still have it. It's worth noting that TDI actually depends on this ability a bit more to avoid process inversions (where main frames would frequently end up in the subframe process). (Note: I'll post this explanation to the bug as well.) https://codereview.chromium.org/1887553002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2677: "accessing a cross-origin frame.")); Note: We won't get the security error in --site-per-process, right? In that case, the frame always ends up in the correct process. I would suggest skipping this check if AreAllSitesIsolatedForTesting() returns true (or maybe checking for the correct result). https://codereview.chromium.org/1887553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/1887553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: // processes. See https://crbug.com/601629 nit: End with period, here and below (as you did in DOMWindow).
> > Why are there RemoteFrame instances when OOPIF is disabled? > Daniel's answer is correct, but this also applies to main frames, as the test shows. FWIW, if you can think of a more concise way to explain this, I think it'd be nice to add to the CL description. I mainly focused on the subframe case, since that was easier to explain without writing a novel. =) https://codereview.chromium.org/1887553002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2677: "accessing a cross-origin frame.")); On 2016/04/13 at 16:45:56, Charlie Reis wrote: > Note: We won't get the security error in --site-per-process, right? In that case, the frame always ends up in the correct process. I would suggest skipping this check if AreAllSitesIsolatedForTesting() returns true (or maybe checking for the correct result). Done. https://codereview.chromium.org/1887553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/1887553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: // processes. See https://crbug.com/601629 On 2016/04/13 at 16:45:56, Charlie Reis wrote: > nit: End with period, here and below (as you did in DOMWindow). Done.
On 2016/04/13 17:13:01, dcheng wrote: > FWIW, if you can think of a more concise way to explain this, I think it'd be > nice to add to the CL description. I mainly focused on the subframe case, since > that was easier to explain without writing a novel. =) The subframe case is intuitive, but Elliot's right that we don't have RemoteFrames for subframes in default Chrome. I think the two b.com subframes just can't find each other in your example. (Confirmed by testing-- we don't crash there.) Might I suggest: 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 in both windows to c.com. Because browser-initiated navigations go cross-process but renderer-initiated navigations do not (see https://www.chromium.org/developers/design-documents/process-models#Caveats), the two c.com windows will end up in different processes. Both windows have the same origin but see each other as RemoteFrames. This means that the access check will pass… but this ends up violating many assumptions in blink that passing the security check implies a local frame. https://codereview.chromium.org/1887553002/diff/50004/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/50004/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2666: // SecurityError. nit: , unless we're in --site-per-process mode. (Or just move this comment into the else branch below.)
> The subframe case is intuitive, but Elliot's right that we don't have RemoteFrames for subframes in default Chrome. I think the two b.com subframes just can't find each other in your example. (Confirmed by testing-- we don't crash there.) The current explanation should still be accurate in default Chrome. We still mirror frame trees, and thus, RemoteFrames are still involved (for main frames and subframes). https://codereview.chromium.org/1887553002/diff/50004/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/50004/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2666: // SecurityError. On 2016/04/13 at 17:43:25, Charlie Reis wrote: > nit: , unless we're in --site-per-process mode. > > (Or just move this comment into the else branch below.) Done.
Description was changed from ========== 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 never that simple. There are several instances where this assumption fails to hold. 1. Navigate to a.com, which has a b.com subframe. 2. a.com opens a new window. 3. Navigate the new window to c.com, with a b.com subframe. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access check will pass… but this ends up violating many assumptions in blink that passing the security check implies a local frame. BUG=601629 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 never 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 the access check will pass… but this ends up violating many assumptions in blink that passing the security check implies a local frame. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access 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 ==========
Description was changed from ========== 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 never 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 the access check will pass… but this ends up violating many assumptions in blink that passing the security check implies a local frame. When OOPIF is disabled, both subframes are in the same process as their respective main frames. However, both subframes have the origin b.com! This means that the access 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 ========== to ========== 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 ==========
Updated the comment: in fact, we don't currently replicate the frame tree in default Chrome (and probably won't do so until we reduce memory usage for remote v8 contexts).
Thanks! content/ and CL description LGTM.
nick@chromium.org changed reviewers: + nick@chromium.org
lgtm https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2688: } The test looks good. There is some risk that as our oopif logic evolves, this test gets updated and stops exercising the interesting case, since the conditions that cause RFHM to produce this situation are a combination of bugs (as in the blob case), heuristics (as in tdi), and policies (no oopifs in default chrome). Hopefully we'll remember to avoid that, but I wonder: if it's possible to set up a blink unittest test that creates a local and a remote frame with the same origin, might that be worthwhile too, since it would be less brittle?
lgtm
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/1887553002/#ps60001 (title: ".")
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
https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1887553002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2688: } On 2016/04/13 at 18:29:17, ncarter wrote: > The test looks good. > > There is some risk that as our oopif logic evolves, this test gets updated and stops exercising the interesting case, since the conditions that cause RFHM to produce this situation are a combination of bugs (as in the blob case), heuristics (as in tdi), and policies (no oopifs in default chrome). > > Hopefully we'll remember to avoid that, but I wonder: if it's possible to set up a blink unittest test that creates a local and a remote frame with the same origin, might that be worthwhile too, since it would be less brittle? Oops, forgot to reply to this comment. We could, but since this behavior is ultimately controlled wholly by the embedder, I'd prefer to keep the test there, so we don't end up with unit tests in Blink that exercise states that the embedder will never actually reach.
Message was sent while issue was closed.
Committed patchset #5 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f23b8e77a83a5aafabf64acf723cf2ac02c5cf0e Cr-Commit-Position: refs/heads/master@{#387087} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
