Changeset 252035 in webkit


Ignore:
Timestamp:
Nov 4, 2019 8:05:07 PM (4 years ago)
Author:
jiewen_tan@apple.com
Message:

[WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
​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):

Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r252030 r252035  
     12019-11-04  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        [WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
     4        https://bugs.webkit.org/show_bug.cgi?id=203830
     5        <rdar://problem/56797134>
     6
     7        Reviewed by Brent Fulgham .
     8
     9        -[_WKWebAuthenticationPanel cancel] was only expected to be called on behalf of an
     10        explicit user cancel from the UI. However, clients may call it in different other
     11        unexpected scenarios as well. We should guard against that.
     12
     13        To do so, two counter ways are implemented:
     14        1) AuthenticatorManager::cancelRequest is changed to invoke the pending request if there
     15        is no GlobalFrameID. This case can only be reached via calling -[_WKWebAuthenticationPanel cancel]
     16        before AuthenticatorManager::cancelRequest.
     17        2) WebAuthenticationPanelClient::updatePanel and WebAuthenticationPanelClient::dismissPanel
     18        will call delegate methods in the next run loop to prevent -[_WKWebAuthenticationPanel cancel]
     19        being called in the delegates.
     20
     21        Covered by new API tests.
     22
     23        * UIProcess/WebAuthentication/AuthenticatorManager.cpp:
     24        (WebKit::AuthenticatorManager::cancelRequest):
     25        (WebKit::AuthenticatorManager::createService const):
     26        (WebKit::AuthenticatorManager::invokePendingCompletionHandler):
     27        * UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:
     28        (WebKit::WebAuthenticationPanelClient::updatePanel const):
     29        (WebKit::WebAuthenticationPanelClient::dismissPanel const):
     30
    1312019-11-04  Ross Kirsling  <ross.kirsling@sony.com>
    232

  • trunk/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp

    r251762 r252035  
    176176}
    177177
    178 void AuthenticatorManager::cancelRequest(const WebCore::PageIdentifier& pageID, const Optional<FrameIdentifier>& frameID)
     178void AuthenticatorManager::cancelRequest(const PageIdentifier& pageID, const Optional<FrameIdentifier>& frameID)
    179179{
    180180    if (!m_pendingCompletionHandler)
    181181        return;
    182     ASSERT(m_pendingRequestData.frameID);
    183     if (m_pendingRequestData.frameID->pageID != pageID)
    184         return;
    185     if (frameID && frameID != m_pendingRequestData.frameID->frameID)
    186         return;
     182    if (auto pendingFrameID = m_pendingRequestData.frameID) {
     183        if (pendingFrameID->pageID != pageID)
     184            return;
     185        if (frameID && frameID != pendingFrameID->frameID)
     186            return;
     187    }
    187188    invokePendingCompletionHandler(ExceptionData { NotAllowedError, "Operation timed out."_s });
    188189    clearState();
    … …  
    195196void AuthenticatorManager::cancelRequest(const API::WebAuthenticationPanel& panel)
    196197{
     198    RELEASE_ASSERT(RunLoop::isMain());
    197199    if (!m_pendingCompletionHandler || m_pendingRequestData.panel.get() != &panel)
    198200        return;
    … …  
    268270}
    269271
    270 UniqueRef<AuthenticatorTransportService> AuthenticatorManager::createService(WebCore::AuthenticatorTransport transport, AuthenticatorTransportService::Observer& observer) const
     272UniqueRef<AuthenticatorTransportService> AuthenticatorManager::createService(AuthenticatorTransport transport, AuthenticatorTransportService::Observer& observer) const
    271273{
    272274    return AuthenticatorTransportService::create(transport, observer);
    … …  
    344346{
    345347    if (auto *panel = m_pendingRequestData.panel.get()) {
    346         WTF::switchOn(respond, [&](const WebCore::PublicKeyCredentialData&) {
     348        WTF::switchOn(respond, [&](const PublicKeyCredentialData&) {
    347349            panel->client().dismissPanel(WebAuthenticationResult::Succeeded);
    348         }, [&](const WebCore::ExceptionData&) {
     350        }, [&](const ExceptionData&) {
    349351            panel->client().dismissPanel(WebAuthenticationResult::Failed);
    350352        });
    … …  
    352354    m_pendingCompletionHandler(WTFMove(respond));
    353355}
    354 
    355356
    356357void AuthenticatorManager::resetState()

  • trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm

    r251317 r252035  
    3131#import "WebAuthenticationFlags.h"
    3232#import "_WKWebAuthenticationPanel.h"
     33#import <wtf/RunLoop.h>
    3334
    3435namespace WebKit {
    … …  
    5960void WebAuthenticationPanelClient::updatePanel(WebAuthenticationStatus status) const
    6061{
    61     if (!m_delegateMethods.panelUpdateWebAuthenticationPanel)
    62         return;
     62    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the state
     63    // of the current run loop in unexpected ways.
     64    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, status] {
     65        if (!weakThis)
     66            return;
    6367
    64     auto delegate = m_delegate.get();
    65     if (!delegate)
    66         return;
     68        if (!m_delegateMethods.panelUpdateWebAuthenticationPanel)
     69            return;
    6770
    68     [delegate panel:m_panel updateWebAuthenticationPanel:wkWebAuthenticationPanelUpdate(status)];
     71        auto delegate = m_delegate.get();
     72        if (!delegate)
     73            return;
     74
     75        [delegate panel:m_panel updateWebAuthenticationPanel:wkWebAuthenticationPanelUpdate(status)];
     76    });
    6977}
    7078
    … …  
    8391void WebAuthenticationPanelClient::dismissPanel(WebAuthenticationResult result) const
    8492{
    85     if (!m_delegateMethods.panelDismissWebAuthenticationPanelWithResult)
    86         return;
     93    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the state
     94    // of the current run loop in unexpected ways.
     95    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this, result] {
     96        if (!weakThis)
     97            return;
    8798
    88     auto delegate = m_delegate.get();
    89     if (!delegate)
    90         return;
     99        if (!m_delegateMethods.panelDismissWebAuthenticationPanelWithResult)
     100            return;
    91101
    92     [delegate panel:m_panel dismissWebAuthenticationPanelWithResult:wkWebAuthenticationResult(result)];
     102        auto delegate = m_delegate.get();
     103        if (!delegate)
     104            return;
     105
     106        [delegate panel:m_panel dismissWebAuthenticationPanelWithResult:wkWebAuthenticationResult(result)];
     107    });
    93108}
    94109

  • trunk/Tools/ChangeLog

    r252031 r252035  
     12019-11-04  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        [WebAuthn] Guard against unexpected -[_WKWebAuthenticationPanel cancel]
     4        https://bugs.webkit.org/show_bug.cgi?id=203830
     5        <rdar://problem/56797134>
     6
     7        Reviewed by Brent Fulgham .
     8
     9        * TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
     10        (-[TestWebAuthenticationPanelDelegate panel:updateWebAuthenticationPanel:]):
     11        (-[TestWebAuthenticationPanelDelegate panel:dismissWebAuthenticationPanelWithResult:]):
     12        (TestWebKitAPI::TEST):
     13
    1142019-11-04  Jonathan Bedard  <jbedard@apple.com>
    215

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm

    r251762 r252035  
    4545static bool webAuthenticationPanelUpdateMultipleNFCTagsPresent = false;
    4646static bool webAuthenticationPanelUpdateNoCredentialsFound = false;
     47static bool webAuthenticationPanelCancelImmediately = false;
    4748
    4849@interface TestWebAuthenticationPanelDelegate : NSObject <_WKWebAuthenticationPanelDelegate>
    … …  
    5455{
    5556    ASSERT_NE(panel, nil);
     57    if (webAuthenticationPanelCancelImmediately)
     58        [panel cancel];
     59
    5660    if (update == _WKWebAuthenticationPanelUpdateMultipleNFCTagsPresent) {
    5761        webAuthenticationPanelUpdateMultipleNFCTagsPresent = true;
    … …  
    6771{
    6872    ASSERT_NE(panel, nil);
     73    if (webAuthenticationPanelCancelImmediately)
     74        [panel cancel];
     75
    6976    if (result == _WKWebAuthenticationResultFailed) {
    7077        webAuthenticationPanelFailed = true;
    … …  
    190197    webAuthenticationPanelFailed = false;
    191198    webAuthenticationPanelSucceded = false;
     199    webAuthenticationPanelCancelImmediately = false;
    192200}
    193201
    … …  
    681689#endif
    682690
     691TEST(WebAuthenticationPanel, PanelHidCancelReloadNoCrash)
     692{
     693    reset();
     694    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid-cancel" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     695
     696    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
     697    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
     698
     699    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
     700    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
     701    [webView setUIDelegate:delegate.get()];
     702
     703    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     704    Util::run(&webAuthenticationPanelRan);
     705    [[delegate panel] cancel];
     706    [webView reload];
     707    [webView waitForMessage:@"Operation timed out."];
     708}
     709
     710TEST(WebAuthenticationPanel, PanelHidSuccessCancelNoCrash)
     711{
     712    reset();
     713    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     714
     715    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
     716    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
     717
     718    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
     719    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
     720    [webView setUIDelegate:delegate.get()];
     721    webAuthenticationPanelCancelImmediately = true;
     722
     723    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     724    [webView waitForMessage:@"Succeeded!"];
     725}
     726
     727TEST(WebAuthenticationPanel, PanelHidCtapNoCredentialsFoundCancelNoCrash)
     728{
     729    reset();
     730    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-get-assertion-hid-no-credentials" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     731
     732    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
     733    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
     734
     735    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
     736    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
     737    [webView setUIDelegate:delegate.get()];
     738    webAuthenticationPanelCancelImmediately = true;
     739
     740    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     741    Util::run(&webAuthenticationPanelUpdateNoCredentialsFound);
     742}
     743
    683744} // namespace TestWebKitAPI
    684745
Note: See TracChangeset for help on using the changeset viewer.