Partially undo r250811
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Oct 2019 04:15:29 +0000 (04:15 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Oct 2019 04:15:29 +0000 (04:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202715
<rdar://problem/56084287>

Reviewed by Chris Dumez.

Source/WebCore:

This patch changes the SerializedScriptValue to always wrap CryptoKey objects again.
CryptoKey objects could belong to an array or another object. In those cases, IDBObjectStore
cannot set the flag for the embedded Cryptokey objects. Neither can postMessage to unset
the flag. Therefore, there is no way to separate the serialization process into two and
this patch restores the old behaviour. However, the hardening part of r250811 is kept
and therefore the crash should still be prevented.

No new test, updated existing test

* Modules/indexeddb/IDBObjectStore.cpp:
(WebCore::IDBObjectStore::putOrAdd):
(WebCore::JSC::setIsWrappingRequiredForCryptoKey): Deleted.
* bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::dumpIfTerminal):
(WebCore::CloneDeserializer::readTerminal):
* crypto/CryptoKey.h:
(WebCore::CryptoKey::allows const):
(WebCore::CryptoKey::isWrappingRequired const): Deleted.
(WebCore::CryptoKey::setIsWrappingRequired): Deleted.
(): Deleted.
* dom/ScriptExecutionContext.h:

Tools:

* TestWebKitAPI/Tests/WebKit/navigation-client-default-crypto.html:
Modified to crash if SerializedScriptValue doesn't wrap CryptoKey objects.

LayoutTests:

Some rebaselines.

* crypto/workers/subtle/ec-postMessage-worker-expected.txt:
* crypto/workers/subtle/hrsa-postMessage-worker-expected.txt:
* crypto/workers/subtle/rsa-postMessage-worker-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250887 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/crypto/workers/subtle/ec-postMessage-worker-expected.txt
LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker-expected.txt
LayoutTests/crypto/workers/subtle/rsa-postMessage-worker-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp
Source/WebCore/bindings/js/SerializedScriptValue.cpp
Source/WebCore/crypto/CryptoKey.h
Source/WebCore/dom/ScriptExecutionContext.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit/navigation-client-default-crypto.html
Tools/TestWebKitAPI/Tests/WebKitCocoa/WebCryptoMasterKey.mm

index 610e34b..854fa09 100644 (file)
@@ -1,3 +1,17 @@
+2019-10-08  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Partially undo r250811
+        https://bugs.webkit.org/show_bug.cgi?id=202715
+        <rdar://problem/56084287>
+
+        Reviewed by Chris Dumez.
+
+        Some rebaselines.
+
+        * crypto/workers/subtle/ec-postMessage-worker-expected.txt:
+        * crypto/workers/subtle/hrsa-postMessage-worker-expected.txt:
+        * crypto/workers/subtle/rsa-postMessage-worker-expected.txt:
+
 2019-10-08  Justin Fan  <justin_fan@apple.com>
 
         WebGPU tests are skipped on iOS
index 2f9b3a0..75bc79e 100644 (file)
@@ -3,14 +3,6 @@ Test sending ec crypto keys via postMessage to a worker.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'ECDH'
-PASS publicKey.algorithm.namedCurve is 'P-256'
-PASS publicKey.usages is [ ]
-PASS exportedKey.x is publicKeyJSON.x
-PASS exportedKey.y is publicKeyJSON.y
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is true
@@ -20,6 +12,14 @@ PASS privateKey.usages is ['deriveBits']
 PASS exportedKey.x is privateKeyJSON.x
 PASS exportedKey.y is privateKeyJSON.y
 PASS exportedKey.d is privateKeyJSON.d
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'ECDH'
+PASS publicKey.algorithm.namedCurve is 'P-256'
+PASS publicKey.usages is [ ]
+PASS exportedKey.x is publicKeyJSON.x
+PASS exportedKey.y is publicKeyJSON.y
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 6e1d6fe..0ed74f9 100644 (file)
@@ -3,14 +3,6 @@ Test sending hashed rsa crypto keys via postMessage to a worker.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'RSASSA-PKCS1-v1_5'
-PASS publicKey.algorithm.modulusLength is 2048
-PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
-PASS publicKey.algorithm.hash.name is 'SHA-256'
-PASS publicKey.usages is ['verify']
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is false
@@ -19,6 +11,14 @@ PASS privateKey.algorithm.modulusLength is 2048
 PASS bytesToHexString(privateKey.algorithm.publicExponent) is '010001'
 PASS privateKey.algorithm.hash.name is 'SHA-256'
 PASS privateKey.usages is ['sign']
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'RSASSA-PKCS1-v1_5'
+PASS publicKey.algorithm.modulusLength is 2048
+PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
+PASS publicKey.algorithm.hash.name is 'SHA-256'
+PASS publicKey.usages is ['verify']
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 61921c4..0c39790 100644 (file)
@@ -3,16 +3,6 @@ Test sending rsa crypto keys via postMessage to a worker.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'RSAES-PKCS1-v1_5'
-PASS publicKey.algorithm.modulusLength is 2048
-PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
-PASS publicKey.algorithm.hash is undefined.
-PASS publicKey.usages is ['encrypt']
-PASS exportedKey.n is publicKeyJSON.n
-PASS exportedKey.e is publicKeyJSON.e
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is true
@@ -29,6 +19,16 @@ PASS exportedKey.q is privateKeyJSON.q
 PASS exportedKey.dp is privateKeyJSON.dp
 PASS exportedKey.dq is privateKeyJSON.dq
 PASS exportedKey.qi is privateKeyJSON.qi
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'RSAES-PKCS1-v1_5'
+PASS publicKey.algorithm.modulusLength is 2048
+PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
+PASS publicKey.algorithm.hash is undefined.
+PASS publicKey.usages is ['encrypt']
+PASS exportedKey.n is publicKeyJSON.n
+PASS exportedKey.e is publicKeyJSON.e
 PASS successfullyParsed is true
 
 TEST COMPLETE
index d7b50e9..709b182 100644 (file)
@@ -1,3 +1,33 @@
+2019-10-08  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Partially undo r250811
+        https://bugs.webkit.org/show_bug.cgi?id=202715
+        <rdar://problem/56084287>
+
+        Reviewed by Chris Dumez.
+
+        This patch changes the SerializedScriptValue to always wrap CryptoKey objects again.
+        CryptoKey objects could belong to an array or another object. In those cases, IDBObjectStore
+        cannot set the flag for the embedded Cryptokey objects. Neither can postMessage to unset
+        the flag. Therefore, there is no way to separate the serialization process into two and
+        this patch restores the old behaviour. However, the hardening part of r250811 is kept
+        and therefore the crash should still be prevented.
+
+        No new test, updated existing test
+
+        * Modules/indexeddb/IDBObjectStore.cpp:
+        (WebCore::IDBObjectStore::putOrAdd):
+        (WebCore::JSC::setIsWrappingRequiredForCryptoKey): Deleted.
+        * bindings/js/SerializedScriptValue.cpp:
+        (WebCore::CloneSerializer::dumpIfTerminal):
+        (WebCore::CloneDeserializer::readTerminal):
+        * crypto/CryptoKey.h:
+        (WebCore::CryptoKey::allows const):
+        (WebCore::CryptoKey::isWrappingRequired const): Deleted.
+        (WebCore::CryptoKey::setIsWrappingRequired): Deleted.
+        (): Deleted.
+        * dom/ScriptExecutionContext.h:
+
 2019-10-08  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Canvas: modifications to shader modules can be shared between vertex/fragment shaders
index 0bddef5..a597ac1 100644 (file)
 #include <JavaScriptCore/JSCJSValueInlines.h>
 #include <wtf/Locker.h>
 
-#if ENABLE(WEB_CRYPTO)
-#include "JSCryptoKey.h"
-#endif
-
 namespace WebCore {
 using namespace JSC;
 
-#if ENABLE(WEB_CRYPTO)
-namespace {
-static inline void setIsWrappingRequiredForCryptoKey(VM& vm, const JSValue& value)
-{
-    if (!value.isObject())
-        return;
-    auto* obj = asObject(value);
-    if (auto* key = JSCryptoKey::toWrapped(vm, obj))
-        key->setIsWrappingRequired();
-}
-}
-#endif
-
 IDBObjectStore::IDBObjectStore(ScriptExecutionContext& context, const IDBObjectStoreInfo& info, IDBTransaction& transaction)
     : ActiveDOMObject(&context)
     , m_info(info)
@@ -356,9 +339,6 @@ ExceptionOr<Ref<IDBRequest>> IDBObjectStore::putOrAdd(ExecState& state, JSValue
     if (m_transaction.isReadOnly())
         return Exception { ReadonlyError, "Failed to store record in an IDBObjectStore: The transaction is read-only."_s };
 
-#if ENABLE(WEB_CRYPTO)
-    setIsWrappingRequiredForCryptoKey(vm, value);
-#endif
     auto serializedValue = SerializedScriptValue::create(state, value);
     if (UNLIKELY(scope.exception()))
         return Exception { DataCloneError, "Failed to store record in an IDBObjectStore: An object could not be cloned."_s };
index d3436c6..3b000b8 100644 (file)
@@ -167,9 +167,6 @@ enum SerializationTag {
     RTCCertificateTag = 44,
 #endif
     ImageBitmapTag = 45,
-#if ENABLE(WEB_CRYPTO)
-    UnwrappedCryptoKeyTag = 46,
-#endif
     ErrorTag = 255
 };
 
@@ -353,7 +350,6 @@ static const unsigned StringDataIs8BitFlag = 0x80000000;
  *    | ImageBitmapTransferTag <value:uint32_t>
  *    | RTCCertificateTag
  *    | ImageBitmapTag <originClean:uint8_t> <logicalWidth:int32_t> <logicalHeight:int32_t> <resolutionScale:double> <byteLength:uint32_t>(<imageByteData:uint8_t>)
- *    | UnwrappedCryptoKeyTag <unwrappedKeyLength:uint32_t> <factor:byte{unwrappedKeyLength}>
  *
  * Inside certificate, data is serialized in this format as per spec:
  *
@@ -1087,10 +1083,7 @@ private:
             }
 #if ENABLE(WEB_CRYPTO)
             if (auto* key = JSCryptoKey::toWrapped(vm, obj)) {
-                if (key->isWrappingRequired())
-                    write(CryptoKeyTag);
-                else
-                    write(UnwrappedCryptoKeyTag);
+                write(CryptoKeyTag);
                 Vector<uint8_t> serializedKey;
                 Vector<String> dummyBlobURLs;
                 Vector<RefPtr<MessagePort>> dummyMessagePorts;
@@ -1105,14 +1098,10 @@ private:
 #endif
                     dummyBlobURLs, serializedKey, SerializationContext::Default, dummySharedBuffers);
                 rawKeySerializer.write(key);
-                if (key->isWrappingRequired()) {
-                    Vector<uint8_t> wrappedKey;
-                    if (!wrapCryptoKey(m_exec, serializedKey, wrappedKey))
-                        return false;
-                    write(wrappedKey);
-                    return true;
-                }
-                write(serializedKey);
+                Vector<uint8_t> wrappedKey;
+                if (!wrapCryptoKey(m_exec, serializedKey, wrappedKey))
+                    return false;
+                write(wrappedKey);
                 return true;
             }
 #endif
@@ -3103,28 +3092,6 @@ private:
 #endif
         case ImageBitmapTag:
             return readImageBitmap();
-#if ENABLE(WEB_CRYPTO)
-        case UnwrappedCryptoKeyTag: {
-            Vector<uint8_t> serializedKey;
-            if (!read(serializedKey)) {
-                fail();
-                return JSValue();
-            }
-            JSValue cryptoKey;
-            Vector<RefPtr<MessagePort>> dummyMessagePorts;
-            CloneDeserializer rawKeyDeserializer(m_exec, m_globalObject, dummyMessagePorts, nullptr, { },
-#if ENABLE(WEBASSEMBLY)
-                nullptr,
-#endif
-                serializedKey);
-            if (!rawKeyDeserializer.readCryptoKey(cryptoKey)) {
-                fail();
-                return JSValue();
-            }
-            m_gcBuffer.appendWithCrashOnOverflow(cryptoKey);
-            return cryptoKey;
-        }
-#endif
         default:
             m_ptr--; // Push the tag back
             return JSValue();
index 3865d29..0ebc572 100644 (file)
@@ -72,8 +72,6 @@ public:
     CryptoKeyUsageBitmap usagesBitmap() const { return m_usages; }
     void setUsagesBitmap(CryptoKeyUsageBitmap usage) { m_usages = usage; };
     bool allows(CryptoKeyUsageBitmap usage) const { return usage == (m_usages & usage); }
-    bool isWrappingRequired() const { return m_isWrappingRequired; }
-    void setIsWrappingRequired() { m_isWrappingRequired = true; }
 
     static Vector<uint8_t> randomData(size_t);
 
@@ -82,7 +80,6 @@ private:
     Type m_type;
     bool m_extractable;
     CryptoKeyUsageBitmap m_usages;
-    bool m_isWrappingRequired { false };
 };
 
 inline auto CryptoKey::type() const -> Type
