Bug 164738 - Update SubtleCrypto::encrypt to match the latest spec
Summary: Update SubtleCrypto::encrypt to match the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 160883
  Show dependency treegraph
 
Reported: 2016-11-14 15:49 PST by Jiewen Tan
Modified: 2016-11-18 13:31 PST (History)
8 users (show)

See Also:


Attachments
Patch (200.26 KB, patch)
2016-11-17 19:00 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (201.49 KB, patch)
2016-11-17 19:45 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (204.37 KB, patch)
2016-11-17 20:08 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (221.89 KB, patch)
2016-11-18 12:16 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2016-11-14 15:49:00 PST
Update SubtleCrypto::encrypt to match the latest spec.
Comment 1 Radar WebKit Bug Importer 2016-11-14 16:37:07 PST
<rdar://problem/29257812>
Comment 2 Jiewen Tan 2016-11-17 19:00:11 PST
Created attachment 295119 [details]
Patch
Comment 3 WebKit Commit Bot 2016-11-17 19:03:05 PST
Attachment 295119 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:71:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:41:  encryptRSA_OAEP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:56:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:78:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:41:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:77:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:44:  IV_SIZE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::encrypt 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:62:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:39:  encryptRSAES_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:71:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 12 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jiewen Tan 2016-11-17 19:45:39 PST
Created attachment 295128 [details]
Patch
Comment 5 WebKit Commit Bot 2016-11-17 19:48:30 PST
Attachment 295128 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:71:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:41:  encryptRSA_OAEP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:56:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:78:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:39:  encryptRSAES_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:71:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::encrypt 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:62:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:41:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:75:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 11 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Jiewen Tan 2016-11-17 20:08:33 PST
Created attachment 295129 [details]
Patch
Comment 7 WebKit Commit Bot 2016-11-17 20:09:55 PST
Attachment 295129 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:71:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:36:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:41:  encryptRSA_OAEP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:56:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:78:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:39:  encryptRSAES_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:71:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::encrypt 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:62:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:41:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:75:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:37:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:36:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 14 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Andy Estes 2016-11-18 10:46:19 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55
> +    Encrypt,

