Summary: | Fix an integer overflow in WebCrypto AES-CTR Mac implementation, which may detect a false loop | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tomoki Imai <tomoki.imai> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | don.olmstead, Hironori.Fujii, jiewen_tan, webkit-bug-importer, yoshiaki.jitsukawa | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Mac | ||||||||
OS: | macOS 10.15 | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=208186 | ||||||||
Attachments: |
|
Description
Tomoki Imai
2020-04-15 01:33:08 PDT
Created attachment 396514 [details]
patch
Comment on attachment 396514 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=396514&action=review Good catch! Please address my comments below. > Source/WebCore/ChangeLog:6 > + (1 << counterLength) causes an integer overflow, and the undefined behavior. Maybe you could reference here: https://en.cppreference.com/w/cpp/language/integer_literal. > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) ((size_t)1 => 1ull (In reply to Jiewen Tan from comment #2) > Comment on attachment 396514 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > Good catch! Please address my comments below. Thanks for your review! > > > Source/WebCore/ChangeLog:6 > > + (1 << counterLength) causes an integer overflow, and the undefined behavior. > > Maybe you could reference here: > https://en.cppreference.com/w/cpp/language/integer_literal. Will do. > > > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > > + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > > ((size_t)1 => 1ull I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. How do you think about it? Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. Comment on attachment 396514 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=396514&action=review >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) >> >> ((size_t)1 => 1ull > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > How do you think about it? > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. I was not aware we are still supporting 32 bit machines. Doing a C style cast is not appropriate in WebKit, please change it to static_cast<size_t>(1). Created attachment 396737 [details]
patch
Reflected the review comments
(In reply to Jiewen Tan from comment #5) > Comment on attachment 396514 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > >> > >> ((size_t)1 => 1ull > > > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > > > How do you think about it? > > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. > > I was not aware we are still supporting 32 bit machines. Doing a C style > cast is not appropriate in WebKit, please change it to > static_cast<size_t>(1). I changed it to static_cast<size_t>(1). I don't know about Mac implementation, but the other code seems to try to support 32-bit by using size_t. > > > Source/WebCore/ChangeLog:6 > > + (1 << counterLength) causes an integer overflow, and the undefined behavior. > > Maybe you could reference here: > https://en.cppreference.com/w/cpp/language/integer_literal. I added - https://en.cppreference.com/w/cpp/language/integer_literal and - https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators as references. (In reply to Tomoki Imai from comment #7) > (In reply to Jiewen Tan from comment #5) > > Comment on attachment 396514 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > > > >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > > >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > > >> > > >> ((size_t)1 => 1ull > > > > > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > > > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > > > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > > > > > How do you think about it? > > > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. > > > > I was not aware we are still supporting 32 bit machines. Doing a C style > > cast is not appropriate in WebKit, please change it to > > static_cast<size_t>(1). > > I changed it to static_cast<size_t>(1). > I don't know about Mac implementation, but the other code seems to try to > support 32-bit by using size_t. > That was true 4 years ago for Apple ports but not now. Anyway, I think it is better to keep the consistency. Comment on attachment 396737 [details]
patch
r=me.
Committed r260238: <https://trac.webkit.org/changeset/260238> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396737 [details]. |