Bug 162554 - REGRESSION (Safari 10 combined with WK changes): Unable to store WebCrypto keys in IndexedDB database
Summary: REGRESSION (Safari 10 combined with WK changes): Unable to store WebCrypto ke...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: Safari 10
Hardware: Unspecified macOS 10.12
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
: 156553 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-26 06:26 PDT by oyvind.pedersen
Modified: 2017-04-26 16:18 PDT (History)
9 users (show)

See Also:


Attachments
stand-alone html file that shows the problem (3.59 KB, text/html)
2016-09-26 06:26 PDT, oyvind.pedersen
no flags Details
Patch (4.59 KB, patch)
2016-09-29 16:45 PDT, Brady Eidson
ap: review-
Details | Formatted Diff | Diff
The currently unusable test (14.47 KB, patch)
2016-09-30 14:04 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2016-09-30 14:05 PDT, Brady Eidson
ap: review+
Details | Formatted Diff | Diff
New test case to highlight the issue with saving the CryptoKeyPair (3.64 KB, text/html)
2016-11-15 13:43 PST, Jason Mei
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description oyvind.pedersen 2016-09-26 06:26:25 PDT
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.
Comment 1 oyvind.pedersen 2016-09-26 06:41: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).
Comment 2 Alexey Proskuryakov 2016-09-26 09:32:22 PDT
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.
Comment 3 Radar WebKit Bug Importer 2016-09-26 09:32:34 PDT
<rdar://problem/28475450>
Comment 4 Brady Eidson 2016-09-26 15:13:16 PDT
Using old archive builds we've tracked this to https://trac.webkit.org/changeset/191810 (bizarrely)
Comment 5 Brady Eidson 2016-09-29 16:24:05 PDT
(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.
Comment 6 Brady Eidson 2016-09-29 16:25:38 PDT
(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)
Comment 7 Brady Eidson 2016-09-29 16:45:59 PDT
Created attachment 290264 [details]
Patch
Comment 8 Alexey Proskuryakov 2016-09-29 17:01:52 PDT
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.
Comment 9 Brady Eidson 2016-09-29 17:04:14 PDT
(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.
Comment 10 Brady Eidson 2016-09-29 17:20:30 PDT
(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.
Comment 11 Brady Eidson 2016-09-29 17:31:50 PDT
Yup, going to go about this a little differently. Patch coming in the AM
Comment 12 Trygve 2016-09-30 04:00:22 PDT
Looks very similar to what I encountered in bug 156553.
Comment 13 Brady Eidson 2016-09-30 11:19:27 PDT
Patch is done, but working on an API test which is... slow going.
Comment 14 Brady Eidson 2016-09-30 11:20:02 PDT
(In reply to comment #12)
> Looks very similar to what I encountered in bug 156553.

Yup, this is exactly that issue.
Comment 15 Brady Eidson 2016-09-30 11:20:17 PDT
*** Bug 156553 has been marked as a duplicate of this bug. ***
Comment 16 Brady Eidson 2016-09-30 12:15:25 PDT
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.
Comment 17 Sam Weinig 2016-09-30 13:15:48 PDT
(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?
Comment 18 Brady Eidson 2016-09-30 13:23:13 PDT
(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.
Comment 19 Brady Eidson 2016-09-30 13:57:42 PDT
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.
Comment 20 Brady Eidson 2016-09-30 14:04:28 PDT
Created attachment 290381 [details]
The currently unusable test

For posterity in case we can revisit later.
Comment 21 Brady Eidson 2016-09-30 14:05:56 PDT
Created attachment 290382 [details]
Patch
Comment 22 Alexey Proskuryakov 2016-09-30 15:09:11 PDT
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.
Comment 23 Brady Eidson 2016-09-30 15:41:52 PDT
https://trac.webkit.org/changeset/206684
Comment 24 Trygve 2016-10-05 00:33:20 PDT
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.
Comment 25 Brady Eidson 2016-10-05 09:23:43 PDT
(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.
Comment 26 Jason Mei 2016-11-15 13:43:28 PST
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.
Comment 27 Alexey Proskuryakov 2016-11-15 13:53:46 PST
Jason, can you reproduce that in Safari Tech Preview? Could you please file a new bug about that if you can?
Comment 28 Jason Mei 2016-11-17 08:11:29 PST
(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)
Comment 29 Brady Eidson 2016-11-17 14:06:30 PST
(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.