index 0fbec54..397e18c 100644 (file)
@@ -221,7 +221,9 @@ public:
     void setDatabaseContext(DatabaseContext*);
 
 #if ENABLE(WEB_CRYPTO)
-    // These two methods are used when CryptoKeys are serialized into IndexedDB.
+    // These two methods are used when CryptoKeys are serialized into IndexedDB. As a side effect, it is also
+    // used for things that utilize the same structure clone algorithm, for example, message passing between
+    // worker and document.
     virtual bool wrapCryptoKey(const Vector<uint8_t>& key, Vector<uint8_t>& wrappedKey) = 0;
     virtual bool unwrapCryptoKey(const Vector<uint8_t>& wrappedKey, Vector<uint8_t>& key) = 0;
 #endif
index 72ca9dc..6e11e0d 100644 (file)
@@ -1,3 +1,14 @@
+2019-10-08  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Partially undo r250811
+        https://bugs.webkit.org/show_bug.cgi?id=202715
+        <rdar://problem/56084287>
+
+        Reviewed by Chris Dumez.
+
+        * TestWebKitAPI/Tests/WebKit/navigation-client-default-crypto.html:
+        Modified to crash if SerializedScriptValue doesn't wrap CryptoKey objects.
+
 2019-10-08  Jonathan Bedard  <jbedard@apple.com>
 
         REGRESSION (r250375): [old EWS] JSC EWS is always marking Patches as success
