Bug 207238 – [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength longer than 64

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

Created attachment 389739 [details]
Patch (test only)

Add tests for AES-CTR counter overlap.

[Jiewen Tan]

(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.

[Jiewen Tan]

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.

[Yoshiaki Jitsukawa]

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.

[Yoshiaki Jitsukawa]

Created attachment 390027 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 390030 [details]
Patch

[Yoshiaki Jitsukawa]

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.

[Jiewen Tan]

(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.

[Alexey Proskuryakov]

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?

[Yoshiaki Jitsukawa]

Created attachment 390229 [details]
Patch

[Yoshiaki Jitsukawa]

(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.

[Jiewen Tan]

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.

[Yoshiaki Jitsukawa]

(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.

[Yoshiaki Jitsukawa]

Created attachment 390498 [details]
Patch

[Jiewen Tan]

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.

[Yoshiaki Jitsukawa]

(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.

[Yoshiaki Jitsukawa]

Created attachment 390572 [details]
Patch

[Jiewen Tan]

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.

[Jiewen Tan]

(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

[Yoshiaki Jitsukawa]

Created attachment 390598 [details]
Patch

[Yoshiaki Jitsukawa]

> Some more nits. : )

Thanks. Addressed :)

[Jiewen Tan]

Comment on attachment 390598 [details]
Patch

LGTM. r=me. Please wait until all EWS turns green to land the patch.

[Yoshiaki Jitsukawa]

(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!

[WebKit Commit Bot]

Comment on attachment 390598 [details]
Patch

Clearing flags on attachment: 390598

Committed r256503: <https://trac.webkit.org/changeset/256503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59415022>