Bug 183533 – [WebAuthn] Formalize the Keychain schema

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

[Jiewen Tan]

Figure out a way to store all credential meta data: PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, and Counter.

[Jiewen Tan]

We might also want to update certificates related to the credential as well.

[Radar WebKit Bug Importer]

<rdar://problem/43347926>

[Jiewen Tan]

Created attachment 393016 [details]
Patch

[Jiewen Tan]

Created attachment 393090 [details]
Patch

[Brent Fulgham]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 393180 [details]
Patch

[Brent Fulgham]

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?

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 393289 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393289 [details]
Patch for landing

Clearing flags on attachment: 393289

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