Bug 159189

Um, I'm gonna try to land my patch in bug #155073 to turn off subtle crypto, which I had forgotten about. That will make this less serious. We shouldn't have experimental features enabled unless someone is actually working on them.

FYI, I could not reproduce the crash on Mac.

I'm more and more concerned about this trend of disabling features when something fails instead of fixing the bug. Why is this so risky when subtle crypto is only enable on developer builds?

I think the existing code is incorrect since encryptFailureCallback() is called in exportSuccessCallback even after this encryptFailureCallback is WTFMove()-ed. I think there are double free bug; encryptFailureCallback may be called twice if platformEncrypt is not implemented.

Created attachment 282788 [details] Patch

Created attachment 282790 [details] Patch

Attachment 282790 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:50: CryptoAlgorithmRSA_OAEP::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:49: CryptoAlgorithmAES_CBC::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:48: CryptoAlgorithmAES_KW::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:50: CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:49: CryptoAlgorithmRSAES_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/CryptoAlgorithmRegistry.h:53: 'create' is incorrectly named. It should be named 'protector' or 'protectedCryptoAlgorithmIdentifier'. [readability/naming/protected] [4] Total errors found: 6 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 282790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282790&action=review Thanks! Patch looks to me in general, but I'm not familiar with the promises callback part, so I can't really review that part. > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:149 > return WTFMove(result); I think move is not really needed in return, right? Maybe we could take advantage of this patch to also remove all the return WTFMove() here.

Comment on attachment 282790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282790&action=review Thanks :D >> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:149 >> return WTFMove(result); > > I think move is not really needed in return, right? Maybe we could take advantage of this patch to also remove all the return WTFMove() here. Right! I'll fix this.

Created attachment 283003 [details] Patch

Attachment 283003 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:50: CryptoAlgorithmRSA_OAEP::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:49: CryptoAlgorithmAES_CBC::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:48: CryptoAlgorithmAES_KW::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:50: CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:49: CryptoAlgorithmRSAES_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/CryptoAlgorithmRegistry.h:53: 'create' is incorrectly named. It should be named 'protector' or 'protectedCryptoAlgorithmIdentifier'. [readability/naming/protected] [4] Total errors found: 6 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 283003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283003&action=review Added comment. > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:149 > return WTFMove(result); `return WTFMove(...)` is still efficient if the type of the `result` is not the same to the type of the function's result_type. (implicit type conversion). In the above code, the result is `RefPtr<CryptoAlgorithmParameters>` (since it may return the nullptr), while the type of `result` is `Ref<CryptoAlgorithmParameters>`. In such case, still WTFMove is necessary to invoke the move constructor.

Created attachment 283011 [details] Patch

Attachment 283011 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:50: CryptoAlgorithmRSA_OAEP::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:49: CryptoAlgorithmAES_CBC::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:48: CryptoAlgorithmAES_KW::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:50: CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:49: CryptoAlgorithmRSAES_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/CryptoAlgorithmRegistry.h:53: 'create' is incorrectly named. It should be named 'protector' or 'protectedCryptoAlgorithmIdentifier'. [readability/naming/protected] [4] Total errors found: 6 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 283011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283011&action=review Nice patch! > Source/WebCore/ChangeLog:11 > + In SubtleCrypto code, we have the rule that we must not call failureCallback when the error code is filled in synchronous execution. Is this documented somewhere in the code? It seems quite tricky. > Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h:56 > + bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const override; This doesn't look right. Surely the original code should have been passing a CryptoAlgorithm*, since there is no ownership transfer. I can't think of a reason to pass a std::unique_ptr or a RefPtr<CryptoAlgorithm> by reference. It'd be good to take this opportunity to fix it to take a CryptoAlogrithm*. > Source/WebCore/crypto/CryptoKeySerialization.h:50 > + virtual bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const = 0; Should take a CryptoAlgorithm*. > Source/WebCore/crypto/keys/CryptoKeySerializationRaw.h:54 > + bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const override; Ditto.

