Bug 219711 - [WebAuthn] Adopt new UI for the Security Key getAssertion flow
Summary: [WebAuthn] Adopt new UI for the Security Key getAssertion flow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2020-12-09 14:59 PST by Jiewen Tan
Modified: 2020-12-11 18:03 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.74 KB, patch)
2020-12-11 12:00 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (11.23 KB, patch)
2020-12-11 12:43 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (11.43 KB, patch)
2020-12-11 15:46 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2020-12-09 14:59:17 PST
Adopt new UI for the Security Key getAssertion flow.
Comment 1 Radar WebKit Bug Importer 2020-12-09 14:59:41 PST
<rdar://problem/72154840>
Comment 2 Jiewen Tan 2020-12-11 12:00:43 PST
Created attachment 416026 [details]
Patch
Comment 3 Jiewen Tan 2020-12-11 12:43:49 PST
Created attachment 416034 [details]
Patch
Comment 4 Brent Fulgham 2020-12-11 12:59:54 PST
Comment on attachment 416034 [details]
Patch

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

I don't think this is quite right. I think the m_credentials store should hold the adopted NS object smart pointers to avoid UAF.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.h:81
> +    HashMap<ASCLoginChoiceProtocol *, WebCore::AuthenticatorAssertionResponse*> m_credentials;

I don't like raw pointers. Are we sure these are being cleaned up properly and deallocated?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:111
> +            m_credentials.add((ASCLoginChoiceProtocol *)loginChoice.get(), response.ptr());

I think this would be safer if m_credentals held the adopted NS object. Right now you are relying on the 'loginChoices' array retaining the object, but that might not always hold.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:114
> +        [m_presenter updateInterfaceWithLoginChoices:loginChoices.get()];

Once we reach here, loginChoices will dealloc on return,. That means m_credentials will hold pointers to dealloc'd objects. I don't think we want to do this,

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:152
> +    auto* response = m_credentials.take(loginChoice);

I think this will retrieve a deallocated object in some cases.
Comment 5 Jiewen Tan 2020-12-11 13:08:29 PST
Comment on attachment 416034 [details]
Patch

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

Thanks Brent for reviewing this patch.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.h:81
>> +    HashMap<ASCLoginChoiceProtocol *, WebCore::AuthenticatorAssertionResponse*> m_credentials;
> 
> I don't like raw pointers. Are we sure these are being cleaned up properly and deallocated?

These are just pointers for indexing. There is no need to access the objects of the pointers or holding the objects alive.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:111
>> +            m_credentials.add((ASCLoginChoiceProtocol *)loginChoice.get(), response.ptr());
> 
> I think this would be safer if m_credentals held the adopted NS object. Right now you are relying on the 'loginChoices' array retaining the object, but that might not always hold.

No, I don't need to hold the login choices. Those are "moved" to the UI. I just need a way to remember the mapping such that I can know which response the selected login choice is corresponding to.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:114
>> +        [m_presenter updateInterfaceWithLoginChoices:loginChoices.get()];
> 
> Once we reach here, loginChoices will dealloc on return,. That means m_credentials will hold pointers to dealloc'd objects. I don't think we want to do this,

I think the presenter should retain the array. It is going to need that in its lifetime as the presenter will return which login choice the user selected back to us.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:152
>> +    auto* response = m_credentials.take(loginChoice);
> 
> I think this will retrieve a deallocated object in some cases.

All I need is just the pointer. I don't need the object. The response objects are kept alive in the authenticators.
Comment 6 Brent Fulgham 2020-12-11 15:18:06 PST
Comment on attachment 416034 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.h:81
>>> +    HashMap<ASCLoginChoiceProtocol *, WebCore::AuthenticatorAssertionResponse*> m_credentials;
>> 
>> I don't like raw pointers. Are we sure these are being cleaned up properly and deallocated?
> 
> These are just pointers for indexing. There is no need to access the objects of the pointers or holding the objects alive.

It might avoid confusion in the future if we made these uint64_t or something. This structure looks like a mapping of two types of ObjC objects, and someone might try to pull one and use it for something.

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:152
>>> +    auto* response = m_credentials.take(loginChoice);
>> 
>> I think this will retrieve a deallocated object in some cases.
> 
> All I need is just the pointer. I don't need the object. The response objects are kept alive in the authenticators.

If 'response' is being passed to m_reesponseHandler as an opaque key, I would suggest you make it a uint64_t (or something) so that someone doesn't try to use this code in the future in a way that could trigger UAF.
Comment 7 Jiewen Tan 2020-12-11 15:44:16 PST
Comment on attachment 416034 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.h:81
>>>> +    HashMap<ASCLoginChoiceProtocol *, WebCore::AuthenticatorAssertionResponse*> m_credentials;
>>> 
>>> I don't like raw pointers. Are we sure these are being cleaned up properly and deallocated?
>> 
>> These are just pointers for indexing. There is no need to access the objects of the pointers or holding the objects alive.
> 
> It might avoid confusion in the future if we made these uint64_t or something. This structure looks like a mapping of two types of ObjC objects, and someone might try to pull one and use it for something.

I have changed WebCore::AuthenticatorAssertionResponse* to a RefPtr.

>>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm:152
>>>> +    auto* response = m_credentials.take(loginChoice);
>>> 
>>> I think this will retrieve a deallocated object in some cases.
>> 
>> All I need is just the pointer. I don't need the object. The response objects are kept alive in the authenticators.
> 
> If 'response' is being passed to m_reesponseHandler as an opaque key, I would suggest you make it a uint64_t (or something) so that someone doesn't try to use this code in the future in a way that could trigger UAF.

Got you. Maybe we can fix this issue later. The interface of m_responseHandler is defined a while ago, not by this patch.
Comment 8 Jiewen Tan 2020-12-11 15:46:21 PST
Created attachment 416059 [details]
Patch
Comment 9 Jiewen Tan 2020-12-11 16:59:47 PST Comment hidden (obsolete)
Comment 10 Jiewen Tan 2020-12-11 17:07:13 PST Comment hidden (obsolete)
Comment 11 Brent Fulgham 2020-12-11 17:55:46 PST
Comment on attachment 416059 [details]
Patch

r=me
Comment 12 Jiewen Tan 2020-12-11 18:03:58 PST
Committed r270724: <https://trac.webkit.org/changeset/270724>