Bug 168344

<rdar://problem/20940221>

Created attachment 301565 [details] Patch

Attachment 301565 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:39: transformAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:73: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:97: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:43: usagesAreInvalidForCryptoAlgorithmAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:48: CryptoAlgorithmAES_CFB::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:53: CryptoAlgorithmAES_CFB::identifier is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:58: CryptoAlgorithmAES_CFB::encrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:69: CryptoAlgorithmAES_CFB::decrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:80: CryptoAlgorithmAES_CFB::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:98: CryptoAlgorithmAES_CFB::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:138: CryptoAlgorithmAES_CFB::exportKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 11 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301565&action=review > Source/WebCore/ChangeLog:11 > + importKey, exportKey, wrapKey, and unwrapKey. This implementation follows a slightly old version > + of WebCryptoAPI spec: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/#aes-cfb. Why does it follow a slightly old version of the spec? > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:47 > +#if PLATFORM(COCOA) > + CCAlgorithm aesAlgorithm = kCCAlgorithmAES; > +#else > + CCAlgorithm aesAlgorithm = kCCAlgorithmAES128; > +#endif This needs an explanation. Also, if this is a Mac file, why have a PLATFORM(COCOA) check at all?

Comment on attachment 301565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301565&action=review Thanks Sam for reviewing my patch. >> Source/WebCore/ChangeLog:11 >> + of WebCryptoAPI spec: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/#aes-cfb. > > Why does it follow a slightly old version of the spec? This algorithm is dropped in the current spec: https://www.w3.org/TR/WebCryptoAPI/. However iCloud.com explicitly requests this algorithm. Therefore, I follow the old spec to provide the support. >> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:47 >> +#endif > > This needs an explanation. Also, if this is a Mac file, why have a PLATFORM(COCOA) check at all? I borrow this from CryptoAlgorithmAES_CBCMac.cpp. It was originally PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090. Maybe we can drop support for OS X version < 1090 now?

Comment on attachment 301565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301565&action=review > Source/WebCore/ChangeLog:40 > + * PlatformGTK.cmake: Should I add CryptoAlgorithmAES_CFB.cpp to EFL and GTK?

Comment on attachment 301565 [details] Patch I don't think we should add support for an algorithm that has been dropped from the spec. Presumably Apple controls iCloud.com; why on Earth is it trying to use an algorithm that has never worked in WebKit...?

