Summary: | REGRESSION (Safari 10 combined with WK changes): Unable to store WebCrypto keys in IndexedDB database | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | oyvind.pedersen | ||||||||||||
Component: | WebKit API | Assignee: | Brady Eidson <beidson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | andersca, ap, beidson, jason.mei, jonlee, oyvind.pedersen, sam, trygve.hardersen, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari 10 | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | macOS 10.12 | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=171350 | ||||||||||||||
Attachments: |
|
Description
oyvind.pedersen
2016-09-26 06:26:25 PDT
I have also tried a relevant test on webkit-master found at https://github.com/WebKit/webkit (LayoutTests/crypto/subtle/rsa-indexeddb.html) and verified that it fails on Safari 10 (or rather it does neither PASS nor FAIL, it crashes showing a DataCloneError in the console). DataCloneError (DOM IDBDatabase Exception 25): Failed to store record in an IDBObjectStore: An object could not be cloned. Given that the test passes in WebKitTestRunner, this may be a Safari bug. Using old archive builds we've tracked this to https://trac.webkit.org/changeset/191810 (bizarrely) (In reply to comment #4) > Using old archive builds we've tracked this to > https://trac.webkit.org/changeset/191810 (bizarrely) Never mind this, it's actually because Safari 10 started using WKPageNavigationClient, but does NOT implement the copyWebCryptoMasterKey callback. Combine that with this code in WK2: void WebPageProxy::wrapCryptoKey(const Vector<uint8_t>& key, bool& succeeded, Vector<uint8_t>& wrappedKey) { PageClientProtector protector(m_pageClient); Vector<uint8_t> masterKey; if (m_navigationClient) { if (RefPtr<API::Data> keyData = m_navigationClient->webCryptoMasterKey(*this)) masterKey = keyData->dataReference().vector(); } else if (RefPtr<API::Data> keyData = m_loaderClient->webCryptoMasterKey(*this)) masterKey = keyData->dataReference().vector(); else if (!getDefaultWebCryptoMasterKey(masterKey)) { succeeded = false; return; } succeeded = wrapSerializedCryptoKey(masterKey, key, wrappedKey); } If there *is* a nav client but it does *not* return a master key, this always fails. I think this method should always fallback to the default key. (In reply to comment #5) > (In reply to comment #4) > > If there *is* a nav client but it does *not* return a master key, this > always fails. > > I think this method should always fallback to the default key. (The unwrap method, as well) Created attachment 290264 [details]
Patch
Comment on attachment 290264 [details]
Patch
This has the result of falling back to WebKit even if the delegate implements webCryptoMasterKey(), and calling it legitimately fails (e.g. locked keychain). This seems wrong.
(In reply to comment #8) > Comment on attachment 290264 [details] > Patch > > This has the result of falling back to WebKit even if the delegate > implements webCryptoMasterKey(), and calling it legitimately fails (e.g. > locked keychain). This seems wrong. Hmmmmm good point. (In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 290264 [details] > > Patch > > > > This has the result of falling back to WebKit even if the delegate > > implements webCryptoMasterKey(), and calling it legitimately fails (e.g. > > locked keychain). This seems wrong. > > Hmmmmm good point. Note that this is actually what the LoaderClient part of the code path did already - Even if the client implemented the method but returned a null key, WK2 would still fallback to the default key. I still agree this seems wrong, but with that realization I think I will simplify this patch to only add default key fallback to the navigation client case. Yup, going to go about this a little differently. Patch coming in the AM Looks very similar to what I encountered in bug 156553. Patch is done, but working on an API test which is... slow going. (In reply to comment #12) > Looks very similar to what I encountered in bug 156553. Yup, this is exactly that issue. *** Bug 156553 has been marked as a duplicate of this bug. *** After spending all morning piecing together a glorious API test, I re-discovered why this was never API-tested in the first place: The WebCore::getDefaultWebCryptoMasterKey implemented brings up the keychain dialog, which we can't have during automated testing. DANGIT! Will put together the patch without the test, then attach the test for posterity. (In reply to comment #16) > After spending all morning piecing together a glorious API test, I > re-discovered why this was never API-tested in the first place: > > The WebCore::getDefaultWebCryptoMasterKey implemented brings up the keychain > dialog, which we can't have during automated testing. > > DANGIT! > > Will put together the patch without the test, then attach the test for > posterity. Can we swizzle the keychain dialog out? (In reply to comment #17) > (In reply to comment #16) > > After spending all morning piecing together a glorious API test, I > > re-discovered why this was never API-tested in the first place: > > > > The WebCore::getDefaultWebCryptoMasterKey implemented brings up the keychain > > dialog, which we can't have during automated testing. > > > > DANGIT! > > > > Will put together the patch without the test, then attach the test for > > posterity. > > Can we swizzle the keychain dialog out? I doubt it? But will try for a little bit. The dialog comes from internals deep inside of SecItemCopyMatching. We could swizzle SecItemCopyMatching to return our own canned item, but then we'd run into further issues downstream (more swizzles required) I'm not sure if it's worth going down the rabbit hole or if - in the end - it will even be possible. Created attachment 290381 [details]
The currently unusable test
For posterity in case we can revisit later.
Created attachment 290382 [details]
Patch
Comment on attachment 290382 [details]
Patch
r=me
I'm surprised that this now goes through NavigationClient - WebCrypto has nothing to do with navigation. But that's a pre-existing issue, if an issue at all.
Thanks for fixing this issue so quickly. This might not be possible to answer, but is there any way of knowing if and when this fix will make it into an official Safari release? We have a product that is broken because of this and we're considering an alternative key store implementation to mitigate the issue. (In reply to comment #24)> > This might not be possible to answer, but is there any way of knowing if and > when this fix will make it into an official Safari release? The WebKit open source project is in no position to discuss the future plans for a vendor's product release. Created attachment 294873 [details]
New test case to highlight the issue with saving the CryptoKeyPair
There is a problem adding the CryptoKeyPair object into indexedDB. This testcase is a modification to the existing testcase that tries to save the object when generating a key pair.
This code works in Chrome and Firefox. Neither browser returns a CryptoKeyPair, and instead returns a plain object.
Jason, can you reproduce that in Safari Tech Preview? Could you please file a new bug about that if you can? (In reply to comment #27) > Jason, can you reproduce that in Safari Tech Preview? Could you please file > a new bug about that if you can? I have created tested this in Safari Tech Preview, and it still exists. I have added created a new bug as requested (https://bugs.webkit.org/show_bug.cgi?id=164868) (In reply to comment #28) > (In reply to comment #27) > > Jason, can you reproduce that in Safari Tech Preview? Could you please file > > a new bug about that if you can? > > I have created tested this in Safari Tech Preview, and it still exists. I > have added created a new bug as requested > (https://bugs.webkit.org/show_bug.cgi?id=164868) As mentioned over in 164868, the issue you found is one with our IDL bindings and serialized script values, and not really with IDB. It's been explored/fixed over there. |