Bug 234180

Summary: [WebAuthn] Allow same-site, cross-origin iframe get()
Product: WebKit Reporter: pascoe <pascoe>
Component: WebKit Misc.Assignee: pascoe <pascoe>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, commit-queue, esprehn+autocc, ews-watchlist, gyuyoung.kim, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234283, 234309    
Bug Blocks: 222240    
Attachments:
Description Flags
Patch
none
Patch ews-feeder: commit-queue-

Description pascoe@apple.com 2021-12-10 15:02:51 PST
WebAuthn Level 2 specifies a feature policy: 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.

This patch implements this functionality only for same-site, cross-origin i-frames.
Comment 1 pascoe@apple.com 2021-12-10 15:03:16 PST
rdar://85161142
Comment 2 pascoe@apple.com 2021-12-10 15:18:38 PST
Created attachment 446826 [details]
Patch
Comment 3 Brent Fulgham 2021-12-13 09:43:03 PST
Comment on attachment 446826 [details]
Patch

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

I think this patch needs a bit of work before it's ready to land. Please adjust the same-site test, and switch from a boolean to a two-state enum that documents its purpose.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:184
> +        if (domain != RegistrableDomain(parentDocument->securityOrigin().data()))

You should use "areRegistrableDomainsEqual" from the RegistrableDomains header.

We identify same-site in Document like this:

    // Only prevent cross-site navigations.
    RefPtr targetDocument = targetFrame.document();
    if (targetDocument && (targetDocument->securityOrigin().isSameOriginDomain(SecurityOrigin::create(destinationURL)) || areRegistrableDomainsEqual(targetDocument->url(), destinationURL)))
        return false;

You should probably use something like that to be consistent. It should also handle all the weird cases of data/Blob URLs and so forth properly.

> Source/WebCore/Modules/webauthn/WebAuthenticationUtils.h:55
> +WEBCORE_EXPORT Ref<ArrayBuffer> buildClientDataJson(ClientDataType /*type*/, const BufferSource& challenge, const SecurityOrigin& /*origin*/, const bool crossOrigin);

Instead of this boolean, we should create a two-state enum to represent this state. Something like WebAuthn::CrossOrigin::Yes/No, or WebAuthn::Scope::CrossOrigin/SameOrigin. There are many examples in the sources of this pattern.
Comment 4 pascoe@apple.com 2021-12-13 13:38:52 PST
Created attachment 447053 [details]
Patch
Comment 5 Brent Fulgham 2021-12-13 14:22:16 PST
Comment on attachment 447053 [details]
Patch

Looks great -- thank you for making those changes! r=me
Comment 6 EWS 2021-12-13 16:00:08 PST
Committed r286993 (?): <https://commits.webkit.org/r286993>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447053 [details].
Comment 7 WebKit Commit Bot 2021-12-13 18:40:50 PST
Re-opened since this is blocked by bug 234283