Bug 185848 – Adopt SecKeyProxy SPI in certificate based challenge response code

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Adopt SecKeyProxy API in certificate based challenge response codes.

[Jiewen Tan]

<rdar://problem/34586181>

[Jiewen Tan]

Created attachment 340928 [details]
Patch

[Brent Fulgham]

Comment on attachment 340928 [details]
Patch

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

This is excellent! I'd like one other person to look at the XPC/IPC stuff (perhaps Ryosuke?) but this looks great. Please fix the GTK/WPE build failures.

> Source/WebKit/ChangeLog:13
> +        them to relay crypto operations to the server process while maintaining the same SecKey interfaces alike local operations.

"maintaining the same SecKey interfaces alike local operations."

should probably be:

"maintaining the same SecKey interfaces as used for local operations."

> Source/WebKit/ChangeLog:15
> +        object alive long enough for the client process, i.e. Network Processes in our case, to finish all operations, and destroys

"and destroys the proxy object afterwards." should be "and then destroy the proxy object afterward."

> Source/WebKit/ChangeLog:18
> +        3) A new class called SecKeyProxyStore is then created to bind the lifetime of SecKeyProxy to the WebsiteDataStore while initializes

"while initializes" should be "while initializing"

> Source/WebKit/ChangeLog:19
> +        it correctly. The time while the authentication process reaches WebPageProxy::didReceiveAuthenticationChallengeProxy where we have

"The time while the authentication process reaches" should be "At the time the authentication process reaches"

> Source/WebKit/ChangeLog:20
> +        accesses to the WebsiteDataStore, we haven't yet be able to determine the Credential to authenticate the challenge. Therefore, we

"we haven't yet be able to" should be "we haven't yet been able to"

> Source/WebKit/ChangeLog:23
> +        a strong reference of the SecKeyProxy, and move it to the WebsiteDataStore. And then we create a weak reference of SecKeyProxyStore

"And then we create a weak reference of" should be "We also create a weak reference to"

> Source/WebKit/ChangeLog:24
> +        and move it to the AuthenticationChallenge. Such that we indirectly bind the lifetime of SecKeyProxy to the WebsiteDataStore through

"Such that we indirectly" should be "In this way, we indirectly"

> Source/WebKit/ChangeLog:29
> +        so, it sends xpc messages at the palace where original IPC messages are sent and overwrites the boostrap listener of the xpc connection

'xpc messages at the palace" should be "xpc messages at the place"

> Source/WebKit/ChangeLog:31
> +        5) Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog in macOS should say Safari instead of

"Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog" should be "Tests are manually covered by tlstestwebkit.org. Note that the prompting Keychain dialog"

Also: Is this really macOS-only? I would imagine it would apply to iOS as well.

> Source/WebKit/UIProcess/WebPageProxy.cpp:78
> +#include "SecKeyProxyStore.h"

This include should be wrapped in #if HAVE(SEC_KEY_PROXY)

[Brent Fulgham]

