Bug 230906 – Adopt platform UI for WebAuthn

• Component: New Bugs
• Status: RESOLVED FIXED

[Garrett Davidson]

Adopt platform UI for WebAuthn

[Garrett Davidson]

Created attachment 439506 [details]
Patch

[Garrett Davidson]

Created attachment 439552 [details]
Patch

[Garrett Davidson]

Created attachment 439610 [details]
Patch

[Brent Fulgham]

Comment on attachment 439610 [details]
Patch

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

I think this patch looks good, but I think it has some memory management issues. I'm asking David to do a review -- he can give us the right steps to take.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:87
> +                transportString = @"usb";

Nit: Seems like these string conversion things should live where AuthenticatorTransport is defined.

Not needed for this patch, though.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:127
> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];

You should adoptNS here to avoid the possibility of leaking this (e.g., if we add new code someday that does an early return or something).

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:130
> +    ASCPublicKeyCredentialCreationOptions *credentialCreationOptions = [allocASCPublicKeyCredentialCreationOptionsInstance() init];

This should be wrapped in 'adoptNS', since the refcount is increased when you assign to the requestContext member, and we will leak if this isn't cleaned up.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:144
> +    credentialCreationOptions.supportedAlgorithmIdentifiers = WTFMove(supportedAlgorithmIdentifiers);

I don't think this will do what you want. You are just moving a pointer, and the refcount will now be +1  (assuming supportedAlgorithmIdentifiers is refcounted).

I think you need to adoptNS the NSMutableArray, so it gets cleaned up.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:153
> +        credentialCreationOptions.excludedCredentials = WTFMove(excludedCredentials);

Ditto the above.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:187
> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];

I recommend adoptNS here to avoid future mistakes.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:193
> +        requestContext.platformKeyCredentialAssertionOptions = [allocASCPublicKeyCredentialAssertionOptionsInstance() initWithKind:ASCPublicKeyCredentialKindPlatform relyingPartyIdentifier:options.rpId challenge:challenge userVerificationPreference:userVerification allowedCredentials:allowedCredentials];

I think this will leak.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:196
> +        requestContext.securityKeyCredentialAssertionOptions = [allocASCPublicKeyCredentialAssertionOptionsInstance() initWithKind:ASCPublicKeyCredentialKindSecurityKey relyingPartyIdentifier:options.rpId challenge:challenge userVerificationPreference:userVerification allowedCredentials:allowedCredentials];

Ditto.

[David Kilzer (:ddkilzer)]

Comment on attachment 439610 [details]
Patch

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

