[Webkit-unassigned] [Bug 230906] Adopt platform UI for WebAuthn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230906

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #440851|review?                     |review-
              Flags|                            |

--- Comment #10 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 440851
  --> https://bugs.webkit.org/attachment.cgi?id=440851
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211012/015b1459/attachment.htm>