Bug 219711

<rdar://problem/72154840>

Created attachment 416026 [details] Patch

Created attachment 416034 [details] Patch

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 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 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 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.

Created attachment 416059 [details] Patch

Comment on attachment 416059 [details] Patch r=me

Committed r270724: <https://trac.webkit.org/changeset/270724>