Comment on attachment 340928 [details]
Patch

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

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:62
> +                if (error) {

I think the 'retainError' should probably be done inside the nil check, since there's no point in wrapping the nil in an NSRetainPtr<>. We should probably log this error condition, unless it's a common/expected occurrence.

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:74
> +                for (size_t i = 0; i < total; i++) {

I think this whole loop could be done inside the "if (total)" test.

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:78
> +                    [certificates addObject:(id)certificate];

I think this is a leak: SecCertificateCreateWIthData returns a +1 retain count, and NSMutableArray's "addObject" will retain the argument passed to it.

You should do an "adoptNS" on the return value of SecCertificateCreateWithData,

[Brent Fulgham]

Comment on attachment 340928 [details]
Patch

r- to correct the leak.

[Jiewen Tan]

Comment on attachment 340928 [details]
Patch

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

Thanks Brent for reviewing the patch.

>> Source/WebKit/ChangeLog:13
>> +        them to relay crypto operations to the server process while maintaining the same SecKey interfaces alike local operations.
> 
> "maintaining the same SecKey interfaces alike local operations."
> 
> should probably be:
> 
> "maintaining the same SecKey interfaces as used for local operations."

Fixed.

>> Source/WebKit/ChangeLog:15
>> +        object alive long enough for the client process, i.e. Network Processes in our case, to finish all operations, and destroys
> 
> "and destroys the proxy object afterwards." should be "and then destroy the proxy object afterward."

Fixed.

>> Source/WebKit/ChangeLog:18
>> +        3) A new class called SecKeyProxyStore is then created to bind the lifetime of SecKeyProxy to the WebsiteDataStore while initializes
> 
> "while initializes" should be "while initializing"

Fixed.

>> Source/WebKit/ChangeLog:19
>> +        it correctly. The time while the authentication process reaches WebPageProxy::didReceiveAuthenticationChallengeProxy where we have
> 
> "The time while the authentication process reaches" should be "At the time the authentication process reaches"

Fixed.

>> Source/WebKit/ChangeLog:20
>> +        accesses to the WebsiteDataStore, we haven't yet be able to determine the Credential to authenticate the challenge. Therefore, we
> 
> "we haven't yet be able to" should be "we haven't yet been able to"

Fixed.

>> Source/WebKit/ChangeLog:23
>> +        a strong reference of the SecKeyProxy, and move it to the WebsiteDataStore. And then we create a weak reference of SecKeyProxyStore
> 
> "And then we create a weak reference of" should be "We also create a weak reference to"

Fixed.

>> Source/WebKit/ChangeLog:24
>> +        and move it to the AuthenticationChallenge. Such that we indirectly bind the lifetime of SecKeyProxy to the WebsiteDataStore through
> 
> "Such that we indirectly" should be "In this way, we indirectly"

Fixed.

>> Source/WebKit/ChangeLog:29
>> +        so, it sends xpc messages at the palace where original IPC messages are sent and overwrites the boostrap listener of the xpc connection
> 
> 'xpc messages at the palace" should be "xpc messages at the place"

Fixed.

>> Source/WebKit/ChangeLog:31
>> +        5) Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog in macOS should say Safari instead of
> 
> "Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog" should be "Tests are manually covered by tlstestwebkit.org. Note that the prompting Keychain dialog"
> 
> Also: Is this really macOS-only? I would imagine it would apply to iOS as well.

This is for both iOS and macOS, but only macOS Keychain will prompt the dialog "XXX wants to access the private key stored in XXX".

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:62
>> +                if (error) {
> 
> I think the 'retainError' should probably be done inside the nil check, since there's no point in wrapping the nil in an NSRetainPtr<>. We should probably log this error condition, unless it's a common/expected occurrence.

Fixed. It looks like that we don't usually retain NSError. Hence, I removed the `adoptNS(error)` line. Also, I added the following error message:
LOG_ERROR("Couldn't create identity from end point: %@", error);

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:74
>> +                for (size_t i = 0; i < total; i++) {
> 
> I think this whole loop could be done inside the "if (total)" test.

Fixed.

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:78
>> +                    [certificates addObject:(id)certificate];
> 
> I think this is a leak: SecCertificateCreateWIthData returns a +1 retain count, and NSMutableArray's "addObject" will retain the argument passed to it.
> 
> You should do an "adoptNS" on the return value of SecCertificateCreateWithData,

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:78
>> +#include "SecKeyProxyStore.h"
> 
> This include should be wrapped in #if HAVE(SEC_KEY_PROXY)

Fixed all the places that include SecKeyProxyStore.h. This should solve build error for other builds.

[Jiewen Tan]

Created attachment 341009 [details]
Patch

[Brent Fulgham]

Comment on attachment 341009 [details]
Patch

I think this look good now. I'd like a WK2 owner to double-check the XPC code.

[Alex Christensen]

Comment on attachment 341009 [details]
Patch

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

Is there some kind of signal we should be listening to when the SecKeyProxy becomes invalid?

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:50
> +            ASSERT(type == XPC_TYPE_DICTIONARY);

This seems a bit fragile.  I think we should make it so if anything isn't as expected, we assert and fail gracefully.  Can we trust the values coming from this xpc connection?

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:53
> +            auto challengeID = xpc_dictionary_get_uint64(event, "challenge-id");

It would be good to define these names in a header somewhere so we don't have magic strings in multiple places.  That way we can see that the protocol is the same on the client and server sides.

> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:55
> +            SecIdentityRef identity = NULL;

nullptr, here and elsewhere.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6212
> +        auto secKeyProxyStore = SecKeyProxyStore::create();
> +        authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);
> +        m_websiteDataStore->addSecKeyProxyStore(WTFMove(secKeyProxyStore));

This doesn't look very elegant.  Couldn't we just create it when we're calling initialize in this patch?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:238
> +    Vector<RefPtr<SecKeyProxyStore>> m_secKeyProxyStores;

This could be a Vector<Ref<SecKeyProxyStore>>

[Jiewen Tan]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 341009 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341009&action=review
> 
> Is there some kind of signal we should be listening to when the SecKeyProxy
> becomes invalid?
> 
I don't think so. A finer grained design would be we remove those SecKeyProxy objects once their corresponding challenges are answered by the Networking Process. Since we usually don't expect any callbacks from CFNetwork after submitting the credential, we don't know when to terminate those proxies. We could potential terminate those proxies after receiving a resource response? Not sure if it is necessary though.

[EWS Watchlist]

Comment on attachment 341009 [details]
Patch

Attachment 341009 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7769847

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 341047 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jiewen Tan]

