Bug 213900 - [WebAuthn] Cache the PIN to improve NFC user experience
Summary: [WebAuthn] Cache the PIN to improve NFC user experience
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: iPhone / iPad Other
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
: 214849 (view as bug list)
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2020-07-02 14:38 PDT by login Llama
Modified: 2020-07-28 00:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.23 KB, patch)
2020-07-15 02:01 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (18.25 KB, patch)
2020-07-15 11:31 PDT, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing (17.77 KB, patch)
2020-07-17 15:16 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description login Llama 2020-07-02 14:38:44 PDT
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.
Comment 1 Jiewen Tan 2020-07-15 01:54:40 PDT
<rdar://problem/60073622>
Comment 2 Jiewen Tan 2020-07-15 02:01:34 PDT
Created attachment 404328 [details]
Patch
Comment 3 Jiewen Tan 2020-07-15 02:03:24 PDT
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.
Comment 4 Jiewen Tan 2020-07-15 11:31:37 PDT
Created attachment 404364 [details]
Patch
Comment 5 Brent Fulgham 2020-07-17 14:50:40 PDT
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.
Comment 6 Jiewen Tan 2020-07-17 15:13:14 PDT
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?
Comment 7 Jiewen Tan 2020-07-17 15:16:10 PDT
Created attachment 404603 [details]
Patch for landing
Comment 8 EWS 2020-07-17 15:58:55 PDT
Committed r264543: <https://trac.webkit.org/changeset/264543>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404603 [details].
Comment 9 Jiewen Tan 2020-07-28 00:18:39 PDT
*** Bug 214849 has been marked as a duplicate of this bug. ***