Bug 169761

Summary: [WebCrypto] Add support for AES-CTR
Product: WebKit Reporter: Ivo Pletikosic <ipletikosic>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, jiewen_tan, ossy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 166746    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch bfulgham: review+, commit-queue: commit-queue-

Description Ivo Pletikosic 2017-03-16 10:21:44 PDT
Add support for AES-CTR as requested by Pandora Media: https://www.w3.org/TR/WebCryptoAPI/#aes-ctr
Comment 1 Radar WebKit Bug Importer 2017-03-29 14:53:31 PDT
<rdar://problem/31331321>
Comment 2 Jiewen Tan 2017-03-31 17:59:34 PDT
Created attachment 306032 [details]
Patch
Comment 3 Jiewen Tan 2017-04-03 16:17:52 PDT
Created attachment 306137 [details]
Patch
Comment 4 Jiewen Tan 2017-04-03 16:24:15 PDT
Created attachment 306139 [details]
Patch
Comment 5 Jiewen Tan 2017-04-03 16:26:03 PDT
(In reply to Jiewen Tan from comment #4)
> Created attachment 306139 [details]
> Patch

This patch is large because of rebaselines of w3c tests.
Comment 6 Build Bot 2017-04-03 16:31:29 PDT
Attachment 306139 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:43:  usagesAreInvalidForCryptoAlgorithmAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:57:  CryptoAlgorithmAES_CTR::create is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:62:  CryptoAlgorithmAES_CTR::identifier is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:67:  CryptoAlgorithmAES_CTR::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:78:  CryptoAlgorithmAES_CTR::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:89:  CryptoAlgorithmAES_CTR::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:107:  CryptoAlgorithmAES_CTR::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:147:  CryptoAlgorithmAES_CTR::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:187:  CryptoAlgorithmAES_CTR::getKeyLength is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:36:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:41:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:50:  transformAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:144:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:168:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 14 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Jiewen Tan 2017-04-03 17:50:32 PDT
Created attachment 306148 [details]
Patch
Comment 8 Build Bot 2017-04-03 17:57:26 PDT
Attachment 306148 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:43:  usagesAreInvalidForCryptoAlgorithmAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:57:  CryptoAlgorithmAES_CTR::create is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:62:  CryptoAlgorithmAES_CTR::identifier is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:67:  CryptoAlgorithmAES_CTR::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:78:  CryptoAlgorithmAES_CTR::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:89:  CryptoAlgorithmAES_CTR::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:107:  CryptoAlgorithmAES_CTR::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:147:  CryptoAlgorithmAES_CTR::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:187:  CryptoAlgorithmAES_CTR::getKeyLength is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:36:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:41:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:50:  transformAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:144:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:168:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 14 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brent Fulgham 2017-04-05 17:30:34 PDT
Comment on attachment 306148 [details]
Patch

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

I think this looks good, but I'd like you to clean up a few things before we land.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:48
> +static bool parametersAreValid(CryptoAlgorithmAesCtrParams& paramters)

paramters -> parameters (missing an 'e').

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:40
> +static size_t bigIntegerToSize(const Vector<uint8_t>& bigInteger)

I think you should call this bigIntegerToSizeT.

Also, do none of the "clampTo"

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:55
> +    // by the counterLength. It then increments the nonce which should stay same for the whole operatoin. To remedy this issue,

operatoin -> operation.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:66
> +        // Set Nonce to 1s

This comment makes it sound like you are setting the Nonce to some set of 1s:  "111111111111". But it's not clear what you're doing here.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:107
> +    Vector<uint8_t> resetedCounter(counter.size());

reseted is a weird word. I suggest "remainingCounter" or something like that.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:109
> +    size_t nonceOffset = counter.size() - counterLength / 8 - !!counterOffset;

!!counterOffset is overly clever. Is that to give you either 0 or 1?

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:135
> +    ASSERT(p <= tail.end());

ASSERT_WITH_SECURITY_IMPLICATION

> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/pbkdf2.worker-expected.txt:15
> +PASS Derived key of type name: AES-CTR length: 128  using short password, short salt, SHA-384, with 1 iterations 

Yay! More passes!
Comment 10 Jiewen Tan 2017-04-05 17:55:35 PDT
Comment on attachment 306148 [details]
Patch

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

Thanks Brent for reviewing my patch.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:48
>> +static bool parametersAreValid(CryptoAlgorithmAesCtrParams& paramters)
> 
> paramters -> parameters (missing an 'e').

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:40
>> +static size_t bigIntegerToSize(const Vector<uint8_t>& bigInteger)
> 
> I think you should call this bigIntegerToSizeT.
> 
> Also, do none of the "clampTo"

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:55
>> +    // by the counterLength. It then increments the nonce which should stay same for the whole operatoin. To remedy this issue,
> 
> operatoin -> operation.

Fixed.

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:63
> +    size_t rightMost = bigIntegerToSize(counter);

Comment added:
// convert right most __WORDSIZE bits into a size_t value, which is the longest counter we could possibly use.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:66
>> +        // Set Nonce to 1s
> 
> This comment makes it sound like you are setting the Nonce to some set of 1s:  "111111111111". But it's not clear what you're doing here.

The comment is updated to:
// Used to set nonce in rightMost(nonce + counter) to 1s

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:107
>> +    Vector<uint8_t> resetedCounter(counter.size());
> 
> reseted is a weird word. I suggest "remainingCounter" or something like that.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:109
>> +    size_t nonceOffset = counter.size() - counterLength / 8 - !!counterOffset;
> 
> !!counterOffset is overly clever. Is that to give you either 0 or 1?

Yes, that's my purpose. Any other suggestions?

>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:135
>> +    ASSERT(p <= tail.end());
> 
> ASSERT_WITH_SECURITY_IMPLICATION

Fixed.
Comment 11 Jiewen Tan 2017-04-05 18:05:16 PDT
Created attachment 306349 [details]
Patch
Comment 12 Build Bot 2017-04-05 18:10:45 PDT
Attachment 306349 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:43:  usagesAreInvalidForCryptoAlgorithmAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:57:  CryptoAlgorithmAES_CTR::create is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:62:  CryptoAlgorithmAES_CTR::identifier is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:67:  CryptoAlgorithmAES_CTR::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:78:  CryptoAlgorithmAES_CTR::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:89:  CryptoAlgorithmAES_CTR::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:107:  CryptoAlgorithmAES_CTR::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:147:  CryptoAlgorithmAES_CTR::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:187:  CryptoAlgorithmAES_CTR::getKeyLength is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:36:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CTRGCrypt.cpp:41:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:50:  transformAES_CTR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:143:  CryptoAlgorithmAES_CTR::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:167:  CryptoAlgorithmAES_CTR::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 14 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Brent Fulgham 2017-04-05 21:20:04 PDT
Comment on attachment 306349 [details]
Patch

Looks good! R=me
Comment 14 Jiewen Tan 2017-04-06 11:04:16 PDT
Comment on attachment 306349 [details]
Patch

Thanks Brent for r+ my patch.
Comment 15 WebKit Commit Bot 2017-04-06 11:33:58 PDT
Comment on attachment 306349 [details]
Patch

Rejecting attachment 306349 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 306349, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   42dd4f7..c5190fd  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 215043 = 42dd4f7308880730ce7f117bd47c606298384771
r215044 = c5190fdc1fead1177c5b2e15c9e53254eeb0ff22
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/3485692
Comment 16 Jiewen Tan 2017-04-06 11:56:44 PDT
Committed r215051: <http://trac.webkit.org/changeset/215051>
Comment 17 Csaba Osztrogon√°c 2017-04-08 07:20:31 PDT
(In reply to Jiewen Tan from comment #16)
> Committed r215051: <http://trac.webkit.org/changeset/215051>

And the Apple Mac cmake buildfix landed in
https://trac.webkit.org/changeset/215142/webkit