Bug 191533 - [WebAuthN] Use a real nonce for CTAPHID_INIT
Summary: [WebAuthN] Use a real nonce for CTAPHID_INIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-11-11 18:31 PST by Jiewen Tan
Modified: 2021-03-05 09:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (20.40 KB, patch)
2018-11-14 01:34 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (20.51 KB, patch)
2018-11-15 12:47 PST, 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 Jiewen Tan 2018-11-11 18:31:28 PST
Use a real nonce for CTAPHID_INIT.
Comment 1 Jiewen Tan 2018-11-14 01:34:11 PST
Created attachment 354782 [details]
Patch
Comment 2 Brent Fulgham 2018-11-14 08:21:22 PST
Comment on attachment 354782 [details]
Patch

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

Looks good. r=me

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=191533

Please always include a radar for these changes.

> Source/WebKit/ChangeLog:14
> +        sufficient as the arrived init response will piped in HidConnection::m_inputReports, which is designed in

"will BE piped THROUGH HidConnection::m_inputReports..."

> Source/WebKit/ChangeLog:15
> +        purpose to store any data packets within (initialized, terminated) time interval to prevent data loss in

"... which is designed ON purpose to ..."

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:59
> +    // HidConnection could hold data from other applications, and thereofore invalidate it before each transaction.

It seems like this could be the source of difficult-to-debug issues in the future, though I guess not in WebKit after this change. I wonder how other groups deal with HID connections that contain existing data.
Comment 3 Radar WebKit Bug Importer 2018-11-15 12:07:12 PST
<rdar://problem/46103502>
Comment 4 Jiewen Tan 2018-11-15 12:42:17 PST
Comment on attachment 354782 [details]
Patch

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

Thanks Brent for r+ this change.

>> Source/WebKit/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=191533
> 
> Please always include a radar for these changes.

Sure.

>> Source/WebKit/ChangeLog:14
>> +        sufficient as the arrived init response will piped in HidConnection::m_inputReports, which is designed in
> 
> "will BE piped THROUGH HidConnection::m_inputReports..."

Fixed.

>> Source/WebKit/ChangeLog:15
>> +        purpose to store any data packets within (initialized, terminated) time interval to prevent data loss in
> 
> "... which is designed ON purpose to ..."

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:59
>> +    // HidConnection could hold data from other applications, and thereofore invalidate it before each transaction.
> 
> It seems like this could be the source of difficult-to-debug issues in the future, though I guess not in WebKit after this change. I wonder how other groups deal with HID connections that contain existing data.

Well, actually the CTAPHID_INIT response is the only one we need to pay attention to as the channel id embedded is the same for every application. Other cmd will use a dedicated channel id which is unique per app. Chromium is kind of merging our CtapHidDriver::Worker and CtapHidDriver together, so they just keep reading the pipe when a wrong nonce arrive.This design is free of the deadlock I describe in the change log however it makes the whole driver has much more complicated states than us. At the meantime, our design is simple, CtapHidDriver::Worker only handles each transaction, it doesn't care the content of each transaction. CtapHidDriver manages all states such as channel allocation. Since the worker doesn't care the content of each transaction, it won't know the nonce of the incoming legitimate response (a completed response with a right channel id) is not the expected one, and therefore terminates the transaction. And there is no ways for CtapHidDriver to ask the worker to open a semi transaction which is read only to exhaust the pipe. Therefore, it issues a new transaction. Since the described dead lock should be a quite rare situation in real world as it requires two apps send init cmd at the same time to trigger. I couldn't image how a legitimate user would do this. Therefore, there is no need to make a huge change to tackle this particular issue.
Comment 5 Jiewen Tan 2018-11-15 12:47:12 PST
Created attachment 354972 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2018-11-15 13:25:00 PST
Comment on attachment 354972 [details]
Patch for landing

Clearing flags on attachment: 354972

Committed r238246: <https://trac.webkit.org/changeset/238246>
Comment 7 WebKit Commit Bot 2018-11-15 13:25:02 PST
All reviewed patches have been landed.  Closing bug.