Bug 191523 – [WebAuthn] Implement AuthenticatorCancel

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

We need to implement AuthenticatorCancel for local authenticators and ways within WebKit/WebCore to invoke AuthenticatorCancel. Things like refreshing a page could go through this route.

[Radar WebKit Bug Importer]

<rdar://problem/55920204>

[Jiewen Tan]

Created attachment 381169 [details]
Patch

[Chris Dumez]

Comment on attachment 381169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381169&action=review

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:143
> +    const auto& callerOrigin = document.securityOrigin();

we normally omit the "const" when we're using auto.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:102
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());

Looks to me like you're writing in memory that you don't own here? I get that the vector capacity is kHidPacketSize, but it is not its size.
You should swap your too lines, first resize() and then memset.

Also, grow() here would be more efficient than resize() since serialized.size() <= kHidPacketSize.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:151
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());

Same comment as above. ASAN would likely complain here.

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
> +    if (!m_manager)

I think this can be:
if (m_manager)
    m_manager->cancelRequest(*this);

given how simple the method is.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:167
> +void AuthenticatorManager::cancelRequest(const WebPageProxy& page, Optional<FrameIdentifier> subFrameId)

const Optional<FrameIdentifier>&

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:172
> +    if (subFrameId && (subFrameId != *(m_pendingRequestData.frameId)))

Too many parentheses.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:174
> +    invokePendingCompletionHandler(ExceptionData { NotAllowedError, emptyString() });

Probably want a better error message than ""

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h:68
> +    bool isInitialized() { return m_isInitialized; }

should be const.

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
> +    WTF::Optional<WebCore::FrameIdentifier> frameId;

FrameIdentifier are not unique at UIProcess level, or even at WebPageProxy level. So it looks very fragile to use this in UIProcess to identifier a request. A { WebCore::PageIdentifier + WebCore::FrameIdentifier } is globally unique though.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:42
> +    if (!m_driver)

if (m_driver)
    m_driver->cancel();

> Source/WebKit/UIProcess/WebPageProxy.cpp:4119
> +    m_websiteDataStore->authenticatorManager().cancelRequest(*this, frame->isMainFrame() ? WTF::nullopt : Optional<FrameIdentifier>(frameID));

could use makeOptional(frameID)

Seems odd to not be using a frameID for the main frame, since it has one.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:896
> +        // WebPageProxy takes care of the main frame.

Why? Why can't we have a simple code path no matter if the frame is a main frame or not?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:58
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:68
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?

[Brent Fulgham]

Comment on attachment 381169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381169&action=review

I think your handling of the 'getSerializedData' calls is wrong.

> Source/WebKit/ChangeLog:28
> +        Noted, CtapHidDriver is the only CtapDriver implements the cancel method. Since the message

Note: The CtapHidDriver is the only CtapDriver that implements the cancel method."

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
> 
> Not needed. If you did not have a frame, you wouldn't have a page.

We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):

static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }

Then just say:

