Bug 184214 – Guard against keychain/certificate access outside the network process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Use the ProcessPrivilege assertions to guard against accessing keychain/certificate methods outside of the NetworkProcess.

[Brent Fulgham]

<rdar://problem/38734795>

[Brent Fulgham]

Created attachment 336943 [details]
Patch

[Brent Fulgham]

Created attachment 336946 [details]
Patch

[Brent Fulgham]

Created attachment 336949 [details]
Patch

[Brent Fulgham]

Created attachment 337029 [details]
Patch

[Jiewen Tan]

Comment on attachment 337029 [details]
Patch

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

The patch looks good to me. Please refer to the comments. Also, please consider adding the assertion for the followings:
ArgumentCodersCF::copyPersistentRef,
SecItemShimProxy::secItemRequest.

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:134
> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

I don't think this one is needed.

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:237
> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

Ditto.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:117
> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessRawCookies));

I don't think this one is relevant.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:499
> +        RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

I don't think this one is needed.

[EWS Watchlist]

Comment on attachment 337029 [details]
Patch

Attachment 337029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7184148

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 337037 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 337029 [details]
Patch

Attachment 337029 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7183976

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 337038 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Brent Fulgham]

Comment on attachment 337029 [details]
Patch

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

>> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:134
>> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> I don't think this one is needed.

OK

>> Source/WebCore/platform/mac/SSLKeyGeneratorMac.mm:237
>> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> Ditto.

OK

>> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:117
>> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessRawCookies));
> 
> I don't think this one is relevant.

Yes -- this one was a mistake. Good catch!

>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:499
>> +        RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> I don't think this one is needed.

Really? This seems totally relevant to whether we are accessing credentials outside of the Network process.

[Brent Fulgham]

Created attachment 337044 [details]
Patch

[Jiewen Tan]

Comment on attachment 337029 [details]
Patch

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

>>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:499
>>> +        RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
>> 
>> I don't think this one is needed.
> 
> Really? This seems totally relevant to whether we are accessing credentials outside of the Network process.

You are right. [NSURLCredentialStorage defaultCredentialForProtectionSpace] will use Keychain service API internally when it tries to store persistent credential.

I have a bad feeling that it is actually APIs like this cause us troubles.

[Jiewen Tan]

Comment on attachment 337044 [details]
Patch

Please add assertions to the followings as well:
ArgumentCodersCF::copyPersistentRef,
SecItemShimProxy::secItemRequest.

[Brent Fulgham]

(In reply to Jiewen Tan from comment #14)
> SecItemShimProxy::secItemRequest.

That's in the UIProcess, so it's okay. But I will add to the equivalent parts of Shared/mac/SecItemShim.cpp!

[Brent Fulgham]

Created attachment 337047 [details]
Patch

[Jiewen Tan]

(In reply to Brent Fulgham from comment #15)
> (In reply to Jiewen Tan from comment #14)
> > SecItemShimProxy::secItemRequest.
> 
> That's in the UIProcess, so it's okay. But I will add to the equivalent
> parts of Shared/mac/SecItemShim.cpp!

Got it. I don't think it is necessary to add assertions to Shared/Mac/SecItemShim.cpp though.

[EWS Watchlist]

Comment on attachment 337047 [details]
Patch

Attachment 337047 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7186363

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 337048 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 337047 [details]
Patch

Attachment 337047 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7186406

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 337051 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Brent Fulgham]

Created attachment 337060 [details]
Patch

[youenn fablet]

Comment on attachment 337060 [details]
Patch

LGTM, I am just not sure of the last change.

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

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:115
> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

Maybe we can RELEASE_ASSERT in LocalAuthenticator constructor directly?

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:711
> +        RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

Could we simplify things here? Maybe with something like the following at the beginning of the method:
#if PLATFORM(COCOA)
RELEASE_ASSERT(!secKeyRefDecodingAllowed || hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
#endif
Or maybe the version without secKeyRefDecodingAllowed is good too?

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:746
> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));

Move to the top of the method?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:-310
> -#endif

This change is not explained.
Is it just dead code and we do not want to give CanAccessCredentials to the UIProcess?

[Brent Fulgham]

Comment on attachment 337060 [details]
Patch

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

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:115
>> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> Maybe we can RELEASE_ASSERT in LocalAuthenticator constructor directly?

Good idea. We should never construct one of these outside of the UIProcess.

>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:711
>> +        RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> Could we simplify things here? Maybe with something like the following at the beginning of the method:
> #if PLATFORM(COCOA)
> RELEASE_ASSERT(!secKeyRefDecodingAllowed || hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> #endif
> Or maybe the version without secKeyRefDecodingAllowed is good too?

This might work. I don't think we should be entering this routine outside the Network/UIProcess so the simpler approach might work. I'll upload a patch to see.

>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:746
>> +    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials));
> 
> Move to the top of the method?

Sure!

>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:-310
>> -#endif
> 
> This change is not explained.
> Is it just dead code and we do not want to give CanAccessCredentials to the UIProcess?

We do not want to initialize the security API shims in the WebContent process, so this call should be removed. We only want this initialization to happen in the Networking process.

[Brent Fulgham]

Created attachment 337115 [details]
Patch

[Brent Fulgham]

Committed r230225: <https://trac.webkit.org/changeset/230225>