index fb77702..a496ff3 100644 (file)
@@ -55,7 +55,7 @@ crypto.subtle.importKey("jwk", privateKeyJSON, { name: 'RSASSA-PKCS1-v1_5', hash
     function storeKey() {
         var objectStore = db.transaction("rsa-indexeddb", "readwrite").objectStore("rsa-indexeddb");
         try {
-            var req = objectStore.put(key, "mykey");
+            var req = objectStore.put({ key: key }, "theKey");
             req.onerror = function(event) {
                 alert('failed to store key');
             }
@@ -70,12 +70,12 @@ crypto.subtle.importKey("jwk", privateKeyJSON, { name: 'RSASSA-PKCS1-v1_5', hash
 
     function readKey() {
         var objectStore = db.transaction("rsa-indexeddb").objectStore("rsa-indexeddb");
-        var req = objectStore.get("mykey");
+        var req = objectStore.get("theKey");
         req.onerror = function(event) {
             alert('failed to read key');
         }
         req.onsuccess = function(event) {
-            window.retrievedKey = event.target.result;
+            window.retrievedKey = event.target.result.key;
             if (retrievedKey.type != 'private')
                 return alert("retrievedKey.type != 'private': " + retrievedKey.type);
             if (retrievedKey.extractable != true)
index a55114b..90dfdda 100644 (file)
@@ -32,6 +32,7 @@
 
 static bool receivedMessage = false;
 static String gMessage;
+static bool masterKeyCalled = false;
 
 @interface WebCryptoMasterKeyNavigationDelegate : NSObject <WKNavigationDelegate, WKUIDelegate>
 @end
@@ -40,6 +41,7 @@ static String gMessage;
 
 - (NSData *)_webCryptoMasterKeyForWebView:(WKWebView *)webView
 {
+    masterKeyCalled = true;
     return nil;
 }
 
@@ -66,6 +68,7 @@ TEST(WebKit, WebCryptoNilMasterKey)
     [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     Util::run(&receivedMessage);
     EXPECT_WK_STREQ("DataCloneError", gMessage);
+    EXPECT_TRUE(masterKeyCalled);
 }
 
 } // namespace TestWebKitAPI