Created attachment 289823 [details] stand-alone html file that shows the problem In Safari 10 (at least on Sierra) I am unable to store an RSA KeyPair created by the WebCrypto api in an IndexedDB database. The indexedDB put operation results in a DataCloneError. This works in Safari 9 and Chrome on El Capitan, and on Chrome on Windows. It seems that the CryptoKey object is no longer cloneable in Safari 10. I have also verified that symmetric keys (AES) suffer from the same problem. I have attached a html file which shows the bug. It alerts "works!" on all the above platforms except on Safari 10, where it shows a DataCloneError instead. On Safari 9, indexedDB cannot be accessed from a file url, so it has to be run from a web server instead, e.g. https://api-op.developer.ensafer.no/client/app/RSAIndexedDBtest.html.
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.
<rdar://problem/28475450>
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.
https://trac.webkit.org/changeset/206684
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.