Bug 222240

Summary: [WebAuthn] Using WebAuthn within cross-origin iframe elements
Product: WebKit Reporter: Mathieu Perreault <mathieu.perreault>
Component: WebKit Misc.Assignee: pascoe <pascoe>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, jiewen_tan, katherine_cheney, loginllama, pascoe, simon.fraser, tim.cappalli, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234180    
Bug Blocks: 237399    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Mathieu Perreault 2021-02-20 16:40:28 PST
As mentioned in the  WebAuthn Level 2 editor's draft at https://w3c.github.io/webauthn/#sctn-iframe-guidance, functionality to get credentials from a cross-origin iframe should be enabled if the iframe has the allow="publickey-credentials-get" attribute/value pair. Other browsers implementers have implemented this capability and my group sees a relevant use case for this in our product.
Comment 1 Radar WebKit Bug Importer 2021-02-27 16:41:13 PST
<rdar://problem/74830748>
Comment 2 login Llama 2021-03-03 10:15:29 PST
Agreed This is important for Paments and a number of other use-cases.  It should be part of WebAuthn level 2 support.
Comment 3 John Wilander 2021-10-28 09:23:20 PDT
WebKit on Cocoa platforms (and possibly other platforms too) block cookies for cross-site resources by default. All other state and storage is partitioned per first-party site. Whatever this support entails must not regress the privacy protections we already have in place. One way of resolving this is to tie it to the Storage Access API.
Comment 4 John Wilander 2021-10-28 09:24:12 PDT
It would also be good to know if cross-origin but same-site would be sufficient since that is currently not a privacy boundary.
Comment 5 pascoe@apple.com 2021-10-28 11:09:34 PDT
I'm curious about what allowing WebAuthn within a cross-origin i-frame might reveal. The dialog messages show what site is requesting the WebAuthn panel. 

For example, if we implement this and site1.com embeds a google.com webauthn login page, the dialog would show "google.com" is asking to login.

