<rdar://problem/8758386>
Created attachment 77426 [details] Prevent the web process from using its credential storage and use the UI process’s credential storage
Comment on attachment 77426 [details] Prevent the web process from using its credential storage and use the UI process’s credential storage View in context: https://bugs.webkit.org/attachment.cgi?id=77426&action=review > WebKit2/WebProcess/mac/WebProcessMainMac.mm:107 > + SecKeychainSetPreferenceDomain(kSecPreferencesDomainSystem); Please add a comment indicating what this does.
Fixed in <http://trac.webkit.org/changeset/74648>.
http://trac.webkit.org/changeset/74648 might have broken Qt Linux Release
(In reply to comment #4) > http://trac.webkit.org/changeset/74648 might have broken Qt Linux Release r74652
This change looks surprising for several reasons. - Why do we even want this? There is no explanation in Bugzilla, and the one I see in Radar isn't fully convincing. - IPC doesn't come free - at least chromium tries hard to limit the number of messages. - Doesn't this break default credentials (see ResourceHandle::createNSURLConnection()) - sometimes credentials must be sent preemptively, without waiting for a challenge)? Please CC Brady and myself when making changes to HTTP authentication.
(In reply to comment #6) > This change looks surprising for several reasons. > - Why do we even want this? For security and correctness. The credentials belongs to the client, not to WebKit. > There is no explanation in Bugzilla, and the one I see in Radar isn't fully convincing. > - IPC doesn't come free - at least chromium tries hard to limit the number of messages. > - Doesn't this break default credentials (see ResourceHandle::createNSURLConnection()) - sometimes credentials must be sent preemptively, without waiting for a challenge)? How can there be a default credential for a protection space if there was never a challenge for that protection space? > Please CC Brady and myself when making changes to HTTP authentication. OK.
> For security and correctness. The credentials belongs to the client, not to WebKit. I'm not sure about that, but I may be missing something. Cookies are global (per OS user), and stored credentials seem to be global, too. What makes them private to the client? Maybe Keychain credentials being private to application is how it should be in ideal world, but it doesn't seem to be the case in CFNetwork model. > How can there be a default credential for a protection space if there was never a challenge > for that protection space? Default credentials work when making requests after the first one (ignoring CFNetwork connection pool and per-connection auth schemes like NTLM for simplicity). So, you authenticate to a Basic protection space once, and then all requests to resources in the same space carry credentials upfront. if (shouldUseCredentialStorage && firstRequest().url().protocolInHTTPFamily()) { if (d->m_user.isEmpty() && d->m_pass.isEmpty()) { // <rdar://problem/7174050> - For URLs that match the paths of those previously challenged for HTTP Basic authentication, // try and reuse the credential preemptively, as allowed by RFC 2617. d->m_initialCredential = CredentialStorage::get(firstRequest().url()); Note that WebCore credential storage isn't consulted for default credentials if shouldUseCredentialStorage is false (of course). I'm surprised if this patch didn't break default credential tests. Are those disabled for WebKit2?
(In reply to comment #8) > > For security and correctness. The credentials belongs to the client, not to WebKit. > > I'm not sure about that, but I may be missing something. Cookies are global (per OS user), and stored credentials seem to be global, too. What makes them private to the client? The default credential store is global, but different clients have access to different credentials. > Maybe Keychain credentials being private to application is how it should be in ideal world, but it doesn't seem to be the case in CFNetwork model. Regardless of API, that is how things behave on Mac OS X. > > > How can there be a default credential for a protection space if there was never a challenge > > for that protection space? > > Default credentials work when making requests after the first one (ignoring CFNetwork connection pool and per-connection auth schemes like NTLM for simplicity). So, you authenticate to a Basic protection space once, and then all requests to resources in the same space carry credentials upfront. So the first request is made without credentials, an authentication challenge is issues, and then the credential storage is populated with a default credential, which is used in subsequent requests? > if (shouldUseCredentialStorage && firstRequest().url().protocolInHTTPFamily()) { > if (d->m_user.isEmpty() && d->m_pass.isEmpty()) { > // <rdar://problem/7174050> - For URLs that match the paths of those previously challenged for HTTP Basic authentication, > // try and reuse the credential preemptively, as allowed by RFC 2617. > d->m_initialCredential = CredentialStorage::get(firstRequest().url()); > > Note that WebCore credential storage isn't consulted for default credentials if shouldUseCredentialStorage is false (of course). I think the above code’s interpretation of shouldUseCredentialStorage may need to be corrected. > I'm surprised if this patch didn't break default credential tests. Are those disabled for WebKit2? Strange. If this in fact broke something, please file a new bug.
> The default credential store is global, but different clients have access to different credentials. How does this work? I've always been thinking that unsigned apps didn't have access to passwords unless the user approved, but there wasn't any compartmentalization. I've just verified that a test app sees all credentials in keychain, but calling -[NSURLCredential password] resulted in a confirmation dialog. > So the first request is made without credentials, an authentication challenge is issues, and then the credential storage is populated with a default credential, which is used in subsequent requests? Yes, that's correct. > I think the above code’s interpretation of shouldUseCredentialStorage may need to be corrected. Do you have a specific suggestion? WebCore credential storage isn't properly maintained when the client says that it shouldn't be used, even though it's sometimes written to. > > I'm surprised if this patch didn't break default credential tests. Are those disabled for WebKit2? > Strange. If this in fact broke something, please file a new bug. I expected you to check that, as the person who made this change. I'm still not convinced that this patch general direction is right. If you don't have the time to keep working on this, perhaps it should be rolled out? In fact, our basic-auth-default test is currently disabled with a bogus comment: # WebKit2 needs to support authentication http/tests/appcache/auth.html http/tests/security/credentials-in-referer.html http/tests/xmlhttprequest/basic-auth.html http/tests/xmlhttprequest/basic-auth-default.html http/tests/xmlhttprequest/logout.html http/tests/xmlhttprequest/re-login-async.html
(In reply to comment #10) > > The default credential store is global, but different clients have access to different credentials. > > How does this work? I've always been thinking that unsigned apps didn't have access to passwords unless the user approved, but there wasn't any compartmentalization. Keychain items have access control lists. > > I've just verified that a test app sees all credentials in keychain, but calling -[NSURLCredential password] resulted in a confirmation dialog. The access control list applies to the secret, not other parts of the keychain item. > > So the first request is made without credentials, an authentication challenge is issues, and then the credential storage is populated with a default credential, which is used in subsequent requests? > > Yes, that's correct. > > > I think the above code’s interpretation of shouldUseCredentialStorage may need to be corrected. > > Do you have a specific suggestion? > > WebCore credential storage isn't properly maintained when the client says that it shouldn't be used, even though it's sometimes written to. I will probably need to know more about what that function is trying to do and the expected lifetime of the default credential once it’s entered into the store. In particular, if it is expected to persist across closing all WKViews in an app (which terminates the web process), then it should probably be managed by the UI process. > > > I'm surprised if this patch didn't break default credential tests. Are those disabled for WebKit2? > > Strange. If this in fact broke something, please file a new bug. > > I expected you to check that, as the person who made this change. I'm still not convinced that this patch general direction is right. If you don't have the time to keep working on this, perhaps it should be rolled out? I will see what I can find out using the tests you listed below! > > In fact, our basic-auth-default test is currently disabled with a bogus comment: > > # WebKit2 needs to support authentication > http/tests/appcache/auth.html > http/tests/security/credentials-in-referer.html > http/tests/xmlhttprequest/basic-auth.html > http/tests/xmlhttprequest/basic-auth-default.html > http/tests/xmlhttprequest/logout.html > http/tests/xmlhttprequest/re-login-async.html
> The access control list applies to the secret, not other parts of the keychain item. Thanks, understood. Perhaps we need to find some way to only ask the UI process when it consults the Keychain. Is there is a way to check that the process has access to a secret without displaying UI? I think that there is another loose end here. You changed it so that the web process doesn't read keychain credentials, but it still writes them on its own AFAICT.
(In reply to comment #11) > > In fact, our basic-auth-default test is currently disabled with a bogus comment: That test is failing in the same way in r74647 and in r74648. > > http/tests/xmlhttprequest/logout.html This is the only test of the six that changed after r74648. It is now failing. I am going to investigate.
(In reply to comment #12) > > The access control list applies to the secret, not other parts of the keychain item. > > Thanks, understood. > > Perhaps we need to find some way to only ask the UI process when it consults the Keychain. Is there is a way to check that the process has access to a secret without displaying UI? There is a way, but the web process should never have access to secrets in the keychain anyway. > I think that there is another loose end here. You changed it so that the web process doesn't read keychain credentials, but it still writes them on its own AFAICT. Setting the preference domain is supposed to prevent this.
> > basic-auth-default test is currently disabled with a bogus comment: > That test is failing in the same way in r74647 and in r74648. Did default credentials work in the browser? I don't see how this change could not be breaking them (or making them more difficult to fix if they were broken).
(In reply to comment #15) > > > basic-auth-default test is currently disabled with a bogus comment: > > That test is failing in the same way in r74647 and in r74648. > > Did default credentials work in the browser? I don't see how this change could not be breaking them (or making them more difficult to fix if they were broken). Yes, the test does appear to pass when run in the browser prior to r74648, but not after the change. I am going to use bug 51603 to track this.
> I think that there is another loose end here. You changed it so that the web process doesn't read keychain credentials, but it still writes them on its own AFAICT. > Setting the preference domain is supposed to prevent this. I have a difficulty following this. So, the web process still writes the credentials to Keychain, but in a way that it can't read them, but Safari can? What if the client is not Safari, and it is not signed? Or if the web process doesn't write to Keychain any more, then who does? > I am going to use bug 51603 to track this. Thanks for checking! FWIW, it would be easier for me to review a single patch that fixes <rdar://problem/8758386> without introducing regressions than to follow a trail of follow-ups. If it's split into multiple patches, it's harder to keep track of what remains broken.
Reverted in <http://trac.webkit.org/changeset/74657>.
(In reply to comment #17) > > I think that there is another loose end here. You changed it so that the web process doesn't read keychain credentials, but it still writes them on its own AFAICT. > > Setting the preference domain is supposed to prevent this. > > I have a difficulty following this. So, the web process still writes the credentials to Keychain, but in a way that it can't read them, but Safari can? What if the client is not Safari, and it is not signed? > > Or if the web process doesn't write to Keychain any more, then who does? The UI process should be in charge of read/write access to all permanent credentials, but currently, when a WKCredential with permanent persistence is created in the UI process, it never stores it in the keychain. This, too, will need to be fixed along with this bug.
Currently, CFNetwork writes it. It may take some effort to match its behavior - I don't even know if it writes any "permanent" credential, or if it only does that after authentication succeeds. I haven't checked if WebCore credential storage behavior matches CFNetwork in this regard.