Bug 232977 - _WKWebAuthenticationPanel should expose a way to encode CTAP commands
Summary: _WKWebAuthenticationPanel should expose a way to encode CTAP commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-10 18:13 PST by Garrett Davidson
Modified: 2021-11-15 17:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.80 KB, patch)
2021-11-10 18:17 PST, Garrett Davidson
no flags Details | Formatted Diff | Diff
Patch (7.03 KB, patch)
2021-11-11 15:41 PST, Garrett Davidson
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2021-11-11 17:13 PST, Garrett Davidson
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2021-11-11 17:20 PST, Garrett Davidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Garrett Davidson 2021-11-10 18:13:50 PST
_WKWebAuthenticationPanel should expose a way to encode CTAP commands
Comment 1 Garrett Davidson 2021-11-10 18:17:55 PST
Created attachment 443892 [details]
Patch
Comment 2 Garrett Davidson 2021-11-10 18:17:58 PST
<rdar://problem/85279329>
Comment 3 Garrett Davidson 2021-11-11 15:41:21 PST
Created attachment 444012 [details]
Patch
Comment 4 pascoe@apple.com 2021-11-11 16:00:10 PST
Comment on attachment 444012 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:644
> ++ (NSData *)encodeMakeCredentialCommandWithClientDataJSON:(NSData *)clientDataJSON options:(_WKPublicKeyCredentialCreationOptions *)options userVerificationAvailability:(_WKWebAuthenticationUserVerificationAvailability)userVerificationAvailability

Could we just take in the hash on these? Trying to cut down on the amount of times we generate clientDataJSON.

In web flows, it's originally calculated from CredentialsContainer.cpp:92 -> AuthenticatorCoordinator.cpp:246 (client data json generated here and hash passed along) -> WebAuthenticatorCoordinator.cpp:100 and then the hash gets ignored in the new WebAuthenticatorCoordinatorProxy::getAssertion instead of being passed along to ASC agent only to be regenerated later in a call to getAssertionWithChallenge.
Comment 5 David Kilzer (:ddkilzer) 2021-11-11 16:02:33 PST
Comment on attachment 444012 [details]
Patch

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

r- to add ASSERT_NOT_REACHED() and since it looks like tv/watch builds need to be fixed.

Otherwise looks good, but I'm not a domain expert.  :)

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:206
> +