r- for the reasons Brent mentioned, plus a few others.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:73
> +static inline RetainPtr<NSString> toNSString(UserVerificationRequirement userVerificationRequirement)
> +{
> +    switch (userVerificationRequirement) {
> +    case UserVerificationRequirement::Required:
> +        return adoptNS(@"required");
> +    case UserVerificationRequirement::Discouraged:
> +        return adoptNS(@"discouraged");
> +    case UserVerificationRequirement::Preferred:
> +        return adoptNS(@"preferred");
> +    }
> +
> +    return adoptNS(@"preferred");

There is no need to adoptNS() here as these are immortal NSString constants.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:81
> +    NSMutableArray<NSString *> *transports = nil;
> +    size_t transportCount = descriptor.transports.size();
> +    if (transportCount) {
> +        transports = [[NSMutableArray alloc] initWithCapacity:transportCount];

The `transports` variable here should be a RetainPtr<> (no need to initialize as C++ does that for you):

    RetainPtr<NSMutableArray<NSString *> *> transports;

And assignment should use adoptNS():

         transports = adoptNS([[NSMutableArray alloc] initWithCapacity:transportCount]);

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:105
> +    return adoptNS([allocASCPublicKeyCredentialDescriptorInstance() initWithCredentialID:toNSData(descriptor.idVector).get() transports:WTFMove(transports)]);

The `WTFMove(transports)` expression is wrong; I'm surprised it even compiles!

This should change to `transports.get()` once you use RetainPtr<>/adopNS() above.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:120
> +    std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> authenticatorSelection = options.authenticatorSelection;
> +    if (authenticatorSelection) {
> +        std::optional<AuthenticatorAttachment> attachment = authenticatorSelection->authenticatorAttachment;
> +        if (attachment == AuthenticatorAttachment::Platform)
> +            requestTypes = ASCCredentialRequestTypePlatformPublicKeyRegistration;
> +        else if (attachment == AuthenticatorAttachment::CrossPlatform)
> +            requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyRegistration;

What if `attachment` is not set?  I don't see a check to ensure that `attachment` is set before comparing it.

Also, shouldn't these comparisons use`*attachment` instead of `attachment` to get the value out of the std::optional<>?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:122
> +        userVerification = toNSString(authenticatorSelection->userVerification).get();

If you ever returned non-immortal NSString objects from toNSString() in the future, the NSString returned by get() here would be over-released once this line of code completes.

It's better to change `userVerification` to be `RetainPtr<NSString>` instead, and drop the `.get()` here, and move the `.get()` below to where it's needed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:127
>> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];
> 
> You should adoptNS here to avoid the possibility of leaking this (e.g., if we add new code someday that does an early return or something).

Correct.  You can also use `auto` to reduce typing:

    auto requestContext = adoptNS([allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes]);

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:130
>> +    ASCPublicKeyCredentialCreationOptions *credentialCreationOptions = [allocASCPublicKeyCredentialCreationOptionsInstance() init];
> 
> This should be wrapped in 'adoptNS', since the refcount is increased when you assign to the requestContext member, and we will leak if this isn't cleaned up.

Correct.  Same as previous comment using `auto` and `adoptNS()` here.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:144
>> +    credentialCreationOptions.supportedAlgorithmIdentifiers = WTFMove(supportedAlgorithmIdentifiers);
> 
> I don't think this will do what you want. You are just moving a pointer, and the refcount will now be +1  (assuming supportedAlgorithmIdentifiers is refcounted).
> 
> I think you need to adoptNS the NSMutableArray, so it gets cleaned up.

Correct.  Please switch to adoptNS() and RetainPtr<> for `supportedAlgorithmIdentifiers`.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:153
>> +        credentialCreationOptions.excludedCredentials = WTFMove(excludedCredentials);
> 
> Ditto the above.

Correct.  Please use adoptNS() and RetainPtr<> for `excludedCredentials`.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:174
> +    std::optional<AuthenticatorAttachment> attachment = options.authenticatorAttachment;
> +    if (attachment == AuthenticatorAttachment::Platform)
> +        requestTypes = ASCCredentialRequestTypePlatformPublicKeyAssertion;
> +    else if (attachment == AuthenticatorAttachment::CrossPlatform)
> +        requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyAssertion;

Ditto from comment about about checking whether `attachment` is set before using it, and using `*attachment` instead of `attachment` here.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:176
> +    userVerification = toNSString(options.userVerification).get();

Change `userVerification` to be `RetainPtr<NSString>` in this method as well.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:181
> +    NSMutableArray<ASCPublicKeyCredentialDescriptor *> *allowedCredentials = nil;
> +    if (allowedCredentialCount) {
> +        allowedCredentials = [[NSMutableArray alloc] initWithCapacity:allowedCredentialCount];

Should use RetainPtr<> and adoptNS() here.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:187
>> +    ASCCredentialRequestContext *requestContext = [allocASCCredentialRequestContextInstance() initWithRequestTypes:requestTypes];
> 
> I recommend adoptNS here to avoid future mistakes.

Correct.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:193
>> +        requestContext.platformKeyCredentialAssertionOptions = [allocASCPublicKeyCredentialAssertionOptionsInstance() initWithKind:ASCPublicKeyCredentialKindPlatform relyingPartyIdentifier:options.rpId challenge:challenge userVerificationPreference:userVerification allowedCredentials:allowedCredentials];
> 
> I think this will leak.

Yes, both `allowedCredentials` and `requestContext` need to use RetainPtr<> types with adoptNS().

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:198
> +    return adoptNS(requestContext);

As a bonus, you don't have to use adoptNS() here if you use it above.

This is one place where you could use WTFMove(requestContext) if the variable is already a RetainPtr<>, though.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:203
> +    RetainPtr<ASCCredentialRequestContext> requestContext = configureRegistrationRequestContext(WTFMove(options));

It's a bit strange to use `WTFMove(options)` here, but configureRegistrationRequestContext() doesn't take a rvalue reference.

I don't think this is wrong, but I'm not 100% sure.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:215
> +    ASCAgentProxy *proxy = [allocASCAgentProxyInstance() init];

Please use RetainPtr<>/adoptNS().

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:218
> +    [proxy performAuthorizationRequestsForContext:requestContext.get() withClearanceHandler:makeBlockPtr([this, handler = WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint *daemonEnpoint, NSError *error) mutable {

What happens when the `this` object is deallocated?  Is this call async, or is guaranteed to finish before the method returns?

If it's async (or `this` can be deleted from another thread), we may need to look into using a WeakPtr for `this`.  If this all happens on the main thread, you don't need WeakPtr.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:226
> +        m_presenter = [allocASCAuthorizationRemotePresenterInstance() init];

This needs adoptNS() or it will leak since the assignment to the instance variable will +1 retain it again.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:269
> +                    exceptionCode = (ExceptionCode) error.code;

It's a bit dodgy to use a C-style cast here.  Usually we create a helper function to map from one type to the other, then provide a default value if an unknown value is seen.

Can an `error.code` be returned that doesn't map to `ExceptionCode`?

[Radar WebKit Bug Importer]

<rdar://problem/83896254>

[Garrett Davidson]

Created attachment 440305 [details]
Patch

[Garrett Davidson]

Comment on attachment 439610 [details]
Patch

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

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:120
>> +            requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyRegistration;
> 
> What if `attachment` is not set?  I don't see a check to ensure that `attachment` is set before comparing it.
> 
> Also, shouldn't these comparisons use`*attachment` instead of `attachment` to get the value out of the std::optional<>?

If `attachment` is unset or an unexpected value, then we intentionally leave `requestTypes` unchanged. My understanding from https://en.cppreference.com/w/cpp/utility/optional/operator_cmp is that std::optional does the expected thing with `==` if `attachment` is unset, which local testing seems to confirm (but I'm by no means an expert here 🙂).

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:174
>> +        requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyAssertion;
> 
> Ditto from comment about about checking whether `attachment` is set before using it, and using `*attachment` instead of `attachment` here.

I _think_ this is currently doing what I want, but I'm happy to update it if I'm missing something or there's a style preference.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:218
>> +    [proxy performAuthorizationRequestsForContext:requestContext.get() withClearanceHandler:makeBlockPtr([this, handler = WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint *daemonEnpoint, NSError *error) mutable {
> 
> What happens when the `this` object is deallocated?  Is this call async, or is guaranteed to finish before the method returns?
> 
> If it's async (or `this` can be deleted from another thread), we may need to look into using a WeakPtr for `this`.  If this all happens on the main thread, you don't need WeakPtr.

This method is async. The clearanceHandler may take a while to get invoked, depending on what else is happening on the system. I'll switch to a WeakPtr.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:269
>> +                    exceptionCode = (ExceptionCode) error.code;
> 
> It's a bit dodgy to use a C-style cast here.  Usually we create a helper function to map from one type to the other, then provide a default value if an unknown value is seen.
> 
> Can an `error.code` be returned that doesn't map to `ExceptionCode`?

Currently I don't think so, but I'm also happy to go the helper function route.

[Garrett Davidson]

Created attachment 440851 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 440851 [details]
Patch

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

r-, but fixing the requested changes should lead to an r+.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:239
> +    NSData *challenge = toNSData(options.challenge).get();

This causes `challenge` to be assigned a deallocated NSData pointer because the temporary RetainPtr<NSData> returned will be deallocated (and release the NSData object with it).

You want to keep the return value as a RetainPtr<NSData>.  Here's one way:

     auto challenge = toNSData(options.challenge);

The code below will need to be changed to `challenge.get()`, but the compiler will tell you if you missed any.

This must be fixed or we will introduce a use-after-free bug.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:267
> +    NSWindow *window = m_webPageProxy.platformWindow();
> +    [proxy performAuthorizationRequestsForContext:requestContext.get() withClearanceHandler:makeBlockPtr([weakThis = makeWeakPtr(*this), handler = WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint *daemonEnpoint, NSError *error) mutable {

The `window = WTFMove(window)` here is wrong as you're trying to move an NSWindow * Objective-C object.

I think it would be better to wrap the NSWindow * object in a RetainPtr and WTFMove that:

    RetainPtr<NSWindow> window = m_webPageProxy.platformWindow();

Note that we don't want to use adoptNS() here because we're not getting a +1 retained object back (under MRR).

This could also be written as this, but it makes the type more opaque:

    auto window = retainPtr(m_webPageProxy.platformWindow());

This must be fixed.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:287
> +                ASCPlatformPublicKeyCredentialRegistration *registrationCredential = credential;

There is a new checked_objc_cast<> function in the new <wtf/cocoa/TypeCastsCocoa.h> header since you started writing this patch.  It would probably be a good idea to do that here to make sure `credentials` is the type of object you expect it to be:

                auto *registrationCredential = checked_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);

Note that if you nil-check the result of the cast, you can use `dynamic_objc_cast<>` instead:

                auto *registrationCredential = dynamic_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);
                if (!registrationCredential) {
                    /* return early with an error, possibly calling the handler() function */
                }

Since this code deals with authentication, I would strongly suggest using one or the other of these cast methods here and the other places where `credential` is assigned a specific type below.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:294
> +                ASCSecurityKeyPublicKeyCredentialRegistration *registrationCredential = credential;

Ditto.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:301
> +                ASCPlatformPublicKeyCredentialAssertion *assertionCredential = credential;

Ditto.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:310
> +                ASCPlatformPublicKeyCredentialAssertion *assertionCredential = credential;

Ditto.

[Garrett Davidson]

Created attachment 441011 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 441011 [details]
Patch

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 440851 [details]
Patch

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

> Source/WebKit/SourcesCocoa.txt:595
> +UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm

If you added a new file to the project, that file still needs to show up in the Xcode project (otherwise searches will ignore the file).

That means you'll have to add the file to the Xcode project, but not add it to any build targets (or it will get built twice and link failures will occur).

So there should be a corresponding change to add the file to the Xcode project (but not build it since it's built by the Unified Sources mechanism).

This must be fixed before landing the patch.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:287
>> +                ASCPlatformPublicKeyCredentialRegistration *registrationCredential = credential;
> 
> There is a new checked_objc_cast<> function in the new <wtf/cocoa/TypeCastsCocoa.h> header since you started writing this patch.  It would probably be a good idea to do that here to make sure `credentials` is the type of object you expect it to be:
> 
>                 auto *registrationCredential = checked_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);
> 
> Note that if you nil-check the result of the cast, you can use `dynamic_objc_cast<>` instead:
> 
>                 auto *registrationCredential = dynamic_objc_cast<ASCPlatformPublicKeyCredentialRegistration>(credential);
>                 if (!registrationCredential) {
>                     /* return early with an error, possibly calling the handler() function */
>                 }
> 
> Since this code deals with authentication, I would strongly suggest using one or the other of these cast methods here and the other places where `credential` is assigned a specific type below.

Garrett pointed out that the -isKindOfClass: check is done above, so no need to do it again using checked_objc_cast<> or dynamic_objc_cast<>, and those won't work with soft-linked classes anyway.

[David Kilzer (:ddkilzer)]

Comment on attachment 440851 [details]
Patch

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

>> Source/WebKit/SourcesCocoa.txt:595
>> +UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm
> 
> If you added a new file to the project, that file still needs to show up in the Xcode project (otherwise searches will ignore the file).
> 
> That means you'll have to add the file to the Xcode project, but not add it to any build targets (or it will get built twice and link failures will occur).
> 
> So there should be a corresponding change to add the file to the Xcode project (but not build it since it's built by the Unified Sources mechanism).
> 
> This must be fixed before landing the patch.

Please ignore this comment--I thought I deleted it, and the Xcode change is already much further down in the patch.

[EWS]

Committed r284071 (242899@main): <https://commits.webkit.org/242899@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441011 [details].