Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 301bff4919ab00d3a83c84d908d356add32807a9
https://github.com/WebKit/WebKit/commit/301bff4919ab00d3a83c84d908d356add32807a9
Author: Qianlang Chen <[email protected]>
Date: 2025-02-11 (Tue, 11 Feb 2025)
Changed paths:
M Source/WebKit/UIProcess/API/C/WKPage.cpp
M Source/WebKit/UIProcess/API/C/WKPagePrivate.h
M Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp
M Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h
M Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.messages.in
M Source/WebKit/UIProcess/WebPageProxy.cpp
M Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
M Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h
M Source/WebKit/WebProcess/Inspector/WebInspector.messages.in
M Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp
M Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp
M Source/WebKit/WebProcess/Inspector/WebInspectorInternal.h
M Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
M Tools/WebKitTestRunner/TestInvocation.cpp
Log Message:
-----------
A compromised Web Content process should not be able to start Web Inspector
rdar://98891055
https://bugs.webkit.org/show_bug.cgi?id=283092
Reviewed by Ryosuke Niwa and BJ Burg.
There currently exists a message
WebInspectorUIProxy::OpenLocalInspectorFrontend, which the web process
sends to the UI process to show Web Inspector for the current web page.
This introduces security risks as a compromised website may find its way
to send arbitrary messages to the UI process, opening Web Inspector and
weakening the web content sandbox.
The reason this message exists is because there are useful ways the web
process needs to open Web Inspector with initiative. Normally, Web
Inspector is opened via one of the Develop menu's items, which is
controlled by the UI process. However, Web Inspector can also be opened
without being prompted by the UI process first, in these places:
1. In a web page's context menu, the "Inspect Element" option
2. Inside Web Inspector, if the Debug UI is enabled, on the top right
corner, a button to open inspector^2
3. In WebKitTestRunner, via the TestRunner::showWebInspector function
This patch makes it so that web process can no longer send a message to
a UI process to open Web Inspector. This means web process cannot open
Web Inspector at will -- it must be either due to the UI process's
demand, or it's in one of the above three cases. More details below.
I have tested that this change preserves the above three special cases
and does prevent the web page from opening Web Inspector at will.
- Cases #1 and #2 can be tested from the UI.
- Case #3 can be tested with a WebKit test involving Web Inspector.
I ran the test LayoutTests/inspector/console/js-completions.html,
where I saw the test crashing without special treatment for this
case.
- To verify that the web page can't open Web Inspector, I followed
the reproduction steps from the Radar and saw Web Inspector no
longer opens, and opening the external URL also failed as expected.
* Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.messages.in:
* Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h:
* Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:
(WebKit::WebInspectorUIProxy::connect):
- If the UI process wants to open Web Inspector, it sends a
WebInspector::Show command to the web process. This patch makes
that command take an async reply, so that the anticipated
WebInspectorUIProxy::OpenLocalInspectorFrontend message from the
web process can now be delivered through that async reply instead.
This ensures that OpenLocalInspectorFrontend can only be done
when initiated from the UI process (due to user interaction).
(WebKit::WebInspectorUIProxy::markAsUnderTest):
(WebKit::WebInspectorUIProxy::openLocalInspectorFrontend):
(WebKit::WebInspectorUIProxy::closeFrontendPageAndWindow):
- To avoid relying on the web process for potentially sensitive
parameters, I reworked and removed the canAttach and underTest
arguments from openLocalInspectorFrontend. These two values
are now stored and managed in the UI process instead, instead of
being passed from the web process all the time.
- For canAttach, I noticed that the
WebInspectorUIProxyMac::platformCanAttach method already
implements the same logic as the web process's
WebInspector::canAttachWindow. I filed https://webkit.org/b/283435
as a follow-up to clean up the webProcessCanAttach parameter,
the canAttachWindow function in the web process, and potentially
the m_attached field too, which all become obsolete due to
this change.
- I couldn't figure out what the `if (m_attached)` in
canAttachWindow check does, and to me it had no effect, as
this function is not called while inspector is open.
- For underTest, I'm now letting the test runner directly set
the flag on the WebInspectorUIProxy, as part of my fix to
address case #3 from above.
(WebKit::WebInspectorUIProxy::showConsole):
(WebKit::WebInspectorUIProxy::showResources):
(WebKit::WebInspectorUIProxy::showMainResourceForFrame):
(WebKit::WebInspectorUIProxy::togglePageProfiling):
- As the web process can longer call OpenLocalInspectorFrontend,
call show/connect/openLocalInspectorFrontend here in the UI process
instead.
(WebKit::WebInspectorUIProxy::requestOpenLocalInspectorFrontend):
- To preserve the open inspector^2 button (case #2 from above), we
still maintain this message, but we ignore it unless it's for
opening inspector^2, thus renaming the message as a request.
This is all assuming that the Web Inspector is not a compromised
web process, so we allow that message from it to come through.
* Source/WebKit/WebProcess/Inspector/WebInspector.messages.in:
* Source/WebKit/WebProcess/Inspector/WebInspector.h:
* Source/WebKit/WebProcess/Inspector/WebInspector.cpp:
(WebKit::WebInspector::show):
- The Show message now takes an async reply, which is used to replace
sending WebInspectorUIProxy::OpenLocalInspectorFrontend later.
(WebKit::WebInspector::showConsole):
(WebKit::WebInspector::showResources):
(WebKit::WebInspector::showMainResourceForFrame):
(WebKit::WebInspector::startPageProfiling):
(WebKit::WebInspector::stopPageProfiling):
- Calling inspectorController()->show() no longer does anything,
since it's now the UI process's job to show Web Inspector first,
for these functions to merely switch to the appropriate tabs.
* Source/WebKit/WebProcess/Inspector/WebInspector.cpp:
(WebKit::WebInspector::openLocalInspectorFrontend):
* Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:
(WebKit::WebInspectorClient::openLocalFrontend):
- Adapt to the command's reworked version.
- This is maintained to allow the opening of inspector^2 from the web
process (case #2 from above). For opening inspector^1, this message
will be ignored by the UI process.
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::contextMenuItemSelected):
- When the "Inspect Element" context menu item is selected (case #1
from above), since the web process may not be privileged to open
Web Inspector, handle the showing of inspector here in UI process.
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::showWebInspector):
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle):
* Source/WebKit/UIProcess/API/C/WKPagePrivate.h:
* Source/WebKit/UIProcess/API/C/WKPage.cpp:
(WKPageShowWebInspectorForTesting):
- Preserve letting the WebKitTestRunner open Web Inspector (case #3
from above).
- Adapt to the change that we now also let the UI process know about
the underTest flag for case #3, rather than letting UI process
rely on the value reported by the web process.
* Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:
* Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:
(WKBundlePageShowInspectorForTest): Deleted.
- No longer used due to my special fix for case #3.
Originally-landed-as: 283286.537@safari-7620-branch (694a9b5a1b35).
rdar://144667626
Canonical link: https://commits.webkit.org/290260@main
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