Should have an `ASSERT_NOT_REACHED();` statement here before the `return` statement for Debug builds.
Comment 6 Garrett Davidson 2021-11-11 17:13:54 PST
Created attachment 444027 [details]
Patch
Comment 7 Garrett Davidson 2021-11-11 17:20:32 PST
Created attachment 444029 [details]
Patch
Comment 8 pascoe@apple.com 2021-11-12 08:28:37 PST
(In reply to j_pascoe@apple.com from comment #4)
> Comment on attachment 444012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444012&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:644
> > ++ (NSData *)encodeMakeCredentialCommandWithClientDataJSON:(NSData *)clientDataJSON options:(_WKPublicKeyCredentialCreationOptions *)options userVerificationAvailability:(_WKWebAuthenticationUserVerificationAvailability)userVerificationAvailability
> 
> Could we just take in the hash on these? Trying to cut down on the amount of
> times we generate clientDataJSON.
> 
> In web flows, it's originally calculated from CredentialsContainer.cpp:92 ->
> AuthenticatorCoordinator.cpp:246 (client data json generated here and hash
> passed along) -> WebAuthenticatorCoordinator.cpp:100 and then the hash gets
> ignored in the new WebAuthenticatorCoordinatorProxy::getAssertion instead of
> being passed along to ASC agent only to be regenerated later in a call to
> getAssertionWithChallenge.

Any reason we can't do this?
Comment 9 Garrett Davidson 2021-11-12 09:03:22 PST
> Any reason we can't do this?

Sorry I thought I replied inline on the old patch but I'm not sure what happened to it.

Part of the WebAuthn API contract is that the caller needs the original clientDataJSON back, which is used during signature verification, so the full value needs to be exposed anywhere a command is encoded. That said, _WKWebAuthenticationPanel could definitely start using AuthenticatorCoordinator::produceClientDataJson internally instead of having its own copy, and/or the other methods on _WKWebAuthenticationPanel could accept a full clientDataJSON as well. I think those would both be independent of this change though.
Comment 10 David Kilzer (:ddkilzer) 2021-11-12 14:12:25 PST
Comment on attachment 444029 [details]
Patch

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

r=me
Let me know if you need cq+ as well.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:648
> +    RetainPtr<NSData> clientDataJSON;
> +
> +#if ENABLE(WEB_AUTHN)
> +    clientDataJSON = produceClientDataJson(type, challenge, origin);
> +#endif
> +
> +    return clientDataJSON.autorelease();

Another way to write this would be:

#if ENABLE(WEB_AUTHN)
    return produceClientDataJson(type, challenge, origin).autorelease();
#else
    return nullptr;
#endif

This does NOT need to be changed to land the patch, but it makes it clearer what the code path does when ENABLE(WEB_AUTHN) evaluates to false.

(Similar change could be done for the other methods, too, but no need to change this to land it.)
Comment 11 EWS 2021-11-12 16:55:27 PST
Committed r285763 (244210@main): <https://commits.webkit.org/244210@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444029 [details].
Comment 12 pascoe@apple.com 2021-11-15 15:16:24 PST
(In reply to Garrett Davidson from comment #9)
> > Any reason we can't do this?
> 
> Sorry I thought I replied inline on the old patch but I'm not sure what
> happened to it.
> 
> Part of the WebAuthn API contract is that the caller needs the original
> clientDataJSON back, which is used during signature verification, so the
> full value needs to be exposed anywhere a command is encoded. That said,
> _WKWebAuthenticationPanel could definitely start using
> AuthenticatorCoordinator::produceClientDataJson internally instead of having
> its own copy, and/or the other methods on _WKWebAuthenticationPanel could
> accept a full clientDataJSON as well. I think those would both be
> independent of this change though.

We already made _WKWebAuthenticationPanel start using AuthenticatorCoordinator::produceClientDataJson internally in https://trac.webkit.org/changeset/285617/webkit as using a different implementation broke WebAuthn due to the signature not matching the client data json returned.

The complete ClientDataJson is only needed for returning via the api, which is already done in AuthenticatorCoordinator.cpp:238. Everywhere else, ClientDataHash can be passed around. The authenticator does not need this value, noted here: https://www.w3.org/TR/webauthn-3/#iface-authenticatorresponse

To complete same-site i-frames, we are going to have to add a crossOrigin flag to ClientDataJson, which is complicated by _WKWebAuthenticationPanel also having an SPI to calculate ClientDataJson, so we're going to have to end up removing it and likely changing these new SPIs.
Comment 13 Garrett Davidson 2021-11-15 17:24:38 PST
(In reply to j_pascoe@apple.com from comment #12)

> We already made _WKWebAuthenticationPanel start using
> AuthenticatorCoordinator::produceClientDataJson internally in
> https://trac.webkit.org/changeset/285617/webkit as using a different
> implementation broke WebAuthn due to the signature not matching the client
> data json returned.

I didn't realize that. Neat!


> The complete ClientDataJson is only needed for returning via the api, which
> is already done in AuthenticatorCoordinator.cpp:238. Everywhere else,
> ClientDataHash can be passed around. The authenticator does not need this
> value, noted here:
> https://www.w3.org/TR/webauthn-3/#iface-authenticatorresponse
> 
> To complete same-site i-frames, we are going to have to add a crossOrigin
> flag to ClientDataJson, which is complicated by _WKWebAuthenticationPanel
> also having an SPI to calculate ClientDataJson, so we're going to have to
> end up removing it and likely changing these new SPIs.

Pascoe and I discussed this out of band. We're still thinking about the best way to reduce duplicated work for all of the intended clients here.