[Site Isolation] Cross site iframe fails to window.open the main frame's site#57760
Conversation
RupinMittal
added
the
New Bugs
|
EWS run on previous version of this PR (hash b1c4ce5) Details
|
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
b1c4ce5 to
f34388e
Compare
|
EWS run on previous version of this PR (hash f34388e) Details
|
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
f34388e to
3df8bb4
Compare
|
EWS run on previous version of this PR (hash 3df8bb4) Details |
Safer C++ Build #77899 (3df8bb4)❌ Found 1 failing file with 6 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
3df8bb4 to
b74bb20
Compare
|
EWS run on previous version of this PR (hash b74bb20) Details
|
webkit-ews-buildbot
added
the
merging-blocked
RupinMittal
removed
the
merging-blocked
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
b74bb20 to
7fadcd7
Compare
|
EWS run on previous version of this PR (hash 7fadcd7) Details
|
RupinMittal
requested review from
achristensen07,
charliewolfe,
rniwa and
szewai
|
|
||
| BrowsingContextGroup* browsingContextGroup() const; | ||
|
|
||
| void addFrame(WebFrameProxy& frame) { m_frames.add(frame); } |
There was a problem hiding this comment.
Maybe the parameter can be const? const WebFrameProxy& frame ?
| BrowsingContextGroup* browsingContextGroup() const; | ||
|
|
||
| void addFrame(WebFrameProxy& frame) { m_frames.add(frame); } | ||
| void removeFrame(WebFrameProxy& frame) { m_frames.remove(frame); } |
There was a problem hiding this comment.
Nit: You could also possibly inline addFrame and removeFrame but totally optional.
There was a problem hiding this comment.
Changed these functions to deal with ints, so this isn't really a concern.
| } | ||
|
|
||
| BrowsingContextGroup* FrameProcess::browsingContextGroup() const | ||
| { |
There was a problem hiding this comment.
nit: is it possible to put this in the header file, too?
There was a problem hiding this comment.
To do this we'd have to include BrowsingContextGroup.h in FrameProcess.h. I believe we don't want to increase the number of header includes there.
|
|
||
| // Transit page in old frame process to remote because pages in that process still need access to this page. | ||
| if (frameProecessChanged && pageMainFrame == m_mainFrame && m_browsingContextGroup->isFrameProcessInUseForMainFrame(pageMainFrameProces.get())) { | ||
| if (frameProcessChanged && pageMainFrame == m_mainFrame && pageMainFrameProcess->frameCount() && pageMainFrameProcess->browsingContextGroup() == m_browsingContextGroup.ptr()) { |
There was a problem hiding this comment.
Nit: It might be helpful to update the explanation above for why there are these additional conditions being checked!
| protect(m_frameProcess)->removeFrame(*this); | ||
| m_frameProcess = process.releaseNonNull(); | ||
| protect(m_frameProcess)->addFrame(*this); |
There was a problem hiding this comment.
This would be simplified if it just called setProcess(*process) to reduce the duplicate management code.
|
|
||
| #pragma once | ||
|
|
||
| #include "WebFrameProxy.h" |
There was a problem hiding this comment.
Let's move functions to FrameProcess.cpp to remove the need to include this header here.
| pageMainFrame->setProcess(m_frameProcess); | ||
|
|
||
| // Transit page in old frame process to remote because pages in that process still need access to this page. | ||
| if (frameProecessChanged && pageMainFrame == m_mainFrame && m_browsingContextGroup->isFrameProcessInUseForMainFrame(pageMainFrameProces.get())) { |
There was a problem hiding this comment.
If we make this change, we should remove isFrameProcessInUseForMainFrame.
| const std::optional<WebCore::Site> m_site; | ||
| const WebCore::Site m_mainFrameSite; | ||
| bool m_isArchiveProcess; | ||
| WeakHashSet<WebFrameProxy> m_frames; |
There was a problem hiding this comment.
A WeakHashSet seems a bit heavy-handed when all we really need is a frame count. This could be an integer. It also seems related to refCount(), and I'm not sure I completely understand the difference between the two. If we do need to have another integer, let's assert that it's zero in the destructor to make sure we do proper accounting.
There was a problem hiding this comment.
If I understand correctly, you're talking about this comment in FrameProcess.h:
// Note: This object should only be referenced by WebFrameProxy because its destructor is an
// important part of managing the lifetime of a frame and the process used by the frame.
I see what you mean. The refCount() of FrameProcess already represents the number of frames local to the associated WebProcess. It's true that we could use this instead of a second integer (I would replace the WeakHashSet with an int), but I worry we might shoot ourselves in the foot.
There's no guarantee that something besides the WebFrameProxy doesn't Ref the FrameProcess. For example, the existing code already has such a case:
I don't have a strong opinion either way. Let me know what you think.
There was a problem hiding this comment.
I've changed the code to use an int instead of WeakHashSet for now. Let me know what you think.
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
7fadcd7 to
8d2c17c
Compare
|
EWS run on previous version of this PR (hash 8d2c17c) Details
|
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
8d2c17c to
b80b658
Compare
|
EWS run on previous version of this PR (hash b80b658) Details |
webkit-ews-buildbot
added
the
merging-blocked
| const std::optional<WebCore::Site> m_site; | ||
| const WebCore::Site m_mainFrameSite; | ||
| bool m_isArchiveProcess; | ||
| int m_frameCount; |
There was a problem hiding this comment.
We never expect to have negative frame count. This should be unsigned.
This also needs to be initialized. Right now we're relying on uninitialized memory.
| namespace WebKit { | ||
|
|
||
| class BrowsingContextGroup; | ||
| class WebFrameProxy; |
There was a problem hiding this comment.
This is no longer needed.
| // If the originating FrameProcess still has local frames and is still in the same | ||
| // BrowsingContext group, pages in that process still need access to this page. | ||
| // So transition the WebPageProxy in that process to a RemotePageProxy. | ||
| if (frameProcessChanged && pageMainFrame == m_mainFrame && pageMainFrameProcess->refCount() && pageMainFrameProcess->browsingContextGroup() == m_browsingContextGroup.ptr()) { |
There was a problem hiding this comment.
This currently calls refCount instead of frameCount. refCount should always be nonzero if it is not UAFing.
RupinMittal
removed
the
merging-blocked
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
b80b658 to
aa37486
Compare
|
EWS run on previous version of this PR (hash aa37486) Details
|
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
aa37486 to
165c98b
Compare
|
EWS run on previous version of this PR (hash 165c98b) Details
|
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
165c98b to
e1e88bd
Compare
|
EWS run on previous version of this PR (hash e1e88bd) Details |
| void incrementFrameCount() { m_frameCount++; } | ||
| void decrementFrameCount() { m_frameCount--; } |
There was a problem hiding this comment.
I think we need assertion in these functions to avoid overflow.
If someone sets WebFrameProcess::m_frameProcess without invoking setProcess this could go wrong.
There was a problem hiding this comment.
Good point. I've updated m_frameCount to be of type Checked<unsigned>, so we automatically have assertions when incrementing/decrementing.
RupinMittal
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
e1e88bd to
f4023b0
Compare
|
EWS run on current version of this PR (hash f4023b0) Details |
RupinMittal
added
the
unsafe-merge-queue
…e's site https://bugs.webkit.org/show_bug.cgi?id=306842 rdar://169509909 Reviewed by Alex Christensen and Sihui Liu. Imagine we have a main frame containing site1.com and an iframe containing site2.com. Then, site2.com calls window.open("site.com"). The end result must be: WebProcess1 | WebProcess2 ------------------------------------|------------------------------------------ Site1WindowA Site1WindowB | Site1WindowA Site1WindowB Site2FrameA | Site2FrameA | WebPage1 WebPage2 | WebPage3 WebPage4 (where Site1 is local to WebProcess1). Both WebPage1 and WebPage2 should each have an associated WebPageProxy and both WebPage3 and WebPage4 should each have an associated RemotePageProxy. But currently, after the call to window.open, WebPage4 does not have an associated RemotePageProxy. When the window.open call happens, the steps we follow should be: 1. Create WebPage2 and WebPage4. 2. Create a WebPageProxy associated with WebPage4 (because the window.open call occurred in WebProcess2). 3. Create a RemotePageProxy associated with WebPage2. 4. When the load starts, we realize the load should occur in WebProcess 1, so we create a ProvisionalPageProxy and give it the RemotePageProxy's message registration. So now the ProvisionalPageProxy is associated with WebPage2. The RemotePageProxy is then destroyed. (See ProvisionalPageProxy::initializeWebPage). 5. When the load commits, create a new RemotePageProxy and give it the WebPageProxy's messager registration. So now a RemotePageProxy is associated with WebPage4. (See ProvisionalPageProxy::didCommitLoadForFrame and BrowsingContextGroup::transitionPageToRemotePage). 6. Give the WebPageProxy the message registration of the ProvisionalPageProxy. So now the WebPageProxy is associated with WebPage2. (See WebPageProxy::swapToProvisionalPage). The problem is that step 5 doesn't happen and so WebPage4 gets left without a RemotePageProxy. In ProvisionalPageProxy::didCommitLoadForFrame, we only setup the new RemotePageProxy (by calling BrowsingContextGroup::transitionPageToRemotePage) if "m_browsingContextGroup->isFrameProcessInUseForMainFrame(pageMainFrameProcess.get())" is true. At this point the WebPageProxy is still associated with WebPage4 and so with WebProcess2. WebProcess2 is not in use for the main frame. That's WebProcess1. So this condition is false. The code assumes that this WebProcess2 won't be used after this load commits, so there is no point in setting up the RemotePageProxy. This condition is wrong. It doesn't account for the fact that after the load commits, there will be 1 frame that is local to WebProcess2 (Site2FrameA). So WebProcess2 will still exist and so will WebPage4, and it will need a RemotePageProxy. To fix this, we make FrameProcess track the number of frames local to its associated WebProcess. We amend the condition so that if there are any such frames at the time of the load committing, we must set up the RemotePageProxy. This is tested by a new API test SiteIsolation.CrossSiteIFrameWindowOpensMainFrameSite. This changed caused the test SiteIsolation.BrowsingContextGroupSwitchForIncompatibleCrossOriginOpenerPolicy to fail. In that test, when the page with unsafe-none opens a page with same-origin-allow-popups, we do not want the opener relationship to be preserved, and so the the two WebProcesses are put in different BrowsingContentGroups. So we do not need the RemotePageProxy. So we add an extra check. If the load causes the WebProcess to put into a different BrowsingContextGroup, we do not setup the RemotePageProxy. * Source/WebKit/UIProcess/BrowsingContextGroup.cpp: (WebKit::BrowsingContextGroup::isFrameProcessInUseForMainFrame): Deleted. * Source/WebKit/UIProcess/BrowsingContextGroup.h: * Source/WebKit/UIProcess/FrameProcess.cpp: (WebKit::FrameProcess::~FrameProcess): (WebKit::FrameProcess::browsingContextGroup const): * Source/WebKit/UIProcess/FrameProcess.h: (WebKit::FrameProcess::incrementFrameCount): (WebKit::FrameProcess::decrementFrameCount): (WebKit::FrameProcess::frameCount const): * Source/WebKit/UIProcess/ProvisionalPageProxy.cpp: (WebKit::ProvisionalPageProxy::didCommitLoadForFrame): If the Frame Process's associated WebProcess has any frames that are local to it, and is still in the same BrowsingContextGroup, we must setup the RemotePageProxy. * Source/WebKit/UIProcess/WebFrameProxy.cpp: (WebKit::WebFrameProxy::WebFrameProxy): (WebKit::WebFrameProxy::~WebFrameProxy): (WebKit::WebFrameProxy::commitProvisionalFrame): (WebKit::WebFrameProxy::setProcess): * Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm: (TestWebKitAPI::TEST(SiteIsolation, CrossSiteIFrameWindowOpensMainFrameSite)): Canonical link: https://commits.webkit.org/306784@main
webkit-commit-queue
force-pushed
the
eng/CrossSiteIFrameWindowOpenMainFrameSiteFails
branch
from
f4023b0 to
e4e510e
Compare
|
Committed 306784@main (e4e510e): https://commits.webkit.org/306784@main Reviewed commits have been landed. Closing PR #57760 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
7 participants
e4e510e
f4023b0