Bug 159189 – [GTK] Crash on https://diafygi.github.io/webcrypto-examples with ENABLE_SUBTLE_CRYPTO

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Carlos Alberto Lopez Perez]

When building with -DENABLE_SUBTLE_CRYPTO=ON (default for developer builds), the webprocess crashes when trying to load the following test page for WebCrypto API with the minibrowser:


https://diafygi.github.io/webcrypto-examples/


The backtrace (on r202483): http://sprunge.us/SdGc

[Michael Catanzaro]

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.

[Chris Dumez]

FYI, I could not reproduce the crash on Mac.

[Carlos Garcia Campos]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 282788 [details]
Patch

[Yusuke Suzuki]

Created attachment 282790 [details]
Patch

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 283003 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 283011 [details]
Patch

[WebKit Commit Bot]

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.

[Michael Catanzaro]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 283280 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 283281 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

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