Comment on attachment 341009 [details]
Patch

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

Thanks Alex for reviewing the patch.

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:50
>> +            ASSERT(type == XPC_TYPE_DICTIONARY);
> 
> This seems a bit fragile.  I think we should make it so if anything isn't as expected, we assert and fail gracefully.  Can we trust the values coming from this xpc connection?

like the following?
if (type != XPC_TYPE_DICTIONARY) {
    ASSERT_NOT_REACHED();
   // LOG ?
    return;
}

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:53
>> +            auto challengeID = xpc_dictionary_get_uint64(event, "challenge-id");
> 
> It would be good to define these names in a header somewhere so we don't have magic strings in multiple places.  That way we can see that the protocol is the same on the client and server sides.

Fixed. Added XpcConstants.h

>> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:55
>> +            SecIdentityRef identity = NULL;
> 
> nullptr, here and elsewhere.

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6212
>> +        m_websiteDataStore->addSecKeyProxyStore(WTFMove(secKeyProxyStore));
> 
> This doesn't look very elegant.  Couldn't we just create it when we're calling initialize in this patch?

I don't think so. Please refer to the change log explanation.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:238
>> +    Vector<RefPtr<SecKeyProxyStore>> m_secKeyProxyStores;
> 
> This could be a Vector<Ref<SecKeyProxyStore>>

Fixed.

[Jiewen Tan]

The Win bot's tree is red.

[Alex Christensen]

