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 (694a9b5). rdar://144667626
Canonical link: https://commits.webkit.org/290260@main

Author: the-chenergy

Source/WebKit/UIProcess/API/C/WKPage.cpp

@@ -2952,6 +2952,13 @@ void WKPageSetAllowsRemoteInspection(WKPageRef pageRef, bool allow)
 #endif
 }
 
+void WKPageShowWebInspectorForTesting(WKPageRef pageRef)
+{
+    RefPtr<WebInspectorUIProxy> inspector = toImpl(pageRef)->inspector();
+    inspector->markAsUnderTest();
+    inspector->show();
+}
+
 void WKPageSetMediaVolume(WKPageRef pageRef, float volume)
 {
     CRASH_IF_SUSPENDED;

Source/WebKit/UIProcess/API/C/WKPagePrivate.h

@@ -94,6 +94,8 @@ WK_EXPORT void WKPageSetControlledByAutomation(WKPageRef page, bool controlled);
 WK_EXPORT bool WKPageGetAllowsRemoteInspection(WKPageRef page);
 WK_EXPORT void WKPageSetAllowsRemoteInspection(WKPageRef page, bool allow);
 
+WK_EXPORT void WKPageShowWebInspectorForTesting(WKPageRef page);
+
 WK_EXPORT void WKPageSetMediaVolume(WKPageRef page, float volume);
 WK_EXPORT void WKPageSetMayStartMediaWhenInWindow(WKPageRef page, bool mayStartMedia);
 

Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp

@@ -156,7 +156,12 @@ void WebInspectorUIProxy::connect()
 
     Ref legacyMainFrameProcess = inspectedPage->legacyMainFrameProcess();
     legacyMainFrameProcess->send(Messages::WebInspectorInterruptDispatcher::NotifyNeedDebuggerBreak(), 0);
-    legacyMainFrameProcess->send(Messages::WebInspector::Show(), m_inspectedPage->webPageIDInMainFrameProcess());
+    legacyMainFrameProcess->sendWithAsyncReply(
+        Messages::WebInspector::Show(),
+        [this, protectedThis = Ref { *this }] {
+            openLocalInspectorFrontend();
+        },
+        m_inspectedPage->webPageIDInMainFrameProcess());
 }
 
 void WebInspectorUIProxy::show()
@@ -265,7 +270,7 @@ void WebInspectorUIProxy::showConsole()
     if (!inspectedPage)
         return;
 
-    createFrontendPage();
+    show();
 
     inspectedPage->protectedLegacyMainFrameProcess()->send(Messages::WebInspector::ShowConsole(), inspectedPage->webPageIDInMainFrameProcess());
 }
@@ -276,7 +281,7 @@ void WebInspectorUIProxy::showResources()
     if (!inspectedPage)
         return;
 
-    createFrontendPage();
+    show();
 
     inspectedPage->protectedLegacyMainFrameProcess()->send(Messages::WebInspector::ShowResources(), inspectedPage->webPageIDInMainFrameProcess());
 }
@@ -287,7 +292,7 @@ void WebInspectorUIProxy::showMainResourceForFrame(WebCore::FrameIdentifier fram
     if (!inspectedPage)
         return;
 
-    createFrontendPage();
+    show();
 
     inspectedPage->sendToProcessContainingFrame(frameID, Messages::WebInspector::ShowMainResourceForFrame(frameID));
 }
@@ -392,6 +397,8 @@ void WebInspectorUIProxy::togglePageProfiling()
     if (!m_inspectedPage)
         return;
 
+    show();
+
     RefPtr inspectedPage = m_inspectedPage.get();
     if (m_isProfilingPage)
         inspectedPage->protectedLegacyMainFrameProcess()->send(Messages::WebInspector::StopPageProfiling(), inspectedPage->webPageIDInMainFrameProcess());
@@ -453,7 +460,16 @@ void WebInspectorUIProxy::createFrontendPage()
 #endif
 }
 