(In reply to comment #5) > Comment on attachment 301565 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301565&action=review > > Thanks Sam for reviewing my patch. > > >> Source/WebCore/ChangeLog:11 > >> + of WebCryptoAPI spec: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/#aes-cfb. > > > > Why does it follow a slightly old version of the spec? > > This algorithm is dropped in the current spec: > https://www.w3.org/TR/WebCryptoAPI/. However iCloud.com explicitly requests > this algorithm. Therefore, I follow the old spec to provide the support. Why was it removed from the spec? Is there an effort to bring it back? > > >> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:47 > >> +#endif > > > > This needs an explanation. Also, if this is a Mac file, why have a PLATFORM(COCOA) check at all? > > I borrow this from CryptoAlgorithmAES_CBCMac.cpp. It was originally > PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090. Maybe we can drop > support for OS X version < 1090 now? I don't completely follow. The original code makes sense, iOS or Mac >= 10.9, but your code doesn't do that. What are you trying to achieve with this conditional?

Comment on attachment 301565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301565&action=review >>>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:47 >>>> +#endif >>> >>> This needs an explanation. Also, if this is a Mac file, why have a PLATFORM(COCOA) check at all? >> >> I borrow this from CryptoAlgorithmAES_CBCMac.cpp. It was originally PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090. Maybe we can drop support for OS X version < 1090 now? > > I don't completely follow. The original code makes sense, iOS or Mac >= 10.9, but your code doesn't do that. What are you trying to achieve with this conditional? I think this entire conditional can just go away. I think 1090 is outside the minimum supported build target, so this is effectively PLATFORM(IOS) || PLATFORM(MAC). If this file is actually build on iOS, it doesn't seem like "Mac" is the right name, or the right path for this. We should move this stuff to a "crypto/cocoa" directory.

Comment on attachment 301565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301565&action=review >>>> Source/WebCore/ChangeLog:11 >>>> + of WebCryptoAPI spec: https://www.w3.org/TR/2014/CR-WebCryptoAPI-20141211/#aes-cfb. >>> >>> Why does it follow a slightly old version of the spec? >> >> This algorithm is dropped in the current spec: https://www.w3.org/TR/WebCryptoAPI/. However iCloud.com explicitly requests this algorithm. Therefore, I follow the old spec to provide the support. > > Why was it removed from the spec? Is there an effort to bring it back? It seems that the committee dropped this algorithm only because no user agents has implemented it. https://www.w3.org/2015/09/28-crypto-minutes.html We could file an issue to bring it back once we have the implementation. >>>>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:47 >>>>> +#endif >>>> >>>> This needs an explanation. Also, if this is a Mac file, why have a PLATFORM(COCOA) check at all? >>> >>> I borrow this from CryptoAlgorithmAES_CBCMac.cpp. It was originally PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090. Maybe we can drop support for OS X version < 1090 now? >> >> I don't completely follow. The original code makes sense, iOS or Mac >= 10.9, but your code doesn't do that. What are you trying to achieve with this conditional? > > I think this entire conditional can just go away. I think 1090 is outside the minimum supported build target, so this is effectively PLATFORM(IOS) || PLATFORM(MAC). > > If this file is actually build on iOS, it doesn't seem like "Mac" is the right name, or the right path for this. We should move this stuff to a "crypto/cocoa" directory. My mistake. There is another patch makes PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 to PLATFORM(COCOA). Bug 141014. Agreed with Brent, we could safely drop this conditional now.

(In reply to comment #7) > Comment on attachment 301565 [details] > Patch > > I don't think we should add support for an algorithm that has been dropped > from the spec. Presumably Apple controls iCloud.com; why on Earth is it > trying to use an algorithm that has never worked in WebKit...? From the spec: https://www.w3.org/TR/WebCryptoAPI/#algorithms-section-overview With that being said, user agents are free to choose any set of algorithms to support as long as those algorithms are well defined and conform to the spec's interfaces. If GTK doesn't feel good to have this algorithm implemented. I could implement it only for Apple ports.

I don't like r-ing a patch that you clearly spent a lot of effort working on, but I see only disadvantages to supporting this in WebKit. Why did you decide to work on it? iCloud.com clearly already functions well enough without AES-CFB. > Is there an effort to bring it back? I doubt it. The working group appears to have shut down. Anyway, I do not think CFB is useful to have, and I do not think we should support it in WebKit if it has been removed from the WebCrypto spec. There are tons of block cipher modes [1], and it is not useful or interesting to expose all of them to the web. CFB is a small tweak on the far more popular CBC mode; why not use that instead? Or why not use GCM, which to my understanding is more secure? We should expose what the specification authors have decided should be exposed unless there is a very strong argument for doing otherwise. Supporting obscure cipher modes increases the amount of security-critical code in WebKit that we have to maintain, with no real benefit. That said, I don't feel terribly strongly about this. It just doesn't seem useful. [1] https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation (In reply to comment #11) > If GTK doesn't feel good to have this algorithm implemented. I could > implement it only for Apple ports. We prefer to match as closely as possible the same set of features as Apple.

Looking at your list in bug #166746, all of the other algorithms you're planning to implement look familiar and popular, except maybe AES-CTR, but that one is actually in the spec. So this AES-CFB is the only one that seems odd to me.

(In reply to comment #13) > Looking at your list in bug #166746, all of the other algorithms you're > planning to implement look familiar and popular, except maybe AES-CTR, but > that one is actually in the spec. So this AES-CFB is the only one that seems > odd to me. We do need this specific algorithm for reasons I can't go into in this forum. We can conditionalize this on PLATFORM(COCOA) if that would make it more palatable to you.

That's... not a very great reason. But if it really has to be web exposed, then I'd much rather match the same functionality as Safari.

Ah, I get it now, you want it for some *future* iCloud feature that you can't talk about yet, not because there is some compatibility problem with the current website. I'm confused why this mode would be useful compared to using one of the more popular encryption modes. But if there is really some good reason... like I said, I'm not strongly opposed. I do think we should think twice before adding extra crypto algorithms, though. More is not better here!

Created attachment 301847 [details] Patch

Attachment 301847 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:39: transformAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:67: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:91: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:43: usagesAreInvalidForCryptoAlgorithmAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:48: CryptoAlgorithmAES_CFB::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:53: CryptoAlgorithmAES_CFB::identifier is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:58: CryptoAlgorithmAES_CFB::encrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:69: CryptoAlgorithmAES_CFB::decrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:80: CryptoAlgorithmAES_CFB::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:98: CryptoAlgorithmAES_CFB::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:138: CryptoAlgorithmAES_CFB::exportKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:36: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:41: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 13 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301847&action=review > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. Should be 2017. > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.h:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. Ditto. > Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. Ditto. > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. Ditto.

Comment on attachment 301847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301847&action=review > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:112 > + auto checkAlgCallback = [](size_t length, const std::optional<String>& alg) -> bool { No need for std::optional after Bug 167026.

Created attachment 302192 [details] Patch

Attachment 302192 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:39: transformAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:67: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:91: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:43: usagesAreInvalidForCryptoAlgorithmAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:48: CryptoAlgorithmAES_CFB::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:53: CryptoAlgorithmAES_CFB::identifier is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:58: CryptoAlgorithmAES_CFB::encrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:69: CryptoAlgorithmAES_CFB::decrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:80: CryptoAlgorithmAES_CFB::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:98: CryptoAlgorithmAES_CFB::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:138: CryptoAlgorithmAES_CFB::exportKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:36: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:41: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 13 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 302198 [details] Patch

Attachment 302198 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:39: transformAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:67: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CFBMac.cpp:91: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:43: usagesAreInvalidForCryptoAlgorithmAES_CFB is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:48: CryptoAlgorithmAES_CFB::create is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:53: CryptoAlgorithmAES_CFB::identifier is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:58: CryptoAlgorithmAES_CFB::encrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:69: CryptoAlgorithmAES_CFB::decrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:80: CryptoAlgorithmAES_CFB::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:98: CryptoAlgorithmAES_CFB::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CFB.cpp:138: CryptoAlgorithmAES_CFB::exportKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:36: CryptoAlgorithmAES_CFB::platformEncrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CFBGnuTLS.cpp:41: CryptoAlgorithmAES_CFB::platformDecrypt is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 13 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 302198 [details] Patch Looks good. r=me.

Comment on attachment 302198 [details] Patch Thanks Brent for r+ my patch.

Comment on attachment 302198 [details] Patch Clearing flags on attachment: 302198 Committed r212736: <http://trac.webkit.org/changeset/212736>

All reviewed patches have been landed. Closing bug.

(In reply to comment #27) > Comment on attachment 302198 [details] > Patch > > Clearing flags on attachment: 302198 > > Committed r212736: <http://trac.webkit.org/changeset/212736> and the cmake buildfix landed in https://trac.webkit.org/changeset/212950