[WebAuthn] Cache the PIN to improve NFC user experience

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[login Llama]

In iOS 14 developer beta
The authenticator has a pin set:

The Authenticator is attached over USB/Lightning or NFC.

Tf the external key is removed from the NFC field or USB/Lightning port after the initial CTAP2 getKeyAgrement command getPinToken will fail.

The problem is that the getKeyAgrement happens before the user is prompted to enter pin.  With NFC the natural thing is to remove the key form the NFC field to use your hand to type the pin then tap again.   Unfortunately, since keys are powered via USB or NFC the key will generate a new EC key pair and that will not agree with the one the phone got prior to the power interuption.

The way that Windows 10 implemented it is with two taps.
The first tap the platform gets authenticatorGetInfo.
If the authenticator receives the option uv = True then it proceeds directly to getAssertion with the option uv=1 to trigger internal UV if that fails the authenticator will return pin-required.

If the authenticatorGetInfo has pinToken = true and (uv is not True or received a pin-required error from the previous step):
    Prompt user for pin 
    Prompt user to tap again or re-insert.
    Perform authenticatorClientPIN (0x06) with subcommand getKeyAgreement (0x03)
    Perform authenticatorClientPIN (0x06) with subcommand getPINToken (0x05)
    Perform authenticatorGetAssertion (0x02) with pinAuth form the previous step (might require more than one call depending on allow list size.)

Windows supports CCID NFC readers on the desktop. 
Android only supports CTAP1 over NFC so is not a good example. 

Brave uses the Yubico SDK to implement NFC support.  You can see the two tap pattern there.  Unfortunately, Brave seems to be having a NFC issue on iOS 14 but may work on iOS 13 or did not long ago.

[Jiewen Tan]

<rdar://problem/60073622>

[Jiewen Tan]

Created attachment 404328 [details]
Patch

[Jiewen Tan]

Comment on attachment 404328 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:292
> +        m_pendingRequestData.cachedPin = pin;

Oops, forgot to use the weak pointer.

[Jiewen Tan]

Created attachment 404364 [details]
Patch

[Brent Fulgham]

Comment on attachment 404364 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:284
> +    // issue. The probability of the issue, however, should be rare.

I think this is pretty well documented in the Bugzilla.

I would suggest a shorter note:

// Cache the PIN to improve NFC user experience so that a momentary movement of the NFC key away from the scanner doesn't force the PIN entry to be re-entered.

Question: Should this have some kind of timeout to clear it if some period of time passes?

That might be a reasonable followup patch if you think it's needed. I don't think it's required here.

[Jiewen Tan]

Comment on attachment 404364 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:284
>> +    // issue. The probability of the issue, however, should be rare.
> 
> I think this is pretty well documented in the Bugzilla.
> 
> I would suggest a shorter note:
> 
> // Cache the PIN to improve NFC user experience so that a momentary movement of the NFC key away from the scanner doesn't force the PIN entry to be re-entered.
> 
> Question: Should this have some kind of timeout to clear it if some period of time passes?
> 
> That might be a reasonable followup patch if you think it's needed. I don't think it's required here.

Fixed. For the timer, I'm not sure. The API has a global timeout timer which has a maximum value of 120s. After, the whole request will be cancelled. I guess it's enough?

[Jiewen Tan]

Created attachment 404603 [details]
Patch for landing

[EWS]

Committed r264543: <https://trac.webkit.org/changeset/264543>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404603 [details].

[Jiewen Tan]

*** Bug 214849 has been marked as a duplicate of this bug. ***