-void WebInspectorUIProxy::openLocalInspectorFrontend(bool canAttach, bool underTest)
+void WebInspectorUIProxy::requestOpenLocalInspectorFrontend()
+{
+    // Prevent a compromised malicious web page from opening Web Inspector at will.
+    if (!m_underTest && inspectionLevel() < 2)
+        return;
+
+    openLocalInspectorFrontend();
+}
+
+void WebInspectorUIProxy::openLocalInspectorFrontend()
 {
     RefPtr inspectedPage = m_inspectedPage.get();
     if (!inspectedPage)
@@ -467,7 +483,6 @@ void WebInspectorUIProxy::openLocalInspectorFrontend(bool canAttach, bool underT
         return;
     }
 
-    m_underTest = underTest;
     createFrontendPage();
 
     RefPtr inspectorPage = m_inspectorPage.get();
@@ -482,7 +497,10 @@ void WebInspectorUIProxy::openLocalInspectorFrontend(bool canAttach, bool underT
     inspectedPage->inspectorController().connectFrontend(*this);
 
     if (!m_underTest) {
-        m_canAttach = platformCanAttach(canAttach);
+        // FIXME <https://webkit.org/b/283435>: Remove the webProcessCanAttach argument from platformCanAttach.
+        // The value canAttach in the web process is no longer used or respected.
+        const bool webProcessCanAttach = false;
+        m_canAttach = platformCanAttach(webProcessCanAttach);
         m_isAttached = shouldOpenAttached();
         m_attachmentSide = static_cast<AttachmentSide>(protectedInspectorPagePreferences()->inspectorAttachmentSide());
 
@@ -607,7 +625,6 @@ void WebInspectorUIProxy::closeFrontendPageAndWindow()
 
     m_isAttached = false;
     m_canAttach = false;
-    m_underTest = false;
 
     platformCloseFrontendPageAndWindow();
 }

Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h

@@ -199,6 +199,7 @@ class WebInspectorUIProxy
     void toggleElementSelection();
 
     bool isUnderTest() const { return m_underTest; }
+    void markAsUnderTest() { m_underTest = true; }
 
     void setDiagnosticLoggingAvailable(bool);
 
@@ -264,9 +265,10 @@ class WebInspectorUIProxy
 #endif
 
     // Called by WebInspectorUIProxy messages
-    void openLocalInspectorFrontend(bool canAttach, bool underTest);
+    void requestOpenLocalInspectorFrontend();
     void setFrontendConnection(IPC::Connection::Handle&&);
 
+    void openLocalInspectorFrontend();
     void sendMessageToBackend(const String&);
     void frontendLoaded();
     void didClose();

Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.messages.in

@@ -26,7 +26,7 @@
     DispatchedTo=UI
 ]
 messages -> WebInspectorUIProxy {
-    OpenLocalInspectorFrontend(bool canAttach, bool underTest)
+    RequestOpenLocalInspectorFrontend()
     SetFrontendConnection(IPC::ConnectionHandle connectionHandle)
 
     SendMessageToBackend(String message)

Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -10056,6 +10056,12 @@ void WebPageProxy::contextMenuItemSelected(const WebContextMenuItemData& item)
     case ContextMenuItemTagShowColors:
         showColorPanel();
         return;
+
+    case ContextMenuItemTagInspectElement:
+        // The web process can no longer demand Web Inspector to show, so handle that part here.
+        m_inspector->show();
+        // The actual element-selection is still handled in the web process, so we break instead of return.
+        break;
 #endif // PLATFORM(MAC)
 
     case ContextMenuItemTagShowSpellingPanel:

Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp

@@ -438,11 +438,6 @@ void WKBundlePageListenForLayoutMilestones(WKBundlePageRef pageRef, WKLayoutMile
     WebKit::toImpl(pageRef)->listenForLayoutMilestones(WebKit::toLayoutMilestones(milestones));
 }
 
-void WKBundlePageShowInspectorForTest(WKBundlePageRef page)
-{
-    WebKit::toImpl(page)->inspector()->show();
-}
-
 void WKBundlePageCloseInspectorForTest(WKBundlePageRef page)
 {
     WebKit::toImpl(page)->inspector()->close();

Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h

@@ -99,7 +99,6 @@ WK_EXPORT double WKBundlePageGetBackingScaleFactor(WKBundlePageRef page);
 
 WK_EXPORT void WKBundlePageListenForLayoutMilestones(WKBundlePageRef page, WKLayoutMilestones milestones);
 
-WK_EXPORT void WKBundlePageShowInspectorForTest(WKBundlePageRef page);
 WK_EXPORT void WKBundlePageCloseInspectorForTest(WKBundlePageRef page);
 WK_EXPORT void WKBundlePageEvaluateScriptInInspectorForTest(WKBundlePageRef page, WKStringRef script);
 

Source/WebKit/WebProcess/Inspector/WebInspector.messages.in

@@ -25,7 +25,7 @@
     DispatchedTo=WebContent
 ]
 messages -> WebInspector {
-    Show()
+    Show() -> () Async
     Close()
 
     SetAttached(bool attached)

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp

@@ -102,7 +102,7 @@ void WebInspectorClient::frontendCountChanged(unsigned count)
 Inspector::FrontendChannel* WebInspectorClient::openLocalFrontend(InspectorController* controller)
 {
     if (RefPtr page = m_page.get())
-        page->inspector()->openLocalInspectorFrontend(controller->isUnderTest());
+        page->inspector()->openLocalInspectorFrontend();
     return nullptr;
 }
 

Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp

@@ -75,9 +75,9 @@ WebPage* WebInspector::page() const
     return m_page.get();
 }
 
-void WebInspector::openLocalInspectorFrontend(bool underTest)
+void WebInspector::openLocalInspectorFrontend()
 {
-    WebProcess::singleton().parentProcessConnection()->send(Messages::WebInspectorUIProxy::OpenLocalInspectorFrontend(canAttachWindow(), underTest), m_page->identifier());
+    WebProcess::singleton().parentProcessConnection()->send(Messages::WebInspectorUIProxy::RequestOpenLocalInspectorFrontend(), m_page->identifier());
 }
 
 void WebInspector::setFrontendConnection(IPC::Connection::Handle&& connectionHandle)
@@ -132,12 +132,13 @@ void WebInspector::whenFrontendConnectionEstablished(Function<void()>&& callback
 }
 
 // Called by WebInspector messages
-void WebInspector::show()
+void WebInspector::show(CompletionHandler<void()>&& completionHandler)
 {
     if (!m_page->corePage())
         return;
 
     m_page->corePage()->inspectorController().show();
+    completionHandler();
 }
 
 void WebInspector::close()
@@ -165,8 +166,6 @@ void WebInspector::showConsole()
     if (!m_page->corePage())
         return;
 
-    m_page->corePage()->inspectorController().show();
-
     whenFrontendConnectionEstablished([=, this] {
         m_frontendConnection->send(Messages::WebInspectorUI::ShowConsole(), 0);
     });
@@ -177,8 +176,6 @@ void WebInspector::showResources()
     if (!m_page->corePage())
         return;
 
-    m_page->corePage()->inspectorController().show();
-
     whenFrontendConnectionEstablished([=, this] {
         m_frontendConnection->send(Messages::WebInspectorUI::ShowResources(), 0);
     });
@@ -193,8 +190,6 @@ void WebInspector::showMainResourceForFrame(WebCore::FrameIdentifier frameIdenti
     if (!m_page->corePage())
         return;
 
-    m_page->corePage()->inspectorController().show();
-
     String inspectorFrameIdentifier = m_page->corePage()->inspectorController().ensurePageAgent().frameId(frame->coreLocalFrame());
 
     whenFrontendConnectionEstablished([=, this] {
@@ -207,8 +202,6 @@ void WebInspector::startPageProfiling()
     if (!m_page->corePage())
         return;
 
-    m_page->corePage()->inspectorController().show();
-
     whenFrontendConnectionEstablished([=, this] {
         m_frontendConnection->send(Messages::WebInspectorUI::StartPageProfiling(), 0);
     });
@@ -219,8 +212,6 @@ void WebInspector::stopPageProfiling()
     if (!m_page->corePage())
         return;
 
-    m_page->corePage()->inspectorController().show();
-
     whenFrontendConnectionEstablished([=, this] {
         m_frontendConnection->send(Messages::WebInspectorUI::StopPageProfiling(), 0);
     });
@@ -270,6 +261,8 @@ void WebInspector::setEmulatedConditions(std::optional<int64_t>&& bytesPerSecond
 
 #endif // ENABLE(INSPECTOR_NETWORK_THROTTLING)
 
+// FIXME <https://webkit.org/b/283435>: Remove this unused canAttachWindow function. Its return value is no longer used
+// or respected by the UI process.
 bool WebInspector::canAttachWindow()
 {
     if (!m_page->corePage())

Source/WebKit/WebProcess/Inspector/WebInspectorInternal.h

@@ -58,7 +58,7 @@ class WebInspector : public ThreadSafeRefCounted<WebInspector>, private IPC::Con
     void didClose(IPC::Connection&) override { close(); }
     void didReceiveInvalidMessage(IPC::Connection&, IPC::MessageName, int32_t indexOfObjectFailingDecoding) override { close(); }
 
-    void show();
+    void show(CompletionHandler<void()>&&);
     void close();
 
     void canAttachWindow(bool& result);
@@ -97,7 +97,7 @@ class WebInspector : public ThreadSafeRefCounted<WebInspector>, private IPC::Con
     bool canAttachWindow();
 
     // Called from WebInspectorClient
-    void openLocalInspectorFrontend(bool underTest);
+    void openLocalInspectorFrontend();
     void closeFrontendConnection();
 
     void bringToFront();

Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp

@@ -491,7 +491,7 @@ void TestRunner::makeWindowObject(JSContextRef context)
 
 void TestRunner::showWebInspector()
 {
-    WKBundlePageShowInspectorForTest(page());
+    postMessage("ShowWebInspector");
 }
 
 void TestRunner::closeWebInspector()

Tools/WebKitTestRunner/TestInvocation.cpp

@@ -714,6 +714,10 @@ void TestInvocation::didReceiveMessageFromInjectedBundle(WKStringRef messageName
 
     if (WKStringIsEqualToUTF8CString(messageName, "RequestExitFullscreenFromUIProcess")) {
         TestController::singleton().requestExitFullscreenFromUIProcess(TestController::singleton().mainWebView()->page());
+    }
+
+    if (WKStringIsEqualToUTF8CString(messageName, "ShowWebInspector")) {
+        WKPageShowWebInspectorForTesting(TestController::singleton().mainWebView()->page());
         return;
     }