Use the ProcessPrivilege assertions to guard against accessing keychain/certificate methods outside of the NetworkProcess.
<rdar://problem/38734795>
Created attachment 336943 [details] Patch
Created attachment 336946 [details] Patch
Created attachment 336949 [details] Patch
Created attachment 337029 [details] Patch
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.
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.
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
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.
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
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.
Created attachment 337044 [details] Patch
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.
Comment on attachment 337044 [details] Patch Please add assertions to the followings as well: ArgumentCodersCF::copyPersistentRef, SecItemShimProxy::secItemRequest.
(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!
Created attachment 337047 [details] Patch
(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.
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.
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
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.
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
Created attachment 337060 [details] Patch
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?
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.
Created attachment 337115 [details] Patch
Committed r230225: <https://trac.webkit.org/changeset/230225>