Bug 207238

Summary: [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength longer than 64
Product: WebKit Reporter: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Component: WebCore Misc.Assignee: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, jiewen_tan, tomoki.imai, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=207375
Attachments:
Description Flags
Patch (test only)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Yoshiaki Jitsukawa 2020-02-04 16:36:34 PST 
Created attachment 389739 [details]
Patch (test only)

Add tests for AES-CTR counter overlap.
Comment 1 Jiewen Tan 2020-02-06 12:18:25 PST 
(In reply to Yoshiaki Jitsukawa from comment #0)
> Created attachment 389739 [details]
> Patch (test only)
> 
> Add tests for AES-CTR counter overlap.

Could you have a new test and then mark it fail for Apple ports? We don't support counter size longer than 64 bits at this moment.
Comment 2 Jiewen Tan 2020-02-06 12:29:17 PST 
Comment on attachment 389739 [details]
Patch (test only)

r- because of that. Also, if OpenSSL has no issues about all these test cases, you probably don't need them at all.
Comment 3 Yoshiaki Jitsukawa 2020-02-06 16:41:30 PST 
Hi Jiewen,

As you know (thanks for the comment on bug 207176), we're adding AES support on OpenSSL and facing on a similar restriction of AES-CTR mode. I'm trying to introduce a helper class to handle arbitrary counter length.
Comment 4 Yoshiaki Jitsukawa 2020-02-06 16:45:26 PST 
Created attachment 390027 [details]
Patch
Comment 5 Yoshiaki Jitsukawa 2020-02-06 16:55:38 PST 
Created attachment 390030 [details]
Patch
Comment 6 Yoshiaki Jitsukawa 2020-02-06 16:59:14 PST 
As bug 207176 is about to be resolved, I'll make another ticket for AES-CTR on OpenSSL with a separate test file. Thank you for your advice.
Comment 7 Jiewen Tan 2020-02-06 17:50:57 PST 
(In reply to Yoshiaki Jitsukawa from comment #3)
> Hi Jiewen,
> 
> As you know (thanks for the comment on bug 207176), we're adding AES support
> on OpenSSL and facing on a similar restriction of AES-CTR mode. I'm trying
> to introduce a helper class to handle arbitrary counter length.

It's very kind for you to address the issue for Apple ports as well. I will take a look at the patch.
Comment 8 Alexey Proskuryakov 2020-02-09 18:26:12 PST 
Comment on attachment 390030 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:848
> +		2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1CE41F574AE900D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1CF81F574C3900D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1CFA1F574C3E00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1CFC1F574CEE00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1CFE1F574CF300D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1D001F574CF700D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1D021F574D0000D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1D041F574D0500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1D061F574D0A00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> +		2DDE1D081F574D0E00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
>  		2DE70023192FE82A00B0975C /* DisplayRefreshMonitorMac.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DE70022192FE82A00B0975C /* DisplayRefreshMonitorMac.h */; };

This is suspicious. Why did all these change?
Comment 9 Yoshiaki Jitsukawa 2020-02-09 20:31:57 PST 
Created attachment 390229 [details]
Patch
Comment 10 Yoshiaki Jitsukawa 2020-02-09 21:14:56 PST 
(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 390030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390030&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:848
> > +		2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1CE41F574AE900D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1CF81F574C3900D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1CFA1F574C3E00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1CFC1F574CEE00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1CFE1F574CF300D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1D001F574CF700D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1D021F574D0000D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1D041F574D0500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1D061F574D0A00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> > +		2DDE1D081F574D0E00D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; };
> >  		2DE70023192FE82A00B0975C /* DisplayRefreshMonitorMac.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DE70022192FE82A00B0975C /* DisplayRefreshMonitorMac.h */; };
> 
> This is suspicious. Why did all these change?

Thank you for pointing out. Addressed.
Comment 11 Jiewen Tan 2020-02-11 13:45:44 PST 
Comment on attachment 390229 [details]
Patch

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

Overall, the patch looks good to me. Please address some of my comments.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:1
> +/*

Why don't you have a .cpp file?

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:35
> +template <size_t BlockSize = 16> // Counter block size in bytes.

Any reasons that the BlockSize is not fixed to 16 bytes?

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:36
> +class CryptoAlgorithmCounterBlockHelper {

Could it be more specific about AES-CTR? I guess no other algorithms need this.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:76
> +        for (size_t i = 0; i < sizeof(size_t); i++) {

It is unfortunate that there is no helpers in bitset to get value out.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:101
> +        for (int j = 0; j < 8; j++) {

It looks like that you reverse the endian of the byte? You can use
    for (size_t j = 7; j >= 0; j--)
to keep the endian same.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:102
> +            bits.set(i * 8 + j, byte & 0x01); // Test the LSB and set it to the bit.

This is why I dislike bitset. It is very tedious to manipulate the value and it is not efficient to do that as well. I guess there are no better options if you have dealt with counter size larger then size of size_t. The efficiency is probably no issues here comparing to the bundled decryption/encryption operations.

> Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:110
> +        for (int j = 0; j < 8; j++) {

Ditto.
Comment 12 Yoshiaki Jitsukawa 2020-02-11 23:31:50 PST 
(In reply to Jiewen Tan from comment #11)
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:1
> > +/*
> 
> Why don't you have a .cpp file?
> 
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:35
> > +template <size_t BlockSize = 16> // Counter block size in bytes.
> 
> Any reasons that the BlockSize is not fixed to 16 bytes?
> 
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:36
> > +class CryptoAlgorithmCounterBlockHelper {
> 
> Could it be more specific about AES-CTR? I guess no other algorithms need
> this.

I guess so, too. So how about having CryptoAlgorithmAES_CTRCounterBlockHelper class in CyptoAlgorithmAES_CTR.h/.cpp
with fixed block size 16?

I'm not sure which is better, an internal class like
 class CryptoAlgorithmAES_CTR::CounterBlockHelper
or an external class out of CryptoAlgorithmAES_CTR.

> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:76
> > +        for (size_t i = 0; i < sizeof(size_t); i++) {
> 
> It is unfortunate that there is no helpers in bitset to get value out.
> 
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:101
> > +        for (int j = 0; j < 8; j++) {
> 
> It looks like that you reverse the endian of the byte? You can use
>     for (size_t j = 7; j >= 0; j--)
> to keep the endian same.
> 
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:102
> > +            bits.set(i * 8 + j, byte & 0x01); // Test the LSB and set it to the bit.
> 
> This is why I dislike bitset. It is very tedious to manipulate the value and
> it is not efficient to do that as well. I guess there are no better options
> if you have dealt with counter size larger then size of size_t. The
> efficiency is probably no issues here comparing to the bundled
> decryption/encryption operations.
> 
> > Source/WebCore/crypto/CryptoAlgorithmCounterBlockHelper.h:110
> > +        for (int j = 0; j < 8; j++) {
> 
> Ditto.

Yes, it's super tedious...

Assuming 16 byte block size, I'll rewrite the helper with a pair of uint64_t.
Comment 13 Yoshiaki Jitsukawa 2020-02-12 01:46:07 PST 
Created attachment 390498 [details]
Patch
Comment 14 Jiewen Tan 2020-02-12 14:03:11 PST 
Comment on attachment 390498 [details]
Patch

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

Looks good overall. Please address my comments.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:45
> +        static constexpr size_t BlockSize = 16;

Could this be in the implementation?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:47
> +        explicit CounterBlockHelper(const Vector<uint8_t>& counterVector, size_t counterLength);

Since the constructor takes two parameters, there are no needs to make it explicit.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:50
> +        size_t countToOverflow() const;

In WebKit, we usually don't describe what the method does in comments. Instead, we make the method name be self explanatory.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:53
> +        Vector<uint8_t> counterVectorAfterOverflow() const;

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:56
> +        struct CounterBlockBits {

If this is an internal helper, it should be private.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:57
> +            static constexpr uint64_t All = 0xFFFFFFFFFFFFFFFFUL;

Could this be in the implementation?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:58
> +            void set()

We usually don't put methods into the headers unless they are simple getters or for performance reasons.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:64
> +            bool all() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:69
> +            bool any() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:74
> +            CounterBlockBits operator&(const CounterBlockBits& rhs) const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:82
> +            CounterBlockBits operator~() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:90
> +            CounterBlockBits& operator <<=(unsigned shift)

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:106
> +            CounterBlockBits& operator &=(const CounterBlockBits& rhs)

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:108
> +                *this = *this & rhs;

I believe this will create an unnecessary temp value. You should just modify the members.
Comment 15 Yoshiaki Jitsukawa 2020-02-12 15:19:07 PST 
(In reply to Jiewen Tan from comment #14)
Thank you for your review. I'll address all the points.

> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:45
> > +        static constexpr size_t BlockSize = 16;
> 
> Could this be in the implementation?

Yes. I'll make use of CryptoAlgorithmAES_CTRInternal::CounterSize.

> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:50
> > +        size_t countToOverflow() const;
> 
> In WebKit, we usually don't describe what the method does in comments.
> Instead, we make the method name be self explanatory.

Renaming to countToOverflowSaturating()
 
> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:53
> > +        Vector<uint8_t> counterVectorAfterOverflow() const;
>

I think it's self explanatory so I'm just removing the comment.
Comment 16 Yoshiaki Jitsukawa 2020-02-12 15:37:25 PST 
Created attachment 390572 [details]
Patch
Comment 17 Jiewen Tan 2020-02-12 16:32:07 PST 
Comment on attachment 390572 [details]
Patch

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

Some more nits. : )

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:220
> +    // Make a mask whose lowest counterLength bits are set.

We usually don't explain what we are doing in the code by comments. They should be self explanatory by choosing the right names. Instead, we usually comments why we are going this, like the above big-endian comment.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:225
> +    auto countMinusOne = ~m_bits & mask; // The count to overflow minus one.

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:233
> +    // Get the bits within the size_t range.

Doesn't seem to be related.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.cpp:308
> +CryptoAlgorithmAES_CTR::CounterBlockHelper::CounterBlockBits& CryptoAlgorithmAES_CTR::CounterBlockHelper::CounterBlockBits::operator &=(const CounterBlockBits& rhs)

In this case, you can use:
     auto CryptoAlgorithmAES_CTR::CounterBlockHelper::CounterBlockBits::operator &=(const CounterBlockBits& rhs) -> CounterBlockBits&

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:44
> +    class CounterBlockHelper {

Please move this class definition right below public:. Just a good behavior as helper classes can be used in any methods.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:49
> +

I guess you don't need a blank line here.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:56
> +

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:58
> +

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:61
> +            CounterBlockBits operator&(const CounterBlockBits& rhs) const;

You don't need the parameter name. Please comply the style checker.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:62
> +

No blank line.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:64
> +

No blank line.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:65
> +            CounterBlockBits& operator <<=(unsigned shift);

Omit the parameter line.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:66
> +

No blank line.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:67
> +            CounterBlockBits& operator &=(const CounterBlockBits& rhs);

Omit the parameter line.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:69
> +            static const uint64_t All;

Please move this to CryptoAlgorithmAES_CTRInternal given it adds no useful information in the headers.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:70
> +            uint64_t m_hi = 0;

uint64_t m_hi { 0 };

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:71
> +            uint64_t m_lo = 0;

Ditto.
Comment 18 Jiewen Tan 2020-02-12 16:34:02 PST 
(In reply to Jiewen Tan from comment #17)
> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:65
> > +            CounterBlockBits& operator <<=(unsigned shift);
> 
> Omit the parameter line.

name

> 
> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:66
> > +
> 
> No blank line.
> 
> > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:67
> > +            CounterBlockBits& operator &=(const CounterBlockBits& rhs);
> 
> Omit the parameter line.

name
Comment 19 Yoshiaki Jitsukawa 2020-02-12 17:28:56 PST 
Created attachment 390598 [details]
Patch
Comment 20 Yoshiaki Jitsukawa 2020-02-12 17:31:14 PST 
> Some more nits. : )

Thanks. Addressed :)
Comment 21 Jiewen Tan 2020-02-12 17:38:05 PST 
Comment on attachment 390598 [details]
Patch

LGTM. r=me. Please wait until all EWS turns green to land the patch.
Comment 22 Yoshiaki Jitsukawa 2020-02-12 17:47:31 PST 
(In reply to Jiewen Tan from comment #21)
> Comment on attachment 390598 [details]
> Patch
> 
> LGTM. r=me. Please wait until all EWS turns green to land the patch.

Sure. Thank you for the review!
Comment 23 WebKit Commit Bot 2020-02-12 23:20:22 PST 
Comment on attachment 390598 [details]
Patch

Clearing flags on attachment: 390598

Committed r256503: <https://trac.webkit.org/changeset/256503>
Comment 24 WebKit Commit Bot 2020-02-12 23:20:24 PST 
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2020-02-12 23:21:12 PST 
<rdar://problem/59415022>