[WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 04:05:07 +0000 (04:05 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 04:05:07 +0000 (04:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203830
<rdar://problem/56797134>

Reviewed by Brent Fulgham .

Source/WebKit:

-[_WKWebAuthenticationPanel cancel] was only expected to be called on behalf of an
explicit user cancel from the UI. However, clients may call it in different other
unexpected scenarios as well. We should guard against that.

To do so, two counter ways are implemented:
1) AuthenticatorManager::cancelRequest is changed to invoke the pending request if there
is no GlobalFrameID. This case can only be reached via calling -[_WKWebAuthenticationPanel cancel]
before AuthenticatorManager::cancelRequest.
2) WebAuthenticationPanelClient::updatePanel and WebAuthenticationPanelClient::dismissPanel
will call delegate methods in the next run loop to prevent -[_WKWebAuthenticationPanel cancel]
being called in the delegates.

Covered by new API tests.

* UIProcess/WebAuthentication/AuthenticatorManager.cpp:
(WebKit::AuthenticatorManager::cancelRequest):
(WebKit::AuthenticatorManager::createService const):
(WebKit::AuthenticatorManager::invokePendingCompletionHandler):
* UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:
(WebKit::WebAuthenticationPanelClient::updatePanel const):
(WebKit::WebAuthenticationPanelClient::dismissPanel const):

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
(-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]):
(-[TestWebAuthenticationPanelDelegate panel:dismissWebAuthenticationPanelWithResult:]):
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252035 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm

index 19c0d62..0a0aa08 100644 (file)
@@ -1,3 +1,33 @@
+2019-11-04  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
+        https://bugs.webkit.org/show_bug.cgi?id=203830
+        <rdar://problem/56797134>
+
+        Reviewed by Brent Fulgham .
+
+        -[_WKWebAuthenticationPanel cancel] was only expected to be called on behalf of an
+        explicit user cancel from the UI. However, clients may call it in different other
+        unexpected scenarios as well. We should guard against that.
+
+        To do so, two counter ways are implemented:
+        1) AuthenticatorManager::cancelRequest is changed to invoke the pending request if there
+        is no GlobalFrameID. This case can only be reached via calling -[_WKWebAuthenticationPanel cancel]
+        before AuthenticatorManager::cancelRequest.
+        2) WebAuthenticationPanelClient::updatePanel and WebAuthenticationPanelClient::dismissPanel
+        will call delegate methods in the next run loop to prevent -[_WKWebAuthenticationPanel cancel]
+        being called in the delegates.
+
+        Covered by new API tests.
+
+        * UIProcess/WebAuthentication/AuthenticatorManager.cpp:
+        (WebKit::AuthenticatorManager::cancelRequest):
+        (WebKit::AuthenticatorManager::createService const):
+        (WebKit::AuthenticatorManager::invokePendingCompletionHandler):
+        * UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:
+        (WebKit::WebAuthenticationPanelClient::updatePanel const):
+        (WebKit::WebAuthenticationPanelClient::dismissPanel const):
+
 2019-11-04  Ross Kirsling  <ross.kirsling@sony.com>
 
         Unreviewed fix for non-unified build.
index cce8379..6f371d7 100644 (file)
@@ -175,15 +175,16 @@ void AuthenticatorManager::handleRequest(WebAuthenticationRequestData&& data, Ca
     runPanel();
 }
 
