Bug 169761 – [WebCrypto] Add support for AES-CTR

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ivo Pletikosic]

Add support for AES-CTR as requested by Pandora Media: https://www.w3.org/TR/WebCryptoAPI/#aes-ctr

[Radar WebKit Bug Importer]

<rdar://problem/31331321>

[Jiewen Tan]

Created attachment 306032 [details]
Patch

[Jiewen Tan]

Created attachment 306137 [details]
Patch

[Jiewen Tan]

Created attachment 306139 [details]
Patch

[Jiewen Tan]

(In reply to Jiewen Tan from comment #4)
> Created attachment 306139 [details]
> Patch

This patch is large because of rebaselines of w3c tests.

[Build Bot]

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.

[Jiewen Tan]

Created attachment 306148 [details]
Patch

[Build Bot]

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.

[Brent Fulgham]

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!

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 306349 [details]
Patch

[Build Bot]

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.

[Brent Fulgham]

Comment on attachment 306349 [details]
Patch

Looks good! R=me

[Jiewen Tan]

Comment on attachment 306349 [details]
Patch

Thanks Brent for r+ my patch.

[WebKit Commit Bot]

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

[Jiewen Tan]

Committed r215051: <http://trac.webkit.org/changeset/215051>

[Csaba Osztrogonác]

(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