I have seen some discussion of the embedded document obtaining information about the state of the embedding page via if certain calls pass/fail (therefore obtaining information about the i-frame's allow property) here: https://www.w3.org/TR/permissions-policy-1/#privacy-expose-policy . But we already implement several feature policies for camera, fullscreen, microphone, etc in https://github.com/WebKit/WebKit/blob/main/Source/WebCore/html/FeaturePolicy.cpp . I don't see anything anything special about WebAuthn where we might want to restrict the use further.
Comment 6 John Wilander 2021-10-28 11:25:38 PDT
The Storage Access API is the only shipping way to request a cross-site identity and the prompt tells the user that their activity may be tracked by the requesting party if they grant access.

Cross-site WebAuthn would have to convey the same information since as soon as the user is identified, that identity can be shared with the first party website or stored by the third-party in first-party storage.

This is not the same as asking for permission to use device features such as camera. This is about user identity and linking of user identity across websites.

See WebKit's tracking prevention policy: https://webkit.org/tracking-prevention-policy/
Comment 7 pascoe@apple.com 2021-10-28 11:46:43 PDT
I don't see the change being asked for here as providing any special way of of transmitting information between the embedded document and the embedder, even if it's cross-origin. All the current restrictions on communication between the embedding document and the embedded document would still apply.

The webauthn spec specifies that Web Authentication should be disabled in i-frames with a feature policy to allow it here: https://www.w3.org/TR/webauthn-2/#sctn-iframe-guidance 

If a cross-origin embedded site needs to convey information after authentication, they would still need to use existing mechanisms to do it.
Comment 8 John Wilander 2021-10-28 11:58:03 PDT
(In reply to j_pascoe@apple.com from comment #7)
> I don't see the change being asked for here as providing any special way of
> of transmitting information between the embedded document and the embedder,
> even if it's cross-origin. All the current restrictions on communication
> between the embedding document and the embedded document would still apply.
> 
> The webauthn spec specifies that Web Authentication should be disabled in
> i-frames with a feature policy to allow it here:
> https://www.w3.org/TR/webauthn-2/#sctn-iframe-guidance 
> 
> If a cross-origin embedded site needs to convey information after
> authentication, they would still need to use existing mechanisms to do it.

It's not about transmitting. It's about cross-site access to user IDs.

Today, in shipping WebKit, there is no automatic or invisible way to know a user identity in a cross-site iframe unless it has already been established. Such iframes have to call the Storage Access API on a user gesture in their iframe to request access to cookies. The Storage Access API prompt will inform the user that granting access means their activity may be tracked by the requesting party on this first-party website.

Once a cross-site iframe has access to a user identity it can use regular means to share that identity with the first-party, other third-parties, or with itself in the form of a third-party script running in the first-party context. The most common method is postMessage.

The privacy boundary is to make sure cross-site resources cannot identify the user without the user understanding the potential tracking risk involved. Allowing cross-site WebAuthn without informing the user of that risk would create a loophole in how we try to make sure users understand the privacy implications of their choices.
Comment 9 pascoe@apple.com 2022-03-02 16:44:15 PST
Created attachment 453678 [details]
Patch
Comment 10 pascoe@apple.com 2022-03-03 09:46:33 PST
Created attachment 453752 [details]
Patch
Comment 11 pascoe@apple.com 2022-03-03 10:00:03 PST
Created attachment 453754 [details]
Patch
Comment 12 pascoe@apple.com 2022-03-03 12:02:44 PST
Created attachment 453772 [details]
Patch
Comment 13 Brent Fulgham 2022-03-04 12:26:21 PST
Comment on attachment 453772 [details]
Patch

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

I think this is close, but needs a little work before it's ready to land. r- to adjust the naming and clean up some CredentialContainer:scope implementation.

> Source/WebCore/ChangeLog:11
> +        same-site to cross-origin with consent.

I had to read this sentence a few times. I propose:

"""
This patch relaxes the requirement to perform a Web Authentication assertion
inside an i-frame with the "publickey-credentials-get" feature policy from
'same-site' to 'cross-origin with consent'.
"""

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:49
> +std::pair<WebAuthn::Scope, std::optional<SecurityOriginData>> CredentialsContainer::scopeAndSingleParent()

Maybe "singleParent" should be called something like "AuthenticationOrigin", or "RelyingParty"? I think we are authenticating in a frame (which is the relying party) on behalf of the main frame (which is the site seeking to do something based on the authentication succeeding in the frame).

Maybe we should call it  "scopeAndCrossOriginParent"?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:64
> +            parents.add(origin.data());

Collecting the set of parents really only makes sense if we have a plan to select one over the others due to some criteria. Since we seem only care about the first one we encounter, we should just store one:

if (!origin.isSameOriginAs(document->securityOrigin())) {
    isSameOrigin = false;
    if (!singleParent)
        singleParent = origin.data();
}

Or maybe:

if (isSameOrigin && !origin.isSameOriginAs(document->securityOrigin())) {
    isSameOrigin = false;
    singleParent = origin.data();
}

Or maybe:

if (!singleParent && !origin.isSameOriginAs(document->securityOrigin()))
    singleParent = origin.data()

... and get rid of the 'isSameOrigin' flag entirely.

And maybe "singleParent" should be called "crossOriginParent"

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:62
> +    std::pair<WebAuthn::Scope, std::optional<SecurityOriginData>> scopeAndSingleParent();

It might be worth declaring a typedef for this std::pair-based type, since you use it in a number of places.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:205
> +        promise.reject(Exception { NotAllowedError, "The origin of the document is not the same as multiple of its ancestors."_s });

What does "... not the same as multiple of its ancestors" mean here? I think this would be clearer to say:

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:87
>                  }

I looks like the indentation of everything inside HAVE(UNIFIED_ASC_AUTH_UI) needs to be fixed (I realize this predates your change, but we should fix it).
Comment 14 pascoe@apple.com 2022-03-07 08:07:21 PST
Created attachment 453986 [details]
Patch
Comment 15 Brent Fulgham 2022-03-07 10:13:59 PST
Comment on attachment 453986 [details]
Patch

r=me
Comment 16 EWS 2022-03-07 17:04:53 PST
Tools/Scripts/svn-apply failed to apply attachment 453986 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 17 pascoe@apple.com 2022-03-08 09:06:15 PST
Created attachment 454129 [details]
Patch for landing
Comment 18 Chris Dumez 2022-03-08 09:11:39 PST
Comment on attachment 454129 [details]
Patch for landing

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

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:49
> +ScopeAndCrossOriginParent CredentialsContainer::scopeAndCrossOriginParent()

Can this function be marked const? Doesn't seem like it should be causing mutations.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:52
> +        return std::make_pair(WebAuthn::Scope::CrossOrigin, std::nullopt);

With C++17, you should be able to use `std::pair { WebAuthn::Scope::CrossOrigin, std::nullopt }`, instead of std::make_pair().

Same comment applies below.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:185
> +static RetainPtr<ASCCredentialRequestContext> configureRegistrationRequestContext(const PublicKeyCredentialCreationOptions& options, Vector<uint8_t> hash, std::optional<WebCore::GlobalFrameIdentifier> globalFrameID)

We really shouldn't be passing vectors around by value.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:250
> +static inline RetainPtr<ASCPublicKeyCredentialAssertionOptions> configureAssertionOptions(const PublicKeyCredentialRequestOptions& options, Vector<uint8_t> hash, ASCPublicKeyCredentialKind kind, const std::optional<SecurityOriginData>& parentOrigin, RetainPtr<NSMutableArray<ASCPublicKeyCredentialDescriptor *>> allowedCredentials, RetainPtr<NSString> userVerification)

We really shouldn't be passing vectors around by value.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:269
> +static RetainPtr<ASCCredentialRequestContext> configurationAssertionRequestContext(const PublicKeyCredentialRequestOptions& options, Vector<uint8_t> hash, std::optional<WebCore::MediationRequirement> mediation, std::optional<WebCore::GlobalFrameIdentifier> globalFrameID, std::optional<WebCore::SecurityOriginData>& parentOrigin)

ditto.
Comment 19 pascoe@apple.com 2022-03-08 09:24:00 PST
Created attachment 454132 [details]
Patch for landing
Comment 20 EWS 2022-03-08 15:53:11 PST
Committed r291018 (248192@main): <https://commits.webkit.org/248192@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454132 [details].
Comment 21 Brent Fulgham 2022-05-26 14:47:28 PDT
This fix shipped with Safari 15.5 (all platforms).