Should this go below Digest to keep the values sorted?

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:78
> +    workQueue.dispatch([capturedParameters = WTFMove(parameters), capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {

It's ok to reuse parameter names in your capture list. So this can be [parameters = WTFMove(parameters), key = WTFMove(key), ...].

> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:53
> +    workQueue.dispatch([capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {

Ditto.

> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:59
> +    workQueue.dispatch([capturedParameters = WTFMove(parameters), capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {

Ditto.
Comment 9 Brent Fulgham 2016-11-18 10:51:29 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

Looks good! I had a few nitpicks before we land this.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:432
> +    auto exceptionCallback = [capturedPromise =  promise.copyRef()](ExceptionCode ec) mutable {

Please remove the extra space between "capturedPromise =" and "promise.copyRef()"

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
> +    auto& aesParameters = downcast<CryptoAlgorithmAesCbcParams>(*parameters);

Is parameters guaranteed to be non-null? If so, why isn't it passed as a reference? If it can be nullptr, please check (or at least ASSERT).

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:73
> +        exceptionCallback(OperationError);

Is 'OperationError' from the specification? I wonder if this should be "DataError" or "TypeError" instead?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:64
> +    if (key->type() != CryptoKeyType::Public) {

If key cannot be nullptr, it seems like a better API would be to pass Ref<CryptoKey>. Otherwise, we need a nullptr check.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:73
> +    if (key->type() != CryptoKeyType::Public) {

Ditto nullptr check (or use Ref<>).

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:40
> +// FIXME: We should change iv and data to Vector<uint8_t> type once WebKitSubtleCrypto is deprecated.

I suggest you supply a Bugzilla bug here.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:77
> +    context->ref();

Seems like context should be passed as reference.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:100
> +    assert(sizeof(parameters.iv) == kCCBlockSizeAES128);

Why is this 'assert' instead of our "ASSERT"?

> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:44
> +    if (status)

This could be combined with the line above:

if (CCCryptorStatus status = ....)
  // Do error handling

> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:52
> +    context->ref();

nullptr check or pass by reference?

> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:82
> +// FIXME: We should get rid of the magic number 1024. It only makes sense when key length < 8192 bits

Bugzilla please, or else we will never remember to do this!

> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:40
> +// FIXME: We should change data to Vector<uint8_t> type once WebKitSubtleCrypto is deprecated.

Bugzilla please.

> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:50
> +    if (status)

Suggest you combine with line 49 into one statement.

> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:58
> +    context->ref();

Ditto nullptr check or reference.

> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:60
> +        ASSERT(context);

Yikes! We just dereferenced it above!

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:40
> +// FIXME: We should get rid of magic number 16384. It assumes that the length of provided key will not exceed 16KB.

Ditto Bugzilla

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:234
> +// FIXME: We should use WorkQueue here instead of dispatch_async once WebKitSubtleCrypto is deprecated.

Ditto Bugzilla.
Comment 10 Jiewen Tan 2016-11-18 11:02:59 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

Thanks, Andy.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55
>> +    Encrypt,
> 
> Should this go below Digest to keep the values sorted?

The order I try to follow is that from https://w3c.github.io/webcrypto/Overview.html#subtlecrypto-interface. It makes more sense than alphabetic that it groups all related operations together.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:78
>> +    workQueue.dispatch([capturedParameters = WTFMove(parameters), capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {
> 
> It's ok to reuse parameter names in your capture list. So this can be [parameters = WTFMove(parameters), key = WTFMove(key), ...].

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:53
>> +    workQueue.dispatch([capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {
> 
> Ditto.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:59
>> +    workQueue.dispatch([capturedParameters = WTFMove(parameters), capturedKey = WTFMove(key), capturedPlainText = WTFMove(plainText), capturedCallback = WTFMove(callback), capturedExceptionCallback = WTFMove(exceptionCallback), context]() mutable {
> 
> Ditto.

Fixed.
Comment 11 Jiewen Tan 2016-11-18 11:59:51 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

Thanks, Brent.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:432
>> +    auto exceptionCallback = [capturedPromise =  promise.copyRef()](ExceptionCode ec) mutable {
> 
> Please remove the extra space between "capturedPromise =" and "promise.copyRef()"

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
>> +    auto& aesParameters = downcast<CryptoAlgorithmAesCbcParams>(*parameters);
> 
> Is parameters guaranteed to be non-null? If so, why isn't it passed as a reference? If it can be nullptr, please check (or at least ASSERT).

Yes, it is guaranteed. I use pointer here because I want polymorphism. Added ASSERT.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:73
>> +        exceptionCallback(OperationError);
> 
> Is 'OperationError' from the specification? I wonder if this should be "DataError" or "TypeError" instead?

It is from specification. https://w3c.github.io/webcrypto/Overview.html#aes-cbc-operations Encrypt Step 1.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:64
>> +    if (key->type() != CryptoKeyType::Public) {
> 
> If key cannot be nullptr, it seems like a better API would be to pass Ref<CryptoKey>. Otherwise, we need a nullptr check.

Fixed. Used Ref<CryptoKey>.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:73
>> +    if (key->type() != CryptoKeyType::Public) {
> 
> Ditto nullptr check (or use Ref<>).

Ditto.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:40
>> +// FIXME: We should change iv and data to Vector<uint8_t> type once WebKitSubtleCrypto is deprecated.
> 
> I suggest you supply a Bugzilla bug here.

Added. https://bugs.webkit.org/show_bug.cgi?id=164939.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:77
>> +    context->ref();
> 
> Seems like context should be passed as reference.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:100
>> +    assert(sizeof(parameters.iv) == kCCBlockSizeAES128);
> 
> Why is this 'assert' instead of our "ASSERT"?

Just a typo. Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:44
>> +    if (status)
> 
> This could be combined with the line above:
> 
> if (CCCryptorStatus status = ....)
>   // Do error handling

Probably can't. We don't use status afterward. Use if (CCRSACryptorEncrypt(key, ccPKCS1Padding, data, dataLength, cipherText.data(), &cipherTextLength, 0, 0, kCCDigestNone)) instead.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:52
>> +    context->ref();
> 
> nullptr check or pass by reference?

Fixed. By reference.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:82
>> +// FIXME: We should get rid of the magic number 1024. It only makes sense when key length < 8192 bits
> 
> Bugzilla please, or else we will never remember to do this!

Don't worry. I will fix this in my next decrypt patch.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:40
>> +// FIXME: We should change data to Vector<uint8_t> type once WebKitSubtleCrypto is deprecated.
> 
> Bugzilla please.

https://bugs.webkit.org/show_bug.cgi?id=164939

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:50
>> +    if (status)
> 
> Suggest you combine with line 49 into one statement.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:58
>> +    context->ref();
> 
> Ditto nullptr check or reference.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:60
>> +        ASSERT(context);
> 
> Yikes! We just dereferenced it above!

Where?

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:40
>> +// FIXME: We should get rid of magic number 16384. It assumes that the length of provided key will not exceed 16KB.
> 
> Ditto Bugzilla

https://bugs.webkit.org/show_bug.cgi?id=164942

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:234
>> +// FIXME: We should use WorkQueue here instead of dispatch_async once WebKitSubtleCrypto is deprecated.
> 
> Ditto Bugzilla.

https://bugs.webkit.org/show_bug.cgi?id=164943
Comment 12 Jiewen Tan 2016-11-18 12:16:50 PST
Created attachment 295176 [details]
Patch
Comment 13 WebKit Commit Bot 2016-11-18 12:19:10 PST
Attachment 295176 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:71:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:173:  CryptoAlgorithmRSA_OAEP::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:36:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:42:  encryptRSA_OAEP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:56:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:77:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:40:  encryptRSAES_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:70:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:139:  CryptoAlgorithmAES_CBC::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:122:  CryptoAlgorithmAES_KW::exportKey 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:151:  CryptoAlgorithmRSASSA_PKCS1_v1_5::exportKey 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:62:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt 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:127:  CryptoAlgorithmRSAES_PKCS1_v1_5::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:42:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:76:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:37:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:36:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 19 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Brent Fulgham 2016-11-18 13:07:50 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

>>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:60
>>> +        ASSERT(context);
>> 
>> Yikes! We just dereferenced it above!
> 
> Where?

We dereference it on line 58, then pass it to the lambda.

ASSERTING here is okay, but if it might be nullptr we should check before entering the lambda.
Comment 15 Brent Fulgham 2016-11-18 13:09:19 PST
Comment on attachment 295176 [details]
Patch

Looks great! r=me.
Comment 16 Jiewen Tan 2016-11-18 13:13:26 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

Thanks Brent for r+ my patch.

>>>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:60
>>>> +        ASSERT(context);
>>> 
>>> Yikes! We just dereferenced it above!
>> 
>> Where?
> 
> We dereference it on line 58, then pass it to the lambda.
> 
> ASSERTING here is okay, but if it might be nullptr we should check before entering the lambda.

I think that's ref not deref.
Comment 17 Brent Fulgham 2016-11-18 13:24:58 PST
Comment on attachment 295129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295129&action=review

>>>>> Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:60
>>>>> +        ASSERT(context);
>>>> 
>>>> Yikes! We just dereferenced it above!
>>> 
>>> Where?
>> 
>> We dereference it on line 58, then pass it to the lambda.
>> 
>> ASSERTING here is okay, but if it might be nullptr we should check before entering the lambda.
> 
> I think that's ref not deref.

lol. When it was a pointer, you were calling the "ref()" function through it without null checking. Now that it's a Ref<>, not a RefPtr<> it's clear that this is safe.
Comment 18 Jiewen Tan 2016-11-18 13:31:56 PST
Committed r208891: <http://trac.webkit.org/changeset/208891>