Figure out a way to store all credential meta data: PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, and Counter.
We might also want to update certificates related to the credential as well.
<rdar://problem/43347926>
Created attachment 393016 [details] Patch
Created attachment 393090 [details] Patch
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 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.
Created attachment 393180 [details] Patch
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 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.
Created attachment 393289 [details] Patch for landing
Comment on attachment 393289 [details] Patch for landing Clearing flags on attachment: 393289 Committed r258293: <https://trac.webkit.org/changeset/258293>