Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7056026:
Implement AES-CTR for NSS. (Closed)
|
Created:
9 years, 7 months ago by Alpha Left Google Modified:
9 years, 2 months ago CC:
chromium-reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement AES-CTR for NSS.
Implement AES-128-CTR.
BUG=87152
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90425
Patch Set 1 #
Total comments: 7
Patch Set 2 : done #
Total comments: 10
Patch Set 3 : revised #
Total comments: 22
Patch Set 4 : fixed comments #
Total comments: 18
Patch Set 5 : done #
Total comments: 14
Patch Set 6 : done again #
Total comments: 19
Patch Set 7 : fixed #
Total comments: 2
Patch Set 8 : git try #
Total comments: 4
Patch Set 9 : align byets #
Total comments: 2
Patch Set 10 : endian #
Total comments: 33
Messages
Total messages: 37 (0 generated)
I know it's not published/finished, but some drive-by feedback/pointers for the implementation to hopefully save some trouble. As you get close towards finishing, you may wish to grab joth@ or bulach@ in on the loop for the _openssl.cc implementation, using EVP_aes_[key_length]_ecb(). http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_mac.cc File crypto/encryptor_mac.cc (right): http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_mac.cc#newcode52 crypto/encryptor_mac.cc:52: kCCOptionPKCS7Padding, Bug: kCCOptionECBMode should also be passed in the CCOptions if mode_ is ECB http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc#newcode65 crypto/encryptor_nss.cc:65: param_.reset(PK11_ParamFromIV(GetMechanism(mode), NULL)); PK11_ParamFromIV returns NULL for CKM_AES_ECB http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11mech.c?m... See below at line 68 for the trouble this may cause. http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc#newcode85 crypto/encryptor_nss.cc:85: SECStatus rv = PK11_CipherOp(context.get(), BUG: PK11_CipherOp doesn't support padding when used with CKM_AES_ECB, unlike CKM_AES_CBC_PAD. This means that |ciphertext_len| MUST be a multiple of AES_BLOCK_SIZE You can talk with wtc for more details, considering he responded on this thread - http://www.mail-archive.com/dev-tech-crypto@lists.mozilla.org/msg04546.html The same presumably applies to line 123, but perhaps less important on the decrypt size. http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_unittest.cc#ne... crypto/encryptor_unittest.cc:48: EXPECT_TRUE(encryptor.Init(key.get(), crypto::Encryptor::ECB, iv)); |iv| has no meaning in ECB mode. http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_win.cc File crypto/encryptor_win.cc (right): http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_win.cc#newcode53 crypto/encryptor_win.cc:53: DWORD cipher_mode = CRYPT_MODE_CBC; CRYPT_MODE_ECB for ECB. I'm not sure of KP_PADDING is legitimate for CRYPT_MODE_ECB (MSDN isn't clear), and presumably KP_IV wouldn't be set for CRYPT_MODE_ECB.
hclam: I didn't read the code, but the CL's description has typos. "SHA-ECB" should be "AES-ECB". ECB mode is not secure. It is best if our crypto API doesn't support ECB mode to prevent misuse.
On 2011/05/23 18:48:56, wtc wrote: > hclam: I didn't read the code, but the CL's description > has typos. "SHA-ECB" should be "AES-ECB". > > ECB mode is not secure. It is best if our crypto API doesn't > support ECB mode to prevent misuse. wtc: The reason I didn't make a similar comment is that it appears to be used to actually implement AES-128-CTR in http://codereview.chromium.org/7038053/ . ECB is exposed at the low-level so the protocol-dependent mechanics for the CTR can be handled at the high level. I do agree that both ECB and CTR tend to warrant special attention and care, given how they can be easily abused/misused. While I haven't looked at http://codereview.chromium.org/7038053/ in depth, the use of AES-CTR for a datagram-based protocol like P2P sockets doesn't seem too unreasonable, if absolutely necessary - but that's a review not yet undertaken. To be fair, the crypto protocol itself strikes me as something abarth@ may have designed, as it looks to have similar crypto properties as his proposal for websockets masking. Anyways, this was an in-progress CL that I jumped the gun on, so apologies hclam@ if the design is already changing :-)
On 2011/05/24 00:07:39, Ryan Sleevi wrote: > On 2011/05/23 18:48:56, wtc wrote: > > hclam: I didn't read the code, but the CL's description > > has typos. "SHA-ECB" should be "AES-ECB". > > > > ECB mode is not secure. It is best if our crypto API doesn't > > support ECB mode to prevent misuse. > > wtc: The reason I didn't make a similar comment is that it appears to be used to > actually implement AES-128-CTR in http://codereview.chromium.org/7038053/ . ECB > is exposed at the low-level so the protocol-dependent mechanics for the CTR can > be handled at the high level. I do agree that both ECB and CTR tend to warrant > special attention and care, given how they can be easily abused/misused. > > While I haven't looked at http://codereview.chromium.org/7038053/ in depth, the > use of AES-CTR for a datagram-based protocol like P2P sockets doesn't seem too > unreasonable, if absolutely necessary - but that's a review not yet undertaken. > > To be fair, the crypto protocol itself strikes me as something abarth@ may have > designed, as it looks to have similar crypto properties as his proposal for > websockets masking. > > Anyways, this was an in-progress CL that I jumped the gun on, so apologies > hclam@ if the design is already changing :-) Thanks for reviewing this. This patch was intended as a preview of a series of changes to implement UDP encryption in this spec: http://www.whatwg.org/specs/web-apps/current-work/complete/video-conferencing... See section 9.5. Enabling AES-128-ECB is to implement AES-128-CTR in the other patch, since AES-CTR doesn't seen to be available in NSS/CryptoAPI/Mac. I'll submit a formal review when they are ready. There'll be some cleanups, more testing but the design and structure will pretty much be the same.
On 2011/05/24 00:28:47, Alpha wrote: > On 2011/05/24 00:07:39, Ryan Sleevi wrote: > > On 2011/05/23 18:48:56, wtc wrote: > > > hclam: I didn't read the code, but the CL's description > > > has typos. "SHA-ECB" should be "AES-ECB". > > > > > > ECB mode is not secure. It is best if our crypto API doesn't > > > support ECB mode to prevent misuse. > > > > wtc: The reason I didn't make a similar comment is that it appears to be used > to > > actually implement AES-128-CTR in http://codereview.chromium.org/7038053/ . > ECB > > is exposed at the low-level so the protocol-dependent mechanics for the CTR > can > > be handled at the high level. I do agree that both ECB and CTR tend to warrant > > special attention and care, given how they can be easily abused/misused. > > > > While I haven't looked at http://codereview.chromium.org/7038053/ in depth, > the > > use of AES-CTR for a datagram-based protocol like P2P sockets doesn't seem too > > unreasonable, if absolutely necessary - but that's a review not yet > undertaken. > > > > To be fair, the crypto protocol itself strikes me as something abarth@ may > have > > designed, as it looks to have similar crypto properties as his proposal for > > websockets masking. > > > > Anyways, this was an in-progress CL that I jumped the gun on, so apologies > > hclam@ if the design is already changing :-) > > Thanks for reviewing this. This patch was intended as a preview of a series of > changes to implement UDP encryption in this spec: > http://www.whatwg.org/specs/web-apps/current-work/complete/video-conferencing... > > See section 9.5. > > Enabling AES-128-ECB is to implement AES-128-CTR in the other patch, since > AES-CTR doesn't seen to be available in NSS/CryptoAPI/Mac. I'll submit a formal > review when they are ready. > > There'll be some cleanups, more testing but the design and structure will pretty > much be the same. Thanks for the reference. Do you know if any of the Google crypto-nerds have looked at this? For example, the description of/use of AES CTR here actually seems a bit vague - the referenced http://www.whatwg.org/specs/web-apps/current-work/complete/references.html#re... doesn't actually specify how the counter should be incremented (see http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf , Appendix B, for different strategies). The fact that there are implementation differences (ex: see IPsec's definition/use-of AES-CTR vs OpenSSL's proposed EVP_aes_128_ctr() vs CryptoAPI big-vs-little-endian) is enough to merit looking at this. There are other concerns from looking at it (no rekeying defined, all of the nonce bits are used as the initial counter vs the recommended 2/n, etc), but perhaps the best thing (for review timing/implementation security) would be to have one of the dedicated Googler's look at it. I think there is a marked trade-off here with the protocol security going from the TLS-via-PseudoTCP to what this patch set begins, so it'd be good to know the end goal has been adequately reviewed. That said, copiously comment about how ECB /really/ shouldn't be used unless necessary/appropriate whenever it is implemented, since like wtc said, naked ECB is never good.
http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc#newcode65 crypto/encryptor_nss.cc:65: param_.reset(PK11_ParamFromIV(GetMechanism(mode), NULL)); On 2011/05/23 05:55:04, Ryan Sleevi wrote: > PK11_ParamFromIV returns NULL for CKM_AES_ECB > > http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11mech.c?m... > > See below at line 68 for the trouble this may cause. The function you are pointing at is PK11_IVFromParam. For PK11_ParamFromIV it returns an empty SECItem struct which PK11_CreateContextBySymKey requires. http://codereview.chromium.org/7056026/diff/1/crypto/encryptor_nss.cc#newcode85 crypto/encryptor_nss.cc:85: SECStatus rv = PK11_CipherOp(context.get(), On 2011/05/23 05:55:04, Ryan Sleevi wrote: > BUG: PK11_CipherOp doesn't support padding when used with CKM_AES_ECB, unlike > CKM_AES_CBC_PAD. This means that |ciphertext_len| MUST be a multiple of > AES_BLOCK_SIZE > > You can talk with wtc for more details, considering he responded on this thread > - http://www.mail-archive.com/dev-tech-crypto%40lists.mozilla.org/msg04546.html > > The same presumably applies to line 123, but perhaps less important on the > decrypt size. Done.
To reiterate: the purpose of enabling ECB mode is to simulate AES-CTR encryption mode described in this spec: http://www.whatwg.org/specs/web-apps/current-work/complete/video-conferencing... There might be possible security concerns in this spec but we would like to implement it as an alternative to TLS over PseudoTCP and an experimental encryption mode. The resultant stack will be PseudoTCP over PeerConnection (protected by AES-128). This will also be critical for us to get rid of TCP and use a datagram connection.
Several minor nits and one request for comment below. I think it's fine/correct to implement the PeerConnection spec as written, even though I personally suspect and expect it may change and have my own reservations. It seems some of these have already been expressed at length at by others on the current spec at [1]. Further, the fact that the current spec is very similar to what was proposed for websockets [2], for similar reasons, and the response [3] make me wonder if this will still be in the end solution. That said, as a means to a form of AES-CTR, this seems to be the correct approach, and assuming the nit/comments are addressed, LGTM. However, given wtc's concerns, perhaps he should be the one commenting and LGThim'ing before committing. [1] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-March/030929.html [2] http://www.ietf.org/mail-archive/web/hybi/current/msg05695.html [3] http://www.ietf.org/mail-archive/web/hybi/current/msg05820.html and http://www.ietf.org/mail-archive/web/hybi/current/msg05736.html are two of the major threads about concerns over AES in websockets for cross-protocol defense. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc#newc... crypto/encryptor_nss.cc:51: if (mode == ECB && !iv.empty()) return false; Catch the odd API abuse/misuse to prevent accidentally performing an ECB Encrypt/Decrypt for something (or some key) meant to be used with CBC. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:39: #if defined(OS_LINUX) && defined(USE_NSS) nit: Just "defined(USE_NSS)" is sufficient. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:43: crypto::SymmetricKey::DeriveKeyFromPassword( It would be better for the valgrind/heapchecker bots if this was just GenerateRandomKey() instead. The reason being the number of iterations that PBKDF2 requires to derive often cause the bots to timeout. Same for line 64/65. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:45: EXPECT_TRUE(NULL != key.get()); nit?: EXPECT_NE? My own opinion is that it's fine as written, since the benefit of EXPECT_NE is simply to print the values, and having it print NULL/0 is silly. That said, there seems to have been a number of CLs lately towards the most-specific EXPECT_ case, so I'm not sure if that's a new/emerging Chromium style. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:50: std::string plaintext("normal plaintext"); It's not clear from this string, compared with line 71 & 75, why this is valid plaintext but line 71 is invalid. Please add a comment here about why "normal plaintext" is ok, while "invalid plaintext" is magically not ok (16 bytes vs 17) http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:52: EXPECT_TRUE(encryptor.Encrypt(plaintext, &ciphertext)); nit: Since this is ECB mode, I think you may want to make sure to test the property of no extra padding being added. EXPECT_EQ(plaintext.size(), ciphertext.size()) http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_unittest.cc... crypto/encryptor_unittest.cc:57: EXPECT_TRUE(encryptor.Decrypt(ciphertext, &decypted)); nit: Is the "normal" use case to use two Encryptors, one for writing, the other for reading? Should that be tested here by initializing a new Decryptor before decrypting? The hypothetical edge case trying to be caught here is where Encrypt() modifies some state of the underlying key that causes Decrypt() to behave differently than if directly initialized and Decrypt() called (without calling Encrypt first) I don't feel strongly about this, since ECB should be stateless, but I wasn't sure if it should be a common-case that is tested for regressions. May be overkill.
hclam: I suggest that we expose the CTR mode instead of the insecure ECB mode in the Encryptor class. I suggest some minor changes below. http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc#newc... crypto/encryptor_nss.cc:18: inline CK_MECHANISM_TYPE GetMechanism(Encryptor::Mode mode) { Nit: GetMechanism => GetPKCS11Mechanism http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc#newc... crypto/encryptor_nss.cc:25: NOTREACHED() << "Unsupported mode of operation"; We probably should crash with a CHECK... http://codereview.chromium.org/7056026/diff/9001/crypto/encryptor_nss.cc#newc... crypto/encryptor_nss.cc:50: return false; Move this check into the "if (mode == CBC)" block on lines 56-63 below.
On 2011/06/02 20:10:23, wtc wrote: > hclam: I suggest that we expose the CTR mode instead of the > insecure ECB mode in the Encryptor class. > wtc: The problem with putting CTR in the Encryptor is that CTR is inherently application-/protocol-specific, and so putting it in the crypto/ doesn't seem to make much sense, given how things are layered and kept separate. For example, http://tools.ietf.org/html/rfc3686#section-4 or http://tools.ietf.org/html/rfc5930#section-2 (which references 3686 Section 4) for one possible counter construction. Compare this with the counter construction of OpenSSL 0.9.7 or OpenSSL 0.9.8, which use two different, incompatible counter constructs, both of which are incompatible with the high-level implementation of IPsec/IKE implementation (that is, without assuming how the Encryptor() is implemented or fixing the CTR to only increment the low bits). The OpenSSL CTR is meant to mirror/implement OpenSSH's SDCTR specification ( http://tools.ietf.org/html/rfc4344#section-4 ) More IETF proposed or approved protocols, that use different CTR constructions: http://tools.ietf.org/html/draft-ietf-tls-ctr-01#section-3.1.3 , http://tools.ietf.org/html/rfc5297#section-2.5 or http://tools.ietf.org/html/rfc3711#section-4.1.1 Further, in terms of security risks, CTR still can be easily and readily abused/misused, just like ECB - the most common example being two senders using the same IV to send messages (since a repeat of the CTR means C1 XOR C2 == P1 XOR P2). In addition, because it particularly susceptable to tampering (flip a bit in C1 and it will flip in P1), you still have to worry about the verification code, like a (keyed-) HMAC. So I'm not sure it will be much "safer" to bring it into Encryptor for that reason, unless the goal is just to ensure review/OWNERShip of the counter functionality to go with the cipher functionality.
rsleevi: thank you for the info. You made a good point that crypto code should be in the "crypto" directory. The CTR mode is a crypto construction, so its implementation should be easily identifiable in the source tree. I think we can implement the CTR mode API specified in PKCS #11, which should be able to handle all the variants of CTR mode you cited: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2.../pkcs-11v2-20a3.pdf
On 2011/06/03 02:14:03, wtc wrote: > > I think we can implement the CTR mode API specified in PKCS #11, > which should be able to handle all the variants of CTR mode you > cited: > ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2.../pkcs-11v2-20a3.pdf The URL should be ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-20/pkcs-11v2-20a3.pdf
On 2011/06/03 02:16:01, wtc wrote: > On 2011/06/03 02:14:03, wtc wrote: > > > > I think we can implement the CTR mode API specified in PKCS #11, > > which should be able to handle all the variants of CTR mode you > > cited: > > ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2.../pkcs-11v2-20a3.pdf > > The URL should be > ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-20/pkcs-11v2-20a3.pdf With regard to different CTR counter implementations, I can organize the Encryptor code to take a Counter object that the protocol can implement it's own version if it chooses to. Since the rest of the implementation is the same this should allow good reuse and flexibility.
The counter should be implemented in Encryptor. You can use
a counter structure similar to the one specified in PKCS #11:
typedef struct CK_AES_CTR_PARAMS {
CK_ULONG ulCounterBits;
CK_BYTE cb[16];
} CK_AES_CTR_PARAMS;
You only need to support the most common values of ulCounterBits:
32, 64, and 128. You can even just support ulCounterBits=128
initially.
Hey guys I updated this patch to include CTR encryption mode in Encryptor.
http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode43 crypto/encryptor.cc:43: } style nit: The | operator should go at the end of the line: http://www.chromium.org/developers/coding-style#TOC-Code-formatting http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode96 crypto/encryptor.cc:96: nit: Given that it's crypto code, should there be over/underflow checks here to be extra-defensive? http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode105 crypto/encryptor.cc:105: } DESIGN: This API requires that the entire mask be pre-allocated in order to be used below in MaskMessage(). One of the big advantages of CTR mode compared to other modes, such as CBC, is that the keystream isn't dependent on the blocks before it and can be programatically generated on demand, provided you have a valid, unique count/nonce, which all modern CTR-using protocols do. Further optimizing, ciphertext can be generated in place from the plaintext, with the only memory allocation necessary being that of one block (for the current mask). While it is possible (but not documented) for |ciphertext| and |plaintext| to use the same buffers below in MaskMessage(), requiring the Mask to be pre-allocated is rather unfortunate, and it'd be nice (bikeshedding?) if this wasn't the case. Perhaps that's already covered by the TODO(albertb) in the .h. This would, however, match the design of some of the other crypto/ classes, which allow for data streaming (eg: SignatureCreator/Verifier, SecureHash), and at least minimize the memory footprint. Perhaps that isn't a concern since I think last I saw, P2PSocket messages will be capped at 576 bytes? I would suggest combining MaskMessage() and GenerateCounterMask() into a simple EncryptCtr() type method, so that you only need to do new uint8[kBlockLength]... http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode12 crypto/encryptor.h:12: #include "build/build_config.h" IWYU: #include "base/basictypes.h" for uint64/uint8. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode30 crypto/encryptor.h:30: class Counter { It's not clear why you added virtual methods and made this part of the public interface, when there are no public methods that an Encryptor takes a Counter, so there is no way for clients of this API to override with custom behaviour. Note that if clients did need to derive, I think wtc's comment about CK_AES_CTR_PARAM is sufficient for the API needs, and wouldn't require any custom-derived classes to implement. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode38 crypto/encryptor.h:38: // Write the content of the counter to |buf|. Documentation nit: Should specify that |buf| should be sized to store |GetLengthInBytes()| bytes. API nit?: void* instead of uint8* http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode81 crypto/encryptor.h:81: int* mask_len); style nit: I believe the style guide preference is to use size_t for |plaintext_len| and |mask_len| (also below) http://www.chromium.org/developers/coding-style#TOC-Types style nit: My understanding of http://www.chromium.org/developers/coding-style#TOC-Code-formatting was that each argument should be on it's own line for the declaration. I'm not sure if |plaintext_len| and |mask| have the strict relationship to justify combining. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode86 crypto/encryptor.h:86: const uint8* mask, uint8* ciphertext) const; Documentation nit: |ciphertext| must be at least |plaintext_len| bytes. API nit: Should |plaintext|, |mask|, and |ciphertext| be void* instead of uint8*? I don't see how from a caller's perspective the type should matter... http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:140: } In CTR mode, why not just call Encrypt() with the |ciphertext| and |plaintext| swapped, since a CTR decrypt is just ciphertext xor keystream instead of plaintext xor keystream. That would simply Decrypt() by keeping the two in sync without the code duplication. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:150: int plaintext_len = ciphertext.size(); nit: ciphertext_data, ciphertext_len (also lines 165/166) http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:44: crypto::SymmetricKey::AES, "password", "saltiest", 1000, 128)); Since you're not checking a known-answer test (should there be KATs here?), it would be much better for cycle times to generate a random key rather than a PBKDF2-derived key.
http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode43 crypto/encryptor.cc:43: } On 2011/06/08 01:29:23, Ryan Sleevi wrote: > style nit: The | operator should go at the end of the line: > > http://www.chromium.org/developers/coding-style#TOC-Code-formatting Done. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode96 crypto/encryptor.cc:96: On 2011/06/08 01:29:23, Ryan Sleevi wrote: > nit: Given that it's crypto code, should there be over/underflow checks here to > be extra-defensive? I've changed the types here to size_t and check the input pointers. I think this should enough. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.cc#newcode105 crypto/encryptor.cc:105: } On 2011/06/08 01:29:23, Ryan Sleevi wrote: > DESIGN: This API requires that the entire mask be pre-allocated in order to be > used below in MaskMessage(). One of the big advantages of CTR mode compared to > other modes, such as CBC, is that the keystream isn't dependent on the blocks > before it and can be programatically generated on demand, provided you have a > valid, unique count/nonce, which all modern CTR-using protocols do. > > Further optimizing, ciphertext can be generated in place from the plaintext, > with the only memory allocation necessary being that of one block (for the > current mask). > > While it is possible (but not documented) for |ciphertext| and |plaintext| to > use the same buffers below in MaskMessage(), requiring the Mask to be > pre-allocated is rather unfortunate, and it'd be nice (bikeshedding?) if this > wasn't the case. Perhaps that's already covered by the TODO(albertb) in the .h. > > This would, however, match the design of some of the other crypto/ classes, > which allow for data streaming (eg: SignatureCreator/Verifier, SecureHash), and > at least minimize the memory footprint. Perhaps that isn't a concern since I > think last I saw, P2PSocket messages will be capped at 576 bytes? > > I would suggest combining MaskMessage() and GenerateCounterMask() into a simple > EncryptCtr() type method, so that you only need to do new uint8[kBlockLength]... My first attempt was a combined version of GenerateCounterMask() and MaskMessage(). The mask was also generated in place and XOR with the input message. So the loop was: Approach 1: for i = 1 to blocks Generate 16 bytes mask Encrypt mask message <= mask XOR message However this turned out to be *very slow*. The current approach is like this: Approach 2: Generate mask just bigger than message Encrypt mask messaeg <= mask XOR message Performance numbers: Message size = 33 bytes, Encrypt 1 million times Approach 1: 1600 ms Approach 2: 1659 ms Message size = 1000 bytes, Encrypt 1 million times Approach 1: 43675 ms Approach 2: 3531 ms Generating the block in place and XOR is way too slow compared to generating the whole mask at once. We could have preallocated a mask of size say 1024 bytes to avoid having generating too large a mask everytime, but this logic becomes hard to share among ports I took the easier to implement approach. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode12 crypto/encryptor.h:12: #include "build/build_config.h" On 2011/06/08 01:29:23, Ryan Sleevi wrote: > IWYU: #include "base/basictypes.h" for uint64/uint8. Done. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode30 crypto/encryptor.h:30: class Counter { On 2011/06/08 01:29:23, Ryan Sleevi wrote: > It's not clear why you added virtual methods and made this part of the public > interface, when there are no public methods that an Encryptor takes a Counter, > so there is no way for clients of this API to override with custom behaviour. > > Note that if clients did need to derive, I think wtc's comment about > CK_AES_CTR_PARAM is sufficient for the API needs, and wouldn't require any > custom-derived classes to implement. I made it non virtual now. We could implement them in Counter later for different number of bits. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode38 crypto/encryptor.h:38: // Write the content of the counter to |buf|. On 2011/06/08 01:29:23, Ryan Sleevi wrote: > Documentation nit: Should specify that |buf| should be sized to store > |GetLengthInBytes()| bytes. > > API nit?: void* instead of uint8* Done. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode81 crypto/encryptor.h:81: int* mask_len); On 2011/06/08 01:29:23, Ryan Sleevi wrote: > style nit: I believe the style guide preference is to use size_t for > |plaintext_len| and |mask_len| (also below) > > http://www.chromium.org/developers/coding-style#TOC-Types > > style nit: My understanding of > http://www.chromium.org/developers/coding-style#TOC-Code-formatting was that > each argument should be on it's own line for the declaration. I'm not sure if > |plaintext_len| and |mask| have the strict relationship to justify combining. Done. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor.h#newcode86 crypto/encryptor.h:86: const uint8* mask, uint8* ciphertext) const; On 2011/06/08 01:29:23, Ryan Sleevi wrote: > Documentation nit: |ciphertext| must be at least |plaintext_len| bytes. > > API nit: Should |plaintext|, |mask|, and |ciphertext| be void* instead of > uint8*? I don't see how from a caller's perspective the type should matter... Done. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:140: } On 2011/06/08 01:29:23, Ryan Sleevi wrote: > In CTR mode, why not just call Encrypt() with the |ciphertext| and |plaintext| > swapped, since a CTR decrypt is just ciphertext xor keystream instead of > plaintext xor keystream. That would simply Decrypt() by keeping the two in sync > without the code duplication. I make Encrypt and Decrypt to share the same code now. This should be cleaner. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:150: int plaintext_len = ciphertext.size(); On 2011/06/08 01:29:23, Ryan Sleevi wrote: > nit: ciphertext_data, ciphertext_len (also lines 165/166) See the new functions, they are just called input and output, easier to read. http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/13002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:44: crypto::SymmetricKey::AES, "password", "saltiest", 1000, 128)); On 2011/06/08 01:29:23, Ryan Sleevi wrote: > Since you're not checking a known-answer test (should there be KATs here?), it > would be much better for cycle times to generate a random key rather than a > PBKDF2-derived key. Done.
hclam: I made a pass through the CL. Here are my comments. I'd like to review it again after you address the comments. In general the code looks correct but can use more comments. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode42 crypto/encryptor.cc:42: (static_cast<uint64>(Get8(memory, 7)) << 0); Nit: it may be better to violate the Style Guide and align these static_cast lines with the static_cast on line 35. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode69 crypto/encryptor.cc:69: uint8* buf_ptr = reinterpret_cast<uint8*>(buf); This typecast to unit8* is not necessary because you defined SetBE64 to take void*. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode80 crypto/encryptor.cc:80: // Partial Encryptor Implementation. Nit: add a blank line. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode98 crypto/encryptor.cc:98: const int kBlockLength = counter_->GetLengthInBytes(); Use size_t. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode31 crypto/encryptor.h:31: class Counter { Please document this class. At least mention it is related to the CTR mode. Your Counter class is less general than the one used in PKCS #11. Why? http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode44 crypto/encryptor.h:44: const int GetLengthInBytes() const; GetLengthInBytes() should return size_t. The return value should not be const: size_t GetLengthInBytes() const; http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode67 crypto/encryptor.h:67: // counter value is supported. The limitation of 128-bits counter values should be documented in the Counter class above instead of here. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:23: return CKM_AES_ECB; A comment explaining why ECB mode is used would be nice. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:95: LOG(ERROR) << "Count value not set in CTR mode."; Typo: Count => Counter
http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode42 crypto/encryptor.cc:42: (static_cast<uint64>(Get8(memory, 7)) << 0); On 2011/06/14 18:11:06, wtc wrote: > Nit: it may be better to violate the Style Guide and > align these static_cast lines with the static_cast on > line 35. Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode69 crypto/encryptor.cc:69: uint8* buf_ptr = reinterpret_cast<uint8*>(buf); On 2011/06/14 18:11:06, wtc wrote: > This typecast to unit8* is not necessary because > you defined SetBE64 to take void*. This is because of the pointer arithemetic below at line 72. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode80 crypto/encryptor.cc:80: // Partial Encryptor Implementation. On 2011/06/14 18:11:06, wtc wrote: > Nit: add a blank line. Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.cc#newcode98 crypto/encryptor.cc:98: const int kBlockLength = counter_->GetLengthInBytes(); On 2011/06/14 18:11:06, wtc wrote: > Use size_t. Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode31 crypto/encryptor.h:31: class Counter { On 2011/06/14 18:11:06, wtc wrote: > Please document this class. At least mention it is related > to the CTR mode. > > Your Counter class is less general than the one used in > PKCS #11. Why? This is implemented to support AES-CTR-128 mode only. This is the only CTR mode that we use. Since the Encryptor interface assumes 128-bits already so we don't need that flexibility in Counter. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode44 crypto/encryptor.h:44: const int GetLengthInBytes() const; On 2011/06/14 18:11:06, wtc wrote: > GetLengthInBytes() should return size_t. The return value > should not be const: > size_t GetLengthInBytes() const; Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor.h#newcode67 crypto/encryptor.h:67: // counter value is supported. On 2011/06/14 18:11:06, wtc wrote: > The limitation of 128-bits counter values should be documented > in the Counter class above instead of here. Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:23: return CKM_AES_ECB; On 2011/06/14 18:11:06, wtc wrote: > A comment explaining why ECB mode is used would be nice. Done. http://codereview.chromium.org/7056026/diff/22001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:95: LOG(ERROR) << "Count value not set in CTR mode."; On 2011/06/14 18:11:06, wtc wrote: > Typo: Count => Counter Done. Also I moved this block to CryptCTR.
Hopefully I haven't screwed up the math below. It's probably an extreme edge case, and one unlikely to be hit with P2PSocket, but as far as a general API goes... http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc#newcode92 crypto/encryptor.cc:92: void Encryptor::GenerateCounterMask(size_t plaintext_len, pathological abuse of/hostile attack of API: Step 1: Input: kBlockLength = 16 (AES = 128-bit block size/key) plaintext_len = (std::numeric_limits<size_t>::max() - kBlockLength + 2) assume std::numeric_limits<size_t>::max() == 4,294,967,295 plaintext_len = 4,294,967,295 - 16 + 2 plaintext_len = 4,294,967,281 http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc#newcode100 crypto/encryptor.cc:100: size_t blocks = (plaintext_len + kBlockLength - 1) / kBlockLength; Step 2: size_t blocks = (plaintext_len + kBlockLength - 1) / kBlockLength blocks = (4,294,967,281 + 16 - 1) / 16 blocks = (1, due to wrapping) / 16 blocks = 0 http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc#newcode102 crypto/encryptor.cc:102: *mask_len = blocks * kBlockLength; Step 3: *mask_len = blocks (0) * kBlockLength (16) *mask_len = 0 * 16 *mask_len = 0 http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc#newcode103 crypto/encryptor.cc:103: mask->reset(new uint8[*mask_len]); Step 4: BUG?: mask->reset(new uint8[0]) I know in general in Chromium, allocation errors aren't checked (eg: malloc() returning NULL), since OOM is expected to cause a crash. Given that new (type)[0] returns a valid pointer, but one that cannot/should not be de-referenced, I'm not sure if this type of issue is treated similarly. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:107: int input_len = input.size(); Any issues with over/underflow here or on lines 102, 141, 146? I suppose the bug would have existed in the previous code as well, and it certainly represents a pathological case. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:152: CHECK(input_len <= output_len); Any value to CHECK_LE? Only difference would be it would include |input_len| and |output_len| in the message itself. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:164: According to http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn5.html , Encrypt/Decrypt should call PK11_DigestFinal Ultimately, this calls into the PKCS#11 function C_EncryptFinal / C_DecryptFinal, so I don't believe this call can be skipped even though the data isn't padded. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:9: #include "base/rand_util.h" not needed; see below. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:45: base::RandBytesAsString(16))); nit: crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES, 128)
http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor.cc#newcode103 crypto/encryptor.cc:103: mask->reset(new uint8[*mask_len]); On 2011/06/15 06:27:00, Ryan Sleevi wrote: > Step 4: > BUG?: mask->reset(new uint8[0]) > > I know in general in Chromium, allocation errors aren't checked (eg: malloc() > returning NULL), since OOM is expected to cause a crash. Given that new > (type)[0] returns a valid pointer, but one that cannot/should not be > de-referenced, I'm not sure if this type of issue is treated similarly. > I don't think de-referencing a [0] pointer would be a critical problem but I added a CHECK(blocks) to make sure it is non zero. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:107: int input_len = input.size(); On 2011/06/15 06:27:00, Ryan Sleevi wrote: > Any issues with over/underflow here or on lines 102, 141, 146? > > I suppose the bug would have existed in the previous code as well, and it > certainly represents a pathological case. There could be problems at 102 and 141 but not 146. I've added CHECK() to prevent overflow. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:152: CHECK(input_len <= output_len); On 2011/06/15 06:27:00, Ryan Sleevi wrote: > Any value to CHECK_LE? Only difference would be it would include |input_len| and > |output_len| in the message itself. GenerateCounterMask will bump up input_len, this is to ensure there's always enough storage allocated. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:164: On 2011/06/15 06:27:00, Ryan Sleevi wrote: > According to > http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn5.html , > Encrypt/Decrypt should call PK11_DigestFinal > > Ultimately, this calls into the PKCS#11 function C_EncryptFinal / > C_DecryptFinal, so I don't believe this call can be skipped even though the data > isn't padded. Done. http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/27001/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:45: base::RandBytesAsString(16))); On 2011/06/15 06:27:00, Ryan Sleevi wrote: > nit: crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES, 128) Done.
ping.
ping on review.
Sorry Alpha, I've been on the road and haven't been able to review. I believe wtc@ should have final say the API, since he's the one who originally raised concerns about it. I've taken one more pass through, and I think all the major issues have been addressed. I found one more pedantic/paranoid point that I don't believe should be hit in practice, but is worth mentioning. Other than that, the implementation LGTM - but like I said, speak to wtc@ first. You may wish to ping him outside of codereview, either via IM or direct mail, to ensure he notices. http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:143: size_t output_len = input.size() + AES_BLOCK_SIZE; nit: Is it necessary to pad |output_len| ? The underlying cipher is ECB, and the mask code is smart enough to round up, so I'm not sure why. Was this supposed to pad output_len to a multiple of AES_BLOCK_SIZE for PK11_CipherOp? http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:165: input_len); BUG: There is no guarantee that |op_len| will == |input_len| or |output_len|, AFAICT. In the CBC mode/Encryptor::Crypt(), you see that beyond this point, |op_len| is used for the result length (lines 126, 128, and 132). The risk here is that the buffer past |op_len| (either due to overflow of int vs size_t, or due to the PKCS#11 module only doing a subset of |op_len|) will contain the mask value, rather than the encrypted mask value.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:143: size_t output_len = input.size() + AES_BLOCK_SIZE; On 2011/06/20 23:47:23, Ryan Sleevi wrote: > nit: Is it necessary to pad |output_len| ? The underlying cipher is ECB, and the > mask code is smart enough to round up, so I'm not sure why. > > Was this supposed to pad output_len to a multiple of AES_BLOCK_SIZE for > PK11_CipherOp? This is just for convenience, it's easier to check make sure this is correct if we spend a few more bytes. http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:165: input_len); On 2011/06/20 23:47:23, Ryan Sleevi wrote: > BUG: There is no guarantee that |op_len| will == |input_len| or |output_len|, > AFAICT. In the CBC mode/Encryptor::Crypt(), you see that beyond this point, > |op_len| is used for the result length (lines 126, 128, and 132). > > The risk here is that the buffer past |op_len| (either due to overflow of int vs > size_t, or due to the PKCS#11 module only doing a subset of |op_len|) will > contain the mask value, rather than the encrypted mask value. Since this is using ECB and input_len is a multiple of 16 bytes I would like to enforce op_len == input_len. I'll make a check here.
LGTM.
High-Level Comments
1. Please include the flexibility of the PKCS #11 interface:
typedef struct CK_AES_CTR_PARAMS {
CK_ULONG ulCounterBits;
CK_BYTE cb[16];
} CK_AES_CTR_PARAMS;
You can require ulCounterBits == 128 in your code. I have
an comment on this issue below.
2. I believe it is possible to avoid the allocation of the
counter mask in each call to GenerateCounterMask. I have
two comments on this issue below.
3. Please open a bug report on this feature.
Thanks!
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.cc
File crypto/encryptor.cc (right):
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.cc#newcode63
crypto/encryptor.cc:63: // Overflow occurs.
Nit: perhaps past tense ("occurred") would be better?
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.cc#newcode104
crypto/encryptor.cc:104: mask->reset(new uint8[*mask_len]);
It is inefficient to allocate the mask block in every call to
this function. Can you redesign the API to avoid this?
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.h
File crypto/encryptor.h (right):
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.h#newcode35
crypto/encryptor.h:35: Counter(const std::string& counter);
To match PKCS #11's CK_AES_CTR_PARAMS structure, the
constructor should also take a
size_t counter_bits
input argument, to indicate the number of bits in 'counter'
that shall be incremented. The counter bits are the least
significant bits of the counter block.
In general 0 < counter_bits <= 128, but you should add
CHECK_EQ(counter_bits, 128);
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.h#newcode59
crypto/encryptor.h:59: // When |mode| is CTR then |iv| should be empty.
Nit: When => If
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.h#newcode69
crypto/encryptor.h:69: // counter value is supported.
Nit: remove "Currently only 128-bits counter value is supported."
This is documented above in front of the Counter class.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.h#newcode71
crypto/encryptor.h:71: // Return true only if update was successful.
Nit: the Style guide recommends using "descriptive" rather
than "imperative" verbs, so use "Updates", "Returns",
"Generates", and "Masks" instead of "Update", "Return",
"Generate", and "Mask".
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments
Nit: should this method be named SetCounter()?
"UpdateCounter" seems to imply it increments the counter.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc
File crypto/encryptor_nss.cc (right):
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:23: // AES-CTR encryption uses ECB encryptor as a
building block.
Please explain this is necessary because NSS doesn't support
CKM_AES_CTR yet.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:67: }
I would probably use a "switch" statement here so that we
can add new modes of operation more easily in the future.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:83: Crypt(context.get(), plaintext, ciphertext);
Nit: please use an "if" or "switch" statement here and in
the Decrypt method on lines 96-97 below.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new...
crypto/encryptor_nss.cc:155: GenerateCounterMask(input_len, &ctr_mask,
&input_len);
Since PK11_CipherOp can encrypt in place, you should be
able to use output_data to receive the CTR mask and pass
it to PK11_CipherOp for in-place encryption.
This will avoid one memory allocation (of ctr_mask).
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_unittest.cc
File crypto/encryptor_unittest.cc (right):
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_unittest.c...
crypto/encryptor_unittest.cc:97: #endif
Nit: add a
// defined(USE_NSS)
comment.
http://codereview.chromium.org/7056026/diff/41001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/41001/crypto/encryptor.cc#newcode43 crypto/encryptor.cc:43: } There are compiler built-in or intrinsic functions to do SetBE64 and GetBE64 efficiently. For example, Visual C++ has _byteswap_uint64: http://msdn.microsoft.com/en-US/library/a3140177%28v=VS.80%29.aspx You can add a TODO comment to look into this optimization.
http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor.cc#newcode63 crypto/encryptor.cc:63: // Overflow occurs. On 2011/06/21 00:33:32, wtc wrote: > Nit: perhaps past tense ("occurred") would be better? Done. http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:23: // AES-CTR encryption uses ECB encryptor as a building block. On 2011/06/21 00:33:32, wtc wrote: > Please explain this is necessary because NSS doesn't support > CKM_AES_CTR yet. Done. http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:67: } On 2011/06/21 00:33:32, wtc wrote: > I would probably use a "switch" statement here so that we > can add new modes of operation more easily in the future. Done. http://codereview.chromium.org/7056026/diff/33001/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:83: Crypt(context.get(), plaintext, ciphertext); On 2011/06/21 00:33:32, wtc wrote: > Nit: please use an "if" or "switch" statement here and in > the Decrypt method on lines 96-97 below. Done. http://codereview.chromium.org/7056026/diff/41001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/41001/crypto/encryptor.cc#newcode43 crypto/encryptor.cc:43: } On 2011/06/21 00:46:32, wtc wrote: > There are compiler built-in or intrinsic functions > to do SetBE64 and GetBE64 efficiently. > > For example, Visual C++ has _byteswap_uint64: > http://msdn.microsoft.com/en-US/library/a3140177%2528v=VS.80%2529.aspx > > You can add a TODO comment to look into this optimization. Done.
hclam: thank you for implementing the byte swap optimization I suggested. Patch Set 8 has two serious portability problems, described below. http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.cc#newcode22 crypto/encryptor.cc:22: #include <byteswap.h> This could be a porting problem because <byteswap.h> may not be a standard Unix system header. Let's give this a try. http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.h#newcode52 crypto/encryptor.h:52: uint8 counter_buf_[16]; IMPORTANT: in the crypto/encryptor.cc you cast this byte buffer to a uint64: reinterpret_cast<uint64*>(counter_buf_) This is only safe if the byte buffer is aligned for uint64. You need to force alignment with a union: union { uint8 counter_buf_[16]; uint64 force_align_[2]; } u; x86 and x64 CPUs allow unaligned access, but some CPUs will generate a SIGBUS signal and kill the process. It's also bad to write code that only works on little endian processes when portable code can be written. Your previous patch set is portable. What you need is hton macros (host to network byte order conversion) that call bswap on little endian systems and are no-op on big-endian systems.
http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.cc#newcode22 crypto/encryptor.cc:22: #include <byteswap.h> On 2011/06/22 22:12:28, wtc wrote: > This could be a porting problem because <byteswap.h> may > not be a standard Unix system header. Let's give this a try. This set of defines are actually from google3. http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/46001/crypto/encryptor.h#newcode52 crypto/encryptor.h:52: uint8 counter_buf_[16]; On 2011/06/22 22:12:28, wtc wrote: > IMPORTANT: in the crypto/encryptor.cc you cast this byte > buffer to a uint64: > reinterpret_cast<uint64*>(counter_buf_) > > This is only safe if the byte buffer is aligned for uint64. > You need to force alignment with a union: > > union { > uint8 counter_buf_[16]; > uint64 force_align_[2]; > } u; > > x86 and x64 CPUs allow unaligned access, but some CPUs will > generate a SIGBUS signal and kill the process. Done. > > It's also bad to write code that only works on little endian > processes when portable code can be written. Your previous > patch set is portable. What you need is hton macros > (host to network byte order conversion) that call bswap > on little endian systems and are no-op on big-endian systems. I've discussed with brett about byte order, it turns out our code is mostly little endian. He suggested that it's better to assume little endian and give an error if otherwise, rather than having some untested code path.
Please let me know what you think regarding byte order. Thanks!
hclam: anyone who writes networking or crypto code should be able to deal with byte order correctly. It is not hard, and you demonstrated you can do that with your earlier patch set and your runtime byte order check. To be precise, I illustrated how it is usually done below. http://codereview.chromium.org/7056026/diff/53001/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/53001/crypto/encryptor.cc#newcode23 crypto/encryptor.cc:23: #endif To make your code correct for big-endian systems, you just need to do the following. 1. Define the ntoh_64 and hton_64 macros. // This part can be moved to "build/build_config.h". #if defined(ARCH_CPU_X86_64) || defined(ARCH_CPU_X86) || \ defined(ARCH_CPU_ARMEL) #define ARCH_CPU_LITTLE_ENDIAN 1 #else #error Please add your architecture and define either ARCH_CPU_LITTLE_ENDIAN or ARCH_CPU_BIG_ENDIAN #endif #if defined(ARCH_CPU_LITTLE_ENDIAN) #define ntoh_64(x) bswap_64(x) #define hton_64(x) bswap_64(x) #else #define ntoh_64(x) (x) #define hton_64(x) (x) #endif 2. Use ntoh_64 and hton_64 instead of bswap_64 in Encryptor::Counter::Increment(). Note that I also take advantage of the union and remove the typecast. We should rename force_align to something more readable if we use it this way. void Encryptor::Counter::Increment() { uint64 low_num = ntoh_64(counter_.force_align[1]); uint64 new_low_num = low_num + 1; counter_.force_align[1] = hton_64(new_low_num); // Overflow occurred. if (new_low_num < low_num) counter_.force_align[0] = hton_64(ntoh_64(counter_.force_align[0]) + 1); } http://codereview.chromium.org/7056026/diff/53001/crypto/encryptor.cc#newcode25 crypto/encryptor.cc:25: namespace crypto { Add a blank line after this line.
Thanks for your suggestions! I've used your advice and updated the patch, please take a look.
LGTM. Please make the following changes before you check this in. Thanks. http://codereview.chromium.org/7056026/diff/54002/build/build_config.h File build/build_config.h (right): http://codereview.chromium.org/7056026/diff/54002/build/build_config.h#newcode97 build/build_config.h:97: #define ARCH_CPU_64_BITS 1 Please define ARCH_CPU_LITTLE_ENDIAN here, repeated for the three supported CPUs: #define ARCH_CPU_LITTLE_ENDIAN 1 #undef ARCH_CPU_BIG_ENDIAN The #undef is not necessary and merely serves to document what the opposite macro is, since we don't support any big-endian architecture right now. Then we don't need the separate block on lines 111-117. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode47 crypto/encryptor.cc:47: << "This code only runs in little endian system"; Delete the runtime endianness check (lines 43-47). http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode50 crypto/encryptor.cc:50: memcpy(counter_.buf, counter.data(), kCounterLength); This should be memcpy(&counter_, counter.data(), counter.length()); http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode61 crypto/encryptor.cc:61: // Overflow occured then increment the most significant component. Add "If". http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode70 crypto/encryptor.cc:70: memcpy(buf_ptr, counter_.buf, kCounterLength); This function can simply say: memcpy(buf, &counter_, sizeof(counter_)); http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode74 crypto/encryptor.cc:74: return counter_bits_ / 8; This should be return sizeof(counter_); http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode41 crypto/encryptor.h:41: void Increment(); Nit: please make this method return bool, or add a TODO comment about it. It should fail if the counter overflows. If you do this, GenerateCounterMask also needs to return bool, and CryptCTR needs to check the return value of GenerateCounterMask. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode51 crypto/encryptor.h:51: size_t counter_bits_; You misunderstood what I asked you to do with counter_bits_. There are two lengths of interest for a counter block. 1. The counter block size, which must be equal to the size of the block cipher. For AES, this is 128 bits. 2. The number of counter bits in the counter block that are incremented. In our first implementation we only support 128 bits of counter. To make it simple, let's do the following in this CL: 1. Remove counter_bits_ and kCounterLength. 2. Replace kCounterLength with sizeof(counter_). http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode55 crypto/encryptor.h:55: uint64 components64[2]; You can remove the 'buf' union member. You can add the following union member for future use: uint32 components32[4]; This will allow us to support 32-bit, 64-bit, and 128-bit counters. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode74 crypto/encryptor.h:74: // Update the counter value when in CTR mode. Currently only 128-bits Nit: Update => Updates or "Sets" if you rename the method as I suggested below. Please make the same changes to the function comments below. See the Style Guide for this recommendation on descriptive vs. imperative: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode78 crypto/encryptor.h:78: bool UpdateCounter(const std::string& counter); I believe this is a typical setter method. If so, it should be named SetCounter. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:30: return 0; Let's use return static_cast<CK_MECHANISM_TYPE>(-1); 0 is actually a valid mechanism (CKM_RSA_PKCS_KEY_PAIR_GEN). http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:97: GetMechanism(mode_), (mode_ == CTR ? CKA_ENCRYPT : CKA_DECRYPT), Are you sure we have to use CKA_ENCRYPT for CTR mode? http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:151: size_t output_len = input.size() + AES_BLOCK_SIZE; You can use the same formula as in GenerateCounterMask to estimate the output_len: size_t output_len = ((input.size() + AES_BLOCK_SIZE - 1) / AES_BLOCK_SIZE) * AES_BLOCK_SIZE; http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:38: // ECB mode encryption is only implemented using NSS. ECB => CTR
http://codereview.chromium.org/7056026/diff/54002/build/build_config.h File build/build_config.h (right): http://codereview.chromium.org/7056026/diff/54002/build/build_config.h#newcode97 build/build_config.h:97: #define ARCH_CPU_64_BITS 1 On 2011/06/24 18:06:06, wtc wrote: > Please define ARCH_CPU_LITTLE_ENDIAN here, repeated for the > three supported CPUs: > > #define ARCH_CPU_LITTLE_ENDIAN 1 > #undef ARCH_CPU_BIG_ENDIAN > > The #undef is not necessary and merely serves to document > what the opposite macro is, since we don't support any > big-endian architecture right now. > > Then we don't need the separate block on lines 111-117. Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc File crypto/encryptor.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode47 crypto/encryptor.cc:47: << "This code only runs in little endian system"; On 2011/06/24 18:06:06, wtc wrote: > Delete the runtime endianness check (lines 43-47). Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode50 crypto/encryptor.cc:50: memcpy(counter_.buf, counter.data(), kCounterLength); On 2011/06/24 18:06:06, wtc wrote: > This should be > memcpy(&counter_, counter.data(), counter.length()); Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode61 crypto/encryptor.cc:61: // Overflow occured then increment the most significant component. On 2011/06/24 18:06:06, wtc wrote: > Add "If". Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode70 crypto/encryptor.cc:70: memcpy(buf_ptr, counter_.buf, kCounterLength); On 2011/06/24 18:06:06, wtc wrote: > This function can simply say: > memcpy(buf, &counter_, sizeof(counter_)); Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.cc#newcode74 crypto/encryptor.cc:74: return counter_bits_ / 8; On 2011/06/24 18:06:06, wtc wrote: > This should be > return sizeof(counter_); Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h File crypto/encryptor.h (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode41 crypto/encryptor.h:41: void Increment(); On 2011/06/24 18:06:06, wtc wrote: > Nit: please make this method return bool, or add a TODO comment > about it. It should fail if the counter overflows. > > If you do this, GenerateCounterMask also needs to return > bool, and CryptCTR needs to check the return value of > GenerateCounterMask. Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode51 crypto/encryptor.h:51: size_t counter_bits_; On 2011/06/24 18:06:06, wtc wrote: > You misunderstood what I asked you to do with counter_bits_. > > There are two lengths of interest for a counter block. > 1. The counter block size, which must be equal to the size > of the block cipher. For AES, this is 128 bits. > 2. The number of counter bits in the counter block that > are incremented. In our first implementation we only > support 128 bits of counter. > > To make it simple, let's do the following in this CL: > > 1. Remove counter_bits_ and kCounterLength. > > 2. Replace kCounterLength with sizeof(counter_). Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode55 crypto/encryptor.h:55: uint64 components64[2]; On 2011/06/24 18:06:06, wtc wrote: > You can remove the 'buf' union member. > > You can add the following union member for future use: > uint32 components32[4]; > > This will allow us to support 32-bit, 64-bit, and 128-bit > counters. Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode74 crypto/encryptor.h:74: // Update the counter value when in CTR mode. Currently only 128-bits On 2011/06/24 18:06:06, wtc wrote: > Nit: Update => Updates > > or "Sets" if you rename the method as I suggested below. > > Please make the same changes to the function comments below. > See the Style Guide for this recommendation on descriptive > vs. imperative: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor.h#newcode78 crypto/encryptor.h:78: bool UpdateCounter(const std::string& counter); On 2011/06/24 18:06:06, wtc wrote: > I believe this is a typical setter method. If so, it should > be named SetCounter. Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:97: GetMechanism(mode_), (mode_ == CTR ? CKA_ENCRYPT : CKA_DECRYPT), On 2011/06/24 18:06:06, wtc wrote: > Are you sure we have to use CKA_ENCRYPT for CTR mode? Yup, decrypt does't work since we need to encrypt the counter to decrypt. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:151: size_t output_len = input.size() + AES_BLOCK_SIZE; On 2011/06/24 18:06:06, wtc wrote: > You can use the same formula as in GenerateCounterMask to > estimate the output_len: > > size_t output_len = ((input.size() + AES_BLOCK_SIZE - 1) / AES_BLOCK_SIZE) * > AES_BLOCK_SIZE; Done. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:38: // ECB mode encryption is only implemented using NSS. On 2011/06/24 18:06:06, wtc wrote: > ECB => CTR Done.
I don't think you should commit this yet. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc File crypto/encryptor_nss.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:30: return 0; On 2011/06/24 18:06:06, wtc wrote: > Let's use > return static_cast<CK_MECHANISM_TYPE>(-1); > > 0 is actually a valid mechanism (CKM_RSA_PKCS_KEY_PAIR_GEN). +1 http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_nss.cc#new... crypto/encryptor_nss.cc:151: size_t output_len = input.size() + AES_BLOCK_SIZE; No, CTR mode gives you a stream cipher. Unlike CBC mode, the ciphertext size need not be a multiple of the block size. You could pad it, of course, as with CBC mode, if you wanted to "blind" or "hide" the size of the plaintext somewhat. But it's ok and normal for the ciphertext to be the same length as the plaintext. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.cc File crypto/encryptor_unittest.cc (right): http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:47: const std::string kInitialCounter = "0000000000000000"; Since you make callers provide this value, chances are quite high that they will get it wrong — such as by following your example here. (Unit tests are where I would look for example calling code, especially since you don't provide documentation in encryptor.h.) In fact, people might feel required to set the counter to a constant starting value (after all, it's a *counter*, not a nonce or an IV or a key). That would be exactly wrong. Normally in CTR mode we use a nonce distinct from a counter to clarify the distinction between the nonce (random) and the counter (sequential and predictable). The nonce should be random because, for security reasons, any given (nonce, key) pair MUST only ever be used ONCE. (Same as any stream cipher.) Then, internally to CryptCTR, you concatenate the nonce and the counter. If the caller is smart and sets the counter to be completely random, they would get security but they might not be able to encrypt as many bytes as they had hoped to: there is no particular guaranteed range of the counter anymore. With distinct 64-bit nonce + 64-bit counter, the range is well-defined. (This is also a security guarantee, since you can't ever encrypt more than 2**64 messages (each with its own nonce) and each message can't be more than 2**64 blocks long. Those are generous limits, and it's important for callers to know what the limits are. In your scheme, the limits are not clear. If the caller is REALLY smart, they can set the counter to have the high-order 64 bits be random (the nonce), and the low-order bits be 0s (the counter). Then they could get the right behavior for themselves. But why require callers to be that smart, and to know the internals of your implementation that well? I would suggest using a 64-bit counter and a 64-bit nonce, and generating the nonce for the caller or at least documenting that it MUST be random. Follow that example in this unit test code. It might also make sense to make the interface clearer. Instead of Encryptor::Init(key, mode, iv) how about: Encryptor::InitCBC(key, iv) Encryptor::InitCTR(key, nonce) However, it would be ok to use the existing Encryptor::Init(key, mode, iv) interface and, in case of mode == CTR, expect nonce to be 64 random bytes (and document that expectation). Then you could get rid of Encryptor::UpdateCounter. http://codereview.chromium.org/7056026/diff/54002/crypto/encryptor_unittest.c... crypto/encryptor_unittest.cc:56: EXPECT_LT(0U, ciphertext.size()); Check for the exact ciphertext size. As I mentioned earlier, the ciphertext can be the exact same size as the plaintext; if you pad to an integer number of blocks, you'll know the exact size then, too.
palmer: This already landed in http://crrev.com/90425 I have added comments to Issue 87152 ( http://crbug.com/87152 ) providing more context about this code. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