Comment on attachment 283011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283011&action=review Thanks! >> Source/WebCore/ChangeLog:11 >> + In SubtleCrypto code, we have the rule that we must not call failureCallback when the error code is filled in synchronous execution. > > Is this documented somewhere in the code? It seems quite tricky. No, this is the observation based on the mac code. If we call the failureCallback even if we set the exception code, it causes the double free in the ToT code. And mac port implementation avoids this situation by following the above principle in subtle crypto: "In SubtleCrypto code, we have the rule that we must not call failureCallback when the error code is filled in synchronous execution.". This patch also offers robustness. Even if we don't follow the above principle, after this patch, it does not cause the crash. >> Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h:56 >> + bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const override; > > This doesn't look right. Surely the original code should have been passing a CryptoAlgorithm*, since there is no ownership transfer. I can't think of a reason to pass a std::unique_ptr or a RefPtr<CryptoAlgorithm> by reference. It'd be good to take this opportunity to fix it to take a CryptoAlogrithm*. There is ownership transfer because reconcileAlgorithm is somewhat a factory method. It populates CryptoAlgorithm and CryptoAlgorithmParametes pair. But, it seems that, returning Option<struct { algorithm, parameters }> would be nice. Changed. >> Source/WebCore/crypto/CryptoKeySerialization.h:50 >> + virtual bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const = 0; > > Should take a CryptoAlgorithm*. Ditto. >> Source/WebCore/crypto/keys/CryptoKeySerializationRaw.h:54 >> + bool reconcileAlgorithm(RefPtr<CryptoAlgorithm>&, RefPtr<CryptoAlgorithmParameters>&) const override; > > Ditto. Ditto.

Created attachment 283280 [details] Patch for landing

Attachment 283280 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:50: CryptoAlgorithmRSA_OAEP::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:49: CryptoAlgorithmAES_CBC::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:48: CryptoAlgorithmAES_KW::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:50: CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:49: CryptoAlgorithmRSAES_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/CryptoAlgorithmRegistry.h:53: 'create' is incorrectly named. It should be named 'protector' or 'protectedCryptoAlgorithmIdentifier'. [readability/naming/protected] [4] Total errors found: 6 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 283281 [details] Patch for landing

Attachment 283281 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:50: CryptoAlgorithmRSA_OAEP::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:49: CryptoAlgorithmAES_CBC::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:48: CryptoAlgorithmAES_KW::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:50: CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:49: CryptoAlgorithmRSAES_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/CryptoAlgorithmRegistry.h:53: 'create' is incorrectly named. It should be named 'protector' or 'protectedCryptoAlgorithmIdentifier'. [readability/naming/protected] [4] Total errors found: 6 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 283280 [details] Patch for landing Attachment 283280 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1656506 New failing tests: crypto/subtle/unwrapKey-check-usage.html crypto/subtle/aes-kw-wrap-unwrap-aes.html crypto/subtle/aes-cbc-wrap-rsa-non-extractable.html crypto/subtle/postMessage-worker.html crypto/subtle/jwk-export-use-values.html crypto/subtle/aes-cbc-encrypt-decrypt-with-padding.html crypto/subtle/aes-cbc-unwrap-rsa.html crypto/subtle/aes-cbc-unwrap-failure.html crypto/subtle/hmac-sign-verify.html crypto/subtle/aes-cbc-wrong-key-class.html crypto/subtle/rsa-oaep-wrap-unwrap-aes.html crypto/subtle/aes-kw-key-manipulation.html crypto/subtle/hmac-postMessage.html crypto/subtle/wrapKey-check-usage.html crypto/subtle/aes-cbc-invalid-length.html crypto/subtle/aes-cbc-encrypt-decrypt.html crypto/subtle/aes-cbc-192-encrypt-decrypt.html crypto/subtle/aes-postMessage.html crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html crypto/subtle/hmac-sign-verify-empty-key.html crypto/subtle/aes-cbc-wrap-rsa.html crypto/subtle/hmac-check-algorithm.html crypto/subtle/aes-cbc-256-encrypt-decrypt.html

