Bug 183533

Summary: [WebAuthn] Formalize the Keychain schema
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, bfulgham, commit-queue, jiewen_tan, jonathan, jschoi, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2018-03-09 15:50:57 PST
Figure out a way to store all credential meta data: PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, and Counter.
Comment 1 Jiewen Tan 2018-03-09 15:53:06 PST
We might also want to update certificates related to the credential as well.
Comment 2 Radar WebKit Bug Importer 2018-08-15 13:41:28 PDT
<rdar://problem/43347926>
Comment 3 Jiewen Tan 2020-03-09 02:03:11 PDT
Created attachment 393016 [details]
Patch
Comment 4 Jiewen Tan 2020-03-09 16:25:43 PDT
Created attachment 393090 [details]
Patch
Comment 5 Brent Fulgham 2020-03-10 10:47:46 PDT
Comment on attachment 393090 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        This patch formalizes the schema for the Keychain as follow:

... as follows:

> Source/WebKit/ChangeLog:35
> +        the trustworthy information that the UserAgent produce. Others can be potentially used by

Also, this is the only trustworthy information that the UserAgent produces. Others could potentially be used by malicious websites ...

> Source/WebKit/ChangeLog:51
> +        within the SEP. So this field is absolutely useless.

What is the purpose of this paragraph? Are you saying that we don't implement the signature counter? If we do implement it (or perhaps just provide a constant value) I don't think we need to have this paragraph.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:335
> +        OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)credentialIdQuery, nullptr);

Where is the result of this Copy going? That needs to be refcounted.
Comment 6 Jiewen Tan 2020-03-10 14:26:06 PDT
Comment on attachment 393090 [details]
Patch

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

Thanks Brent for the review.

>> Source/WebKit/ChangeLog:9
>> +        This patch formalizes the schema for the Keychain as follow:
> 
> ... as follows:

Fixed.

>> Source/WebKit/ChangeLog:35
>> +        the trustworthy information that the UserAgent produce. Others can be potentially used by
> 
> Also, this is the only trustworthy information that the UserAgent produces. Others could potentially be used by malicious websites ...

Fixed.

>> Source/WebKit/ChangeLog:51
>> +        within the SEP. So this field is absolutely useless.
> 
> What is the purpose of this paragraph? Are you saying that we don't implement the signature counter? If we do implement it (or perhaps just provide a constant value) I don't think we need to have this paragraph.

It explains why it is constantly zero in the implementation.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:335
>> +        OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)credentialIdQuery, nullptr);
> 
> Where is the result of this Copy going? That needs to be refcounted.

This is a query to confirm the SecItem presents, and therefore it doesn't ask the Keychain to return anything. Otherwise, returned values will be returned via the nullptr's parameter. We use the status to confirm the existence.
Comment 7 Jiewen Tan 2020-03-10 14:27:38 PDT
Created attachment 393180 [details]
Patch
Comment 8 Brent Fulgham 2020-03-11 11:00:04 PDT
Comment on attachment 393180 [details]
Patch

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

> Source/WebKit/ChangeLog:51
> +        within the SEP. So this field is absolutely useless.

I suggest:

We hard code a zero value for 'signature counter'. While this is a theoretically interesting technique for a RP to detect private key cloning, it is unlikely to be useful in practice. We store the private keys in our SEP. This counter would only be a meaningful protection if adversaries were able to extract private key data from the SEP without Apple noticing, but were not able to manipulate this counter to fool the RP.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:338
> +        ASSERT(!status);

Should this be a RELEASE_ASSERT?
Comment 9 Jiewen Tan 2020-03-11 14:50:32 PDT
Comment on attachment 393180 [details]
Patch

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

Thanks Brent for the r+.

>> Source/WebKit/ChangeLog:51
>> +        within the SEP. So this field is absolutely useless.
> 
> I suggest:
> 
> We hard code a zero value for 'signature counter'. While this is a theoretically interesting technique for a RP to detect private key cloning, it is unlikely to be useful in practice. We store the private keys in our SEP. This counter would only be a meaningful protection if adversaries were able to extract private key data from the SEP without Apple noticing, but were not able to manipulate this counter to fool the RP.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:338
>> +        ASSERT(!status);
> 
> Should this be a RELEASE_ASSERT?

This code is only executed in debug build. So, I think ASSERT is enough.
Comment 10 Jiewen Tan 2020-03-11 14:52:36 PDT
Created attachment 393289 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2020-03-11 15:42:15 PDT
Comment on attachment 393289 [details]
Patch for landing

Clearing flags on attachment: 393289

Committed r258293: <https://trac.webkit.org/changeset/258293>