Bug 162554 – REGRESSION (Safari 10 combined with WK changes): Unable to store WebCrypto keys in IndexedDB database

• Component: WebKit API
• Status: RESOLVED FIXED

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).

[Alexey Proskuryakov]

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.

[Radar WebKit Bug Importer]

<rdar://problem/28475450>

[Brady Eidson]

Using old archive builds we've tracked this to https://trac.webkit.org/changeset/191810 (bizarrely)

[Brady Eidson]

(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.

[Brady Eidson]

(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)

[Brady Eidson]

Created attachment 290264 [details]
Patch

[Alexey Proskuryakov]

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.

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

Yup, going to go about this a little differently. Patch coming in the AM

[Trygve]

Looks very similar to what I encountered in bug 156553.

[Brady Eidson]

Patch is done, but working on an API test which is... slow going.

[Brady Eidson]

(In reply to comment #12)
> Looks very similar to what I encountered in bug 156553.

Yup, this is exactly that issue.

[Brady Eidson]

*** Bug 156553 has been marked as a duplicate of this bug. ***

[Brady Eidson]

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.

[Sam Weinig]

(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?

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 290381 [details]
The currently unusable test

For posterity in case we can revisit later.

[Brady Eidson]

Created attachment 290382 [details]
Patch

[Alexey Proskuryakov]

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.

[Brady Eidson]

https://trac.webkit.org/changeset/206684

[Trygve]

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.

[Brady Eidson]

(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.

[Jason Mei]

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.

[Alexey Proskuryakov]

Jason, can you reproduce that in Safari Tech Preview? Could you please file a new bug about that if you can?

[Jason Mei]

(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)

[Brady Eidson]

(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.