Created attachment 283282 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 283280 [details] Patch for landing Attachment 283280 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1656507 New failing tests: crypto/subtle/unwrapKey-check-usage.html crypto/subtle/aes-kw-wrap-unwrap-aes.html crypto/subtle/aes-cbc-wrap-rsa-non-extractable.html crypto/subtle/postMessage-worker.html crypto/subtle/jwk-export-use-values.html crypto/subtle/aes-cbc-encrypt-decrypt-with-padding.html crypto/subtle/aes-cbc-unwrap-rsa.html crypto/subtle/aes-cbc-unwrap-failure.html crypto/subtle/hmac-sign-verify.html crypto/subtle/aes-cbc-wrong-key-class.html crypto/subtle/rsa-oaep-wrap-unwrap-aes.html crypto/subtle/aes-kw-key-manipulation.html crypto/subtle/hmac-postMessage.html crypto/subtle/wrapKey-check-usage.html crypto/subtle/aes-cbc-invalid-length.html crypto/subtle/aes-cbc-encrypt-decrypt.html crypto/subtle/aes-cbc-192-encrypt-decrypt.html crypto/subtle/aes-postMessage.html crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html crypto/subtle/hmac-sign-verify-empty-key.html crypto/subtle/aes-cbc-wrap-rsa.html crypto/subtle/hmac-check-algorithm.html crypto/subtle/aes-cbc-256-encrypt-decrypt.html

Created attachment 283283 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 283280 [details] Patch for landing Attachment 283280 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1656498 New failing tests: crypto/subtle/unwrapKey-check-usage.html crypto/subtle/aes-kw-wrap-unwrap-aes.html crypto/subtle/aes-cbc-wrap-rsa-non-extractable.html crypto/subtle/postMessage-worker.html crypto/subtle/jwk-export-use-values.html crypto/subtle/aes-cbc-encrypt-decrypt-with-padding.html crypto/subtle/aes-cbc-unwrap-rsa.html crypto/subtle/aes-cbc-unwrap-failure.html crypto/subtle/hmac-sign-verify.html crypto/subtle/aes-cbc-wrong-key-class.html crypto/subtle/rsa-oaep-wrap-unwrap-aes.html crypto/subtle/aes-kw-key-manipulation.html crypto/subtle/hmac-postMessage.html crypto/subtle/wrapKey-check-usage.html crypto/subtle/aes-cbc-invalid-length.html crypto/subtle/aes-cbc-encrypt-decrypt.html crypto/subtle/aes-cbc-192-encrypt-decrypt.html crypto/subtle/aes-postMessage.html crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html crypto/subtle/hmac-sign-verify-empty-key.html crypto/subtle/aes-cbc-wrap-rsa.html crypto/subtle/hmac-check-algorithm.html crypto/subtle/aes-cbc-256-encrypt-decrypt.html

Created attachment 283284 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 283280 [details] Patch for landing Attachment 283280 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1656509 New failing tests: crypto/subtle/unwrapKey-check-usage.html crypto/subtle/aes-kw-wrap-unwrap-aes.html crypto/subtle/aes-cbc-wrap-rsa-non-extractable.html crypto/subtle/postMessage-worker.html crypto/subtle/jwk-export-use-values.html crypto/subtle/aes-cbc-encrypt-decrypt-with-padding.html crypto/subtle/aes-cbc-unwrap-rsa.html crypto/subtle/aes-cbc-unwrap-failure.html crypto/subtle/hmac-sign-verify.html crypto/subtle/aes-cbc-wrong-key-class.html crypto/subtle/rsa-oaep-wrap-unwrap-aes.html crypto/subtle/aes-kw-key-manipulation.html crypto/subtle/hmac-postMessage.html crypto/subtle/wrapKey-check-usage.html crypto/subtle/aes-cbc-invalid-length.html crypto/subtle/aes-cbc-encrypt-decrypt.html crypto/subtle/aes-cbc-192-encrypt-decrypt.html crypto/subtle/aes-postMessage.html crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html crypto/subtle/hmac-sign-verify-empty-key.html crypto/subtle/aes-cbc-wrap-rsa.html crypto/subtle/hmac-check-algorithm.html crypto/subtle/aes-cbc-256-encrypt-decrypt.html

Created attachment 283285 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5

Committed r203036: <http://trac.webkit.org/changeset/203036>