(In reply to Jiewen Tan from comment #13)
> like the following?
> if (type != XPC_TYPE_DICTIONARY) {
>     ASSERT_NOT_REACHED();
>    // LOG ?
>     return;
> }
And also if the persistence is out of range, if the certificates isn't an array or contains anything unexpected, etc.

[Jiewen Tan]

Created attachment 341058 [details]
Patch

[EWS Watchlist]

Comment on attachment 341058 [details]
Patch

Attachment 341058 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7772299

New failing tests:
stress/destructuring-rest-element.js.ftl-eager

[Alex Christensen]

Comment on attachment 341058 [details]
Patch

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

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:46
> +    // The following xpc event handler overwrites the boostrap event handler and is only used
> +    // to capture client certificate credential.
> +    xpc_connection_set_event_handler(connection->xpcConnection(), ^(xpc_object_t event) {

Will this intercept any other events?

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:50
> +        if (type != XPC_TYPE_ERROR && weakThis) {

This could be an early return instead of indenting everything.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:64
> +            {

I don't think these additional scopes add much.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68
> +                identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error];

This needs to be adopted.  It's a memory leak as it stands now.

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:69
> +                if (error) {

if (!identity || error)

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:81
> +                auto total = xpc_array_get_count(certificateDataArray);
> +                if (total) {

You can declare total inside the if statement.
if (auto total = ...)

> Source/WebKit/UIProcess/WebPageProxy.cpp:6211
> +        auto secKeyProxyStore = SecKeyProxyStore::create();
> +        authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);

I still don't like this design.  Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:182
> +    void addSecKeyProxyStore(RefPtr<SecKeyProxyStore>&&);

This should be a Ref.

[Jiewen Tan]

Comment on attachment 341058 [details]
Patch

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

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:46
>> +    xpc_connection_set_event_handler(connection->xpcConnection(), ^(xpc_object_t event) {
> 
> Will this intercept any other events?

I believe not. Even if so, the beneath assertion would fire.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:50
>> +        if (type != XPC_TYPE_ERROR && weakThis) {
> 
> This could be an early return instead of indenting everything.

Got it.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:64
>> +            {
> 
> I don't think these additional scopes add much.

The scope here is trying to make it clear that the following block is to create the identity.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68
>> +                identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error];
> 
> This needs to be adopted.  It's a memory leak as it stands now.

I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:69
>> +                if (error) {
> 
> if (!identity || error)

Okay.

>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:81
>> +                if (total) {
> 
> You can declare total inside the if statement.
> if (auto total = ...)

Sure.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6211
>> +        authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);
> 
> I still don't like this design.  Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet.

The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that:
1) Bind the lifetime of secKeyProxyStore to websiteDataStore.
2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:182
>> +    void addSecKeyProxyStore(RefPtr<SecKeyProxyStore>&&);
> 
> This should be a Ref.

Fixed.

[Alex Christensen]

Comment on attachment 341058 [details]
Patch

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

>>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68
>>> +                identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error];
>> 
>> This needs to be adopted.  It's a memory leak as it stands now.
> 
> I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool.

I don't think that you're correct.

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:6211
>>> +        authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);
>> 
>> I still don't like this design.  Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet.
> 
> The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that:
> 1) Bind the lifetime of secKeyProxyStore to websiteDataStore.
> 2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready.

I guess you're right.  The challenge doesn't know about the data store.

[Jiewen Tan]

(In reply to Alex Christensen from comment #20)
> Comment on attachment 341058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341058&action=review
> 
> >>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68
> >>> +                identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error];
> >> 
> >> This needs to be adopted.  It's a memory leak as it stands now.
> > 
> > I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool.
> 
> I don't think that you're correct.

Okay. Then what's the rule of using adoptNS()?

> 
> >>> Source/WebKit/UIProcess/WebPageProxy.cpp:6211
> >>> +        authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore);
> >> 
> >> I still don't like this design.  Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet.
> > 
> > The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that:
> > 1) Bind the lifetime of secKeyProxyStore to websiteDataStore.
> > 2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready.
> 
> I guess you're right.  The challenge doesn't know about the data store.

Yup. Maybe there are better ways around?

[Alex Christensen]

(In reply to Jiewen Tan from comment #21)
> Okay. Then what's the rule of using adoptNS()?
If a function returns a newly created object with a +1 retain count, we need to adopt it so we will free it when we're done.

> Yup. Maybe there are better ways around?
Probably.

[Jiewen Tan]

Created attachment 341144 [details]
Patch

[Jiewen Tan]

(In reply to Alex Christensen from comment #22)
> (In reply to Jiewen Tan from comment #21)
> > Okay. Then what's the rule of using adoptNS()?
> If a function returns a newly created object with a +1 retain count, we need
> to adopt it so we will free it when we're done.
> 

Fixed.

> > Yup. Maybe there are better ways around?
> Probably.

[Jiewen Tan]

Comment on attachment 341144 [details]
Patch

Thanks Alex for r+ the patch.

[WebKit Commit Bot]

Comment on attachment 341144 [details]
Patch

Clearing flags on attachment: 341144

Committed r232165: <https://trac.webkit.org/changeset/232165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.