-void AuthenticatorManager::cancelRequest(const WebCore::PageIdentifier& pageID, const Optional<FrameIdentifier>& frameID)
+void AuthenticatorManager::cancelRequest(const PageIdentifier& pageID, const Optional<FrameIdentifier>& frameID)
 {
     if (!m_pendingCompletionHandler)
         return;
-    ASSERT(m_pendingRequestData.frameID);
-    if (m_pendingRequestData.frameID->pageID != pageID)
-        return;
-    if (frameID && frameID != m_pendingRequestData.frameID->frameID)
-        return;
+    if (auto pendingFrameID = m_pendingRequestData.frameID) {
+        if (pendingFrameID->pageID != pageID)
+            return;
+        if (frameID && frameID != pendingFrameID->frameID)
+            return;
+    }
     invokePendingCompletionHandler(ExceptionData { NotAllowedError, "Operation timed out."_s });
     clearState();
     m_requestTimeOutTimer.stop();
@@ -194,6 +195,7 @@ void AuthenticatorManager::cancelRequest(const WebCore::PageIdentifier& pageID,
 // "If the user exercises a user agent user-interface option to cancel the process,".
 void AuthenticatorManager::cancelRequest(const API::WebAuthenticationPanel& panel)
 {
+    RELEASE_ASSERT(RunLoop::isMain());
     if (!m_pendingCompletionHandler || m_pendingRequestData.panel.get() != &panel)
         return;
     resetState();
@@ -267,7 +269,7 @@ void AuthenticatorManager::authenticatorStatusUpdated(WebAuthenticationStatus st
         panel->client().updatePanel(status);
 }
 
-UniqueRef<AuthenticatorTransportService> AuthenticatorManager::createService(WebCore::AuthenticatorTransport transport, AuthenticatorTransportService::Observer& observer) const
+UniqueRef<AuthenticatorTransportService> AuthenticatorManager::createService(AuthenticatorTransport transport, AuthenticatorTransportService::Observer& observer) const
 {
     return AuthenticatorTransportService::create(transport, observer);
 }
@@ -343,16 +345,15 @@ void AuthenticatorManager::runPanel()
 void AuthenticatorManager::invokePendingCompletionHandler(Respond&& respond)
 {
     if (auto *panel = m_pendingRequestData.panel.get()) {
-        WTF::switchOn(respond, [&](const WebCore::PublicKeyCredentialData&) {
+        WTF::switchOn(respond, [&](const PublicKeyCredentialData&) {
             panel->client().dismissPanel(WebAuthenticationResult::Succeeded);
-        }, [&](const WebCore::ExceptionData&) {
+        }, [&](const ExceptionData&) {
             panel->client().dismissPanel(WebAuthenticationResult::Failed);
         });
     }
     m_pendingCompletionHandler(WTFMove(respond));
 }
 
-
 void AuthenticatorManager::resetState()
 {
     m_authenticators.clear();
index 3a40303..35ef65b 100644 (file)
@@ -30,6 +30,7 @@
 
 #import "WebAuthenticationFlags.h"
 #import "_WKWebAuthenticationPanel.h"
+#import <wtf/RunLoop.h>
 
 namespace WebKit {
 
@@ -58,14 +59,21 @@ static _WKWebAuthenticationPanelUpdate wkWebAuthenticationPanelUpdate(WebAuthent
 
 void WebAuthenticationPanelClient::updatePanel(WebAuthenticationStatus status) const
 {
-    if (!m_delegateMethods.panelUpdateWebAuthenticationPanel)
-        return;
+    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the state
+    // of the current run loop in unexpected ways.
+    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, status] {
+        if (!weakThis)
+            return;
 
-    auto delegate = m_delegate.get();
-    if (!delegate)
-        return;
+        if (!m_delegateMethods.panelUpdateWebAuthenticationPanel)
+            return;
 
-    [delegate panel:m_panel updateWebAuthenticationPanel:wkWebAuthenticationPanelUpdate(status)];
+        auto delegate = m_delegate.get();
+        if (!delegate)
+            return;
+
+        [delegate panel:m_panel updateWebAuthenticationPanel:wkWebAuthenticationPanelUpdate(status)];
+    });
 }
 
 static _WKWebAuthenticationResult wkWebAuthenticationResult(WebAuthenticationResult result)
@@ -82,14 +90,21 @@ static _WKWebAuthenticationResult wkWebAuthenticationResult(WebAuthenticationRes
 
 void WebAuthenticationPanelClient::dismissPanel(WebAuthenticationResult result) const
 {
-    if (!m_delegateMethods.panelDismissWebAuthenticationPanelWithResult)
-        return;
+    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the state
+    // of the current run loop in unexpected ways.
+    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, result] {
+        if (!weakThis)
+            return;
+
+        if (!m_delegateMethods.panelDismissWebAuthenticationPanelWithResult)
+            return;
 
-    auto delegate = m_delegate.get();
-    if (!delegate)
-        return;
+        auto delegate = m_delegate.get();
+        if (!delegate)
+            return;
 
-    [delegate panel:m_panel dismissWebAuthenticationPanelWithResult:wkWebAuthenticationResult(result)];
+        [delegate panel:m_panel dismissWebAuthenticationPanelWithResult:wkWebAuthenticationResult(result)];
+    });
 }
 
 } // namespace WebKit
index a5ac67d..8125a75 100644 (file)
@@ -1,3 +1,16 @@
+2019-11-04  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
+        https://bugs.webkit.org/show_bug.cgi?id=203830
+        <rdar://problem/56797134>
+
+        Reviewed by Brent Fulgham .
+
+        * TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
+        (-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]):
+        (-[TestWebAuthenticationPanelDelegate panel:dismissWebAuthenticationPanelWithResult:]):
+        (TestWebKitAPI::TEST):
+
 2019-11-04  Jonathan Bedard  <jbedard@apple.com>
 
         webkitpy: Build ImageDiff if it is missing
index 41ea4ce..f23cfc7 100644 (file)
@@ -44,6 +44,7 @@ static bool webAuthenticationPanelFailed = false;
 static bool webAuthenticationPanelSucceded = false;
 static bool webAuthenticationPanelUpdateMultipleNFCTagsPresent = false;
 static bool webAuthenticationPanelUpdateNoCredentialsFound = false;
+static bool webAuthenticationPanelCancelImmediately = false;
 
 @interface TestWebAuthenticationPanelDelegate : NSObject <_WKWebAuthenticationPanelDelegate>
 @end
@@ -53,6 +54,9 @@ static bool webAuthenticationPanelUpdateNoCredentialsFound = false;
 - (void)panel:(_WKWebAuthenticationPanel *)panel updateWebAuthenticationPanel:(_WKWebAuthenticationPanelUpdate)update
 {
     ASSERT_NE(panel, nil);
+    if (webAuthenticationPanelCancelImmediately)
+        [panel cancel];
+
     if (update == _WKWebAuthenticationPanelUpdateMultipleNFCTagsPresent) {
         webAuthenticationPanelUpdateMultipleNFCTagsPresent = true;
         return;
@@ -66,6 +70,9 @@ static bool webAuthenticationPanelUpdateNoCredentialsFound = false;
 - (void)panel:(_WKWebAuthenticationPanel *)panel dismissWebAuthenticationPanelWithResult:(_WKWebAuthenticationResult)result
 {
     ASSERT_NE(panel, nil);
+    if (webAuthenticationPanelCancelImmediately)
+        [panel cancel];
+
     if (result == _WKWebAuthenticationResultFailed) {
         webAuthenticationPanelFailed = true;
         return;
@@ -189,6 +196,7 @@ static void reset()
     webAuthenticationPanelRan = false;
     webAuthenticationPanelFailed = false;
     webAuthenticationPanelSucceded = false;
+    webAuthenticationPanelCancelImmediately = false;
 }
 
 static void checkPanel(_WKWebAuthenticationPanel *panel, NSString *relyingPartyID, NSArray *transports, _WKWebAuthenticationType type)
@@ -680,6 +688,59 @@ TEST(WebAuthenticationPanel, PanelMultipleNFCTagsPresent)
 }
 #endif
 
+TEST(WebAuthenticationPanel, PanelHidCancelReloadNoCrash)
+{
+    reset();
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid-cancel" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
+    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
+    [webView setUIDelegate:delegate.get()];
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&webAuthenticationPanelRan);
+    [[delegate panel] cancel];
+    [webView reload];
+    [webView waitForMessage:@"Operation timed out."];
+}
+
+TEST(WebAuthenticationPanel, PanelHidSuccessCancelNoCrash)
+{
+    reset();
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
+    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
+    [webView setUIDelegate:delegate.get()];
+    webAuthenticationPanelCancelImmediately = true;
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    [webView waitForMessage:@"Succeeded!"];
+}
+
+TEST(WebAuthenticationPanel, PanelHidCtapNoCredentialsFoundCancelNoCrash)
+{
+    reset();
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid-no-credentials" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
+    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
+    [webView setUIDelegate:delegate.get()];
+    webAuthenticationPanelCancelImmediately = true;
+
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    Util::run(&webAuthenticationPanelUpdateNoCredentialsFound);
+}
+
 } // namespace TestWebKitAPI
 
 #endif // ENABLE(WEB_AUTHN)