if (!documentHasValidState(m_document)) { ...

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
> 
> Not needed. If you did not have a frame, you wouldn't have a page.

if (!documentHasValidState(m_document)) { ...

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:103
> +    serialized.resize(kHidPacketSize);

You said that you switched from grow here, because it doesn't initialized POD types. But resize also shrinks the Vector to match the passed input, if it is smaller than the Vector's current size. Is that what you want?

Previously, this logic would make the serialized Vector larger by 'kHidPacketSize' bytes. Now, we change the size of Vector to be 'kHidPacketSize'.

I think you should still keep the 'grow', and after the 'grow' is executed, perform the memset to initialize the range of data.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:152
> +    serialized.resize(kHidPacketSize);

Ditto.

> Tools/TestWebKitAPI/Tests/WebCore/FidoHidMessageTest.cpp:63
> + * Byte 7-n:  Data block

Was the original comment wrong? You don't seem to have changed this test case at all, but the comment is now very different in terms of which bytes mean what.

[Chris Dumez]

Comment on attachment 381169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381169&action=review

>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>> 
>> Not needed. If you did not have a frame, you wouldn't have a page.
> 
> We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):
> 
> static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }
> 
> Then just say:
> 
> if (!documentHasValidState(m_document)) { ...

Really not a fan of this proposal. I'd rather we keep the bogus check.

[Jiewen Tan]

Comment on attachment 381169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381169&action=review

Thanks Chris and Brent for taking a look.

>> Source/WebKit/ChangeLog:28
>> +        Noted, CtapHidDriver is the only CtapDriver implements the cancel method. Since the message
> 
> Note: The CtapHidDriver is the only CtapDriver that implements the cancel method."

Fixed.

>>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
>>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>>> 
>>> Not needed. If you did not have a frame, you wouldn't have a page.
>> 
>> We use this same test in a few places. I wonder if it would be better to have (perhaps come up with a better name for this predicate):
>> 
>> static bool documentHasValidState(Document* document) { return document &&  document->frame() && document->page(); }
>> 
>> Then just say:
>> 
>> if (!documentHasValidState(m_document)) { ...
> 
> Really not a fan of this proposal. I'd rather we keep the bogus check.

Okay, dropped the new check.

>>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
>>> +    if (!m_document || !m_document->frame() || !m_document->page()) {
>> 
>> Not needed. If you did not have a frame, you wouldn't have a page.
> 
> if (!documentHasValidState(m_document)) { ...

Okay, dropped the new check.

>> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:143
>> +    const auto& callerOrigin = document.securityOrigin();
> 
> we normally omit the "const" when we're using auto.

Omitted.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:102
>> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());
> 
> Looks to me like you're writing in memory that you don't own here? I get that the vector capacity is kHidPacketSize, but it is not its size.
> You should swap your too lines, first resize() and then memset.
> 
> Also, grow() here would be more efficient than resize() since serialized.size() <= kHidPacketSize.

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:103
>> +    serialized.resize(kHidPacketSize);
> 
> You said that you switched from grow here, because it doesn't initialized POD types. But resize also shrinks the Vector to match the passed input, if it is smaller than the Vector's current size. Is that what you want?
> 
> Previously, this logic would make the serialized Vector larger by 'kHidPacketSize' bytes. Now, we change the size of Vector to be 'kHidPacketSize'.
> 
> I think you should still keep the 'grow', and after the 'grow' is executed, perform the memset to initialize the range of data.

Fixed as follow:
auto offset = serialized.size();
serialized.grow(kHidPacketSize);
memset(serialized.data() + offset, 0, kHidPacketSize - offset);

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:151
>> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize - serialized.size());
> 
> Same comment as above. ASAN would likely complain here.

Fixed.

>> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:152
>> +    serialized.resize(kHidPacketSize);
> 
> Ditto.

Fixed.

>> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
>> +    if (!m_manager)
> 
> I think this can be:
> if (m_manager)
>     m_manager->cancelRequest(*this);
> 
> given how simple the method is.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:167
>> +void AuthenticatorManager::cancelRequest(const WebPageProxy& page, Optional<FrameIdentifier> subFrameId)
> 
> const Optional<FrameIdentifier>&

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:172
>> +    if (subFrameId && (subFrameId != *(m_pendingRequestData.frameId)))
> 
> Too many parentheses.

Removed one.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:174
>> +    invokePendingCompletionHandler(ExceptionData { NotAllowedError, emptyString() });
> 
> Probably want a better error message than ""

It doesn't matter. Sites will never get this back. Anyway. changed to "Operation timed out.".

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h:68
>> +    bool isInitialized() { return m_isInitialized; }
> 
> should be const.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
>> +    WTF::Optional<WebCore::FrameIdentifier> frameId;
> 
> FrameIdentifier are not unique at UIProcess level, or even at WebPageProxy level. So it looks very fragile to use this in UIProcess to identifier a request. A { WebCore::PageIdentifier + WebCore::FrameIdentifier } is globally unique though.

I don't think GlobalFrameIdentifier is really helpful here as the request can only live within the lifetime of its corresponding web process, but I don't mind adding another safe guard.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:42
>> +    if (!m_driver)
> 
> if (m_driver)
>     m_driver->cancel();

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4119
>> +    m_websiteDataStore->authenticatorManager().cancelRequest(*this, frame->isMainFrame() ? WTF::nullopt : Optional<FrameIdentifier>(frameID));
> 
> could use makeOptional(frameID)
> 
> Seems odd to not be using a frameID for the main frame, since it has one.

Changed the method signature to: AuthenticatorManager::cancelRequest(const WebCore::PageIdentifier&, const Optional<WebCore::FrameIdentifier>&) to use GlobalFrameIdentifier.

>> Source/WebKit/UIProcess/WebProcessProxy.cpp:896
>> +        // WebPageProxy takes care of the main frame.
> 
> Why? Why can't we have a simple code path no matter if the frame is a main frame or not?

Just thought it will be redundant to do it twice as WebPageProxy::resetState is also calling the cancelRequest. It doesn't hurt to call it twice. So, let's simplify the code. Not sure if WebPageProxy::resetState can be omitted.

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:58
>> +    auto messageId = setRequestCallback(WTFMove(handler));
> 
> Should this be using sendWithAsyncReply() ?

Right, a good idea!

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:68
>> +    auto messageId = setRequestCallback(WTFMove(handler));
> 
> Should this be using sendWithAsyncReply() ?

Fixed.

>> Tools/TestWebKitAPI/Tests/WebCore/FidoHidMessageTest.cpp:63
>> + * Byte 7-n:  Data block
> 
> Was the original comment wrong? You don't seem to have changed this test case at all, but the comment is now very different in terms of which bytes mean what.

I guess this particular test predated WebAuthn/FIDO2 and Google folks forgot to update the comment.

[Jiewen Tan]

Created attachment 381240 [details]
Patch

[Jiewen Tan]

Created attachment 381244 [details]
Patch

[Jiewen Tan]

Created attachment 381319 [details]
Patch

[Brent Fulgham]

Comment on attachment 381319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381319&action=review

Looks good! r=me

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:104
> +    memset(serialized.data() + offset, 0, kHidPacketSize - offset);

Yes, this looks much better.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:154
> +    memset(serialized.data() + offset, 0, kHidPacketSize - offset);

Ditto.

[Jiewen Tan]

Comment on attachment 381319 [details]
Patch

Thanks, Brent.

[WebKit Commit Bot]

Comment on attachment 381319 [details]
Patch

Clearing flags on attachment: 381319

Committed r251295: <https://trac.webkit.org/changeset/251295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/251295/webkit

broke http/wpt/webauthn/public-key-credential-get-success-hid.https.html
and caused it to be severely flakey. Diff makes it look like it is now flaking to a pass in the sub test. 

on Mac and iOS Debug wk2

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html

Diff:
--- /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
@@ -4,5 +4,5 @@
 PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with two consecutive requests. 
+FAIL PublicKeyCredential's [[get]] with two consecutive requests. assert_unreached: Should have rejected: This request has been cancelled by a new request. Reached unreachable code

[Truitt Savell]

> and caused it to be severely flakey. Diff makes it look like it is now
> flaking to a pass in the sub test. 

Meant to type "Diff makes it look like it is now flaking to a fail in the sub test."

[Russell Epstein]

(In reply to Truitt Savell from comment #14)
> It looks like the changes in https://trac.webkit.org/changeset/251295/webkit
> 
> broke http/wpt/webauthn/public-key-credential-get-success-hid.https.html
> and caused it to be severely flakey. Diff makes it look like it is now
> flaking to a pass in the sub test. 
> 
> on Mac and iOS Debug wk2
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> get-success-hid.https.html
> 
> Diff:
> ---
> /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
> +++
> /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
> @@ -4,5 +4,5 @@
>  PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a
> mock hid authenticator. 
>  PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in
> a mock hid authenticator. 
>  PASS PublicKeyCredential's [[get]] with mixed options in a mock hid
> authenticator. 
> -PASS PublicKeyCredential's [[get]] with two consecutive requests. 
> +FAIL PublicKeyCredential's [[get]] with two consecutive requests.
> assert_unreached: Should have rejected: This request has been cancelled by a
> new request. Reached unreachable code

Re-opening due to failing regression tests.

[Jiewen Tan]

Bug 202563 will take care of the test failure.