Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
797723006:
Implement PBKDF2 (except for generateKey) using BoringSSL (Closed)
DescriptionImplement PBKDF2 (except for generateKey) using BoringSSL.
The chromium-side implementation is https://codereview.chromium.org/820523003/.
BUG=399099
R=eroman
Committed: https://crrev.com/22a80e71d7199d9d51cbc1d54149cd48bc878e99
Cr-Commit-Position: refs/heads/master@{#312328}
Patch Set 1 #Patch Set 2 : File includes ordering #Patch Set 3 : Use PKCS5_PBKDF2_HMAC() from BoringSSL #
Total comments: 18
Patch Set 4 : Cleanup and add serialization methods #
Total comments: 10
Patch Set 5 : Added serialization and more accurate error messages #Patch Set 6 : Rebased #
Total comments: 14
Patch Set 7 : #
Total comments: 2
Patch Set 8 : Throw OperationError on importing empty password #
Total comments: 2
Patch Set 9 : Check for empty password when deriving keys instead #Patch Set 10 : Replace the use of std::vector::data() with &vector[0] #
Total comments: 4
Patch Set 11 : #
Messages
Total messages: 31 (4 generated)
PTAL.
not lgtm. Does BoringSSL's PKCS5_PBKDF2_HMAC() not already implement what we need? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
On 2015/01/07 06:36:57, eroman wrote: > not lgtm. > > Does BoringSSL's PKCS5_PBKDF2_HMAC() not already implement what we need? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring... I was not aware of this in boringssl :( I switched the implementation to PKCS5_PBKDF2_HMAC() and my Layout tests are passing.
https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/nss/util_nss.cc (right): https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/nss/util_nss.cc:105: // TODO(xsun): http://crbug.com/399099 Rather than a TODO you can leave this as: // PBKDF2 will only be implemented for BoringSSL, since the NSS implementation is being deprecated. (Since we have no plans of getting feature parity for the NSS implementation) https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:28: class Pbkdf2Implementation : public AlgorithmImplementation { style: Add a newline before class definition https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:72: const unsigned long iterations = params->iterations(); This should be an unsigned int, will comment on blink side changelist. That said, it isn't necessary to create an alias to params->iterations(). I think this code is more readable if the call to PKCS_PBKDF2_HMAC() just calls params->iterations() directly. Same for params->salt() and hash. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:79: unsigned int dkLen = optional_length_bits / 8; (1) dkLen is not chromium style naming. (20 dk_len is not a descriptive variable name. length_bytes or keylen_bytes would be more consistent with function parameters. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:80: unsigned int hLen = EVP_MD_size(digest_algorithm); style naming throughout here. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:82: unsigned long maxLength = ((1L << 32) - 1) * hLen; This doesn't make sense, what is it trying to enforce? dkLen should be 32-bit unsigned quantity (of bytes, which was given as a 32-bit quantity of bits). The only way to hit the operationerror below is if hlen=0 which also doesn't make sense. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:92: (const char*)password.data(), password.size(), salt.data(), salt.size(), (1) use C++ style casts, like static_cast<const char*> or reinterpret_cast<const char*> (2) There needs to be a test to prevent underflowing password.size(). password.size() is an unsigned quantity, however the crypto library expects a signed value. If it were to get -1 it will do something very very bad (treat the input as a null-terminated C-string, which it is not). Use base::CheckedNumeric<> to test first. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:96: else nit: we omit "else" clauses when the "if" above returns, so delete this. Or alternately: return result == 1 ? Status::Succes() : Status::OperationError(); https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:98: } The Serialize methods are needed here as well, since the KDFs support structured cloning. See the comments I left for Nick's change, which is implementing a very similar algorithm (https://codereview.chromium.org/803173010/)
Revised according to @eroman's comments. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/nss/util_nss.cc (right): https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/nss/util_nss.cc:105: // TODO(xsun): http://crbug.com/399099 On 2015/01/09 02:26:21, eroman wrote: > Rather than a TODO you can leave this as: > > // PBKDF2 will only be implemented for BoringSSL, since the NSS implementation > is being deprecated. > > > (Since we have no plans of getting feature parity for the NSS implementation) Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:28: class Pbkdf2Implementation : public AlgorithmImplementation { On 2015/01/09 02:26:21, eroman wrote: > style: Add a newline before class definition Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:72: const unsigned long iterations = params->iterations(); On 2015/01/09 02:26:21, eroman wrote: > This should be an unsigned int, will comment on blink side changelist. > > That said, it isn't necessary to create an alias to params->iterations(). I > think this code is more readable if the call to PKCS_PBKDF2_HMAC() just calls > params->iterations() directly. Same for params->salt() and hash. Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:79: unsigned int dkLen = optional_length_bits / 8; On 2015/01/09 02:26:21, eroman wrote: > (1) dkLen is not chromium style naming. > (20 dk_len is not a descriptive variable name. length_bytes or keylen_bytes > would be more consistent with function parameters. Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:80: unsigned int hLen = EVP_MD_size(digest_algorithm); On 2015/01/09 02:26:21, eroman wrote: > style naming throughout here. Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:82: unsigned long maxLength = ((1L << 32) - 1) * hLen; On 2015/01/09 02:26:21, eroman wrote: > This doesn't make sense, what is it trying to enforce? > > dkLen should be 32-bit unsigned quantity (of bytes, which was given as a 32-bit > quantity of bits). > > The only way to hit the operationerror below is if hlen=0 which also doesn't > make sense. Section 5.2 in https://www.ietf.org/rfc/rfc2898.txt. But I agree this is moot since dkLen is 32-bit. So this check is removed. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:92: (const char*)password.data(), password.size(), salt.data(), salt.size(), On 2015/01/09 02:26:21, eroman wrote: > (1) use C++ style casts, like static_cast<const char*> or reinterpret_cast<const > char*> > > (2) There needs to be a test to prevent underflowing password.size(). > > password.size() is an unsigned quantity, however the crypto library expects a > signed value. If it were to get -1 it will do something very very bad (treat the > input as a null-terminated C-string, which it is not). > > Use base::CheckedNumeric<> to test first. > Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:96: else On 2015/01/09 02:26:21, eroman wrote: > nit: we omit "else" clauses when the "if" above returns, so delete this. > > Or alternately: > > return result == 1 ? Status::Succes() : Status::OperationError(); Done. https://codereview.chromium.org/797723006/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:98: } On 2015/01/09 02:26:21, eroman wrote: > The Serialize methods are needed here as well, since the KDFs support structured > cloning. > > See the comments I left for Nick's change, which is implementing a very similar > algorithm (https://codereview.chromium.org/803173010/) Done.
https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:28: Status GenerateKey(const blink::WebCryptoAlgorithm& algorithm, You can remove this alltogether since the default will throw a NotSupportedError too. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:84: reinterpret_cast<const char*>(password.data()), I am not sure that all chromium platforms support std::vector::data, as it is new to C++11, certainly I have had trouble with this in the past. I guess can see if it makes it past trybots. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:109: } This algorithm also needs an implementation for GetKeyLength(), and corresponding tests which exercise that. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... File content/child/webcrypto/status.cc (right): https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/status.cc:334: "Length for PBKDF2 key derivation must be a multiple of 8 bits."); This is also used in the situation where a length was not provided. Please distinguish the two errors.
https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:65: You also need a call to crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE); to clear the openssl error stack. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... File content/child/webcrypto/status.cc (right): https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/status.cc:334: "Length for PBKDF2 key derivation must be a multiple of 8 bits."); On 2015/01/13 00:19:20, eroman wrote: > This is also used in the situation where a length was not provided. Please > distinguish the two errors. In the case of the latter, HKDF change proposes: "No length was specified for the HKDF Derive Bits operation." Please model that for consistency. See: https://codereview.chromium.org/803173010/diff/160001/content/child/webcrypto...
PTAL https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:28: Status GenerateKey(const blink::WebCryptoAlgorithm& algorithm, On 2015/01/13 00:19:20, eroman wrote: > You can remove this alltogether since the default will throw a NotSupportedError > too. Done. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:65: On 2015/01/13 00:30:17, eroman wrote: > You also need a call to > > crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE); > > to clear the openssl error stack. Done. https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:84: reinterpret_cast<const char*>(password.data()), On 2015/01/13 00:19:20, eroman wrote: > I am not sure that all chromium platforms support std::vector::data, as it is > new to C++11, certainly I have had trouble with this in the past. > > I guess can see if it makes it past trybots. Yeah let's see what happens. Is making a copy the only alternative? https://codereview.chromium.org/797723006/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/pbkdf2_openssl.cc:109: } On 2015/01/13 00:19:20, eroman wrote: > This algorithm also needs an implementation for GetKeyLength(), and > corresponding tests which exercise that. Implemented GetKeyLength(). How show I test it?
LGTM. Also cc-ing David to verify the use of BoringSSL https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:79: base::CheckedNumeric<int> password_size = password.size(); Can you add a comment that BoringSSL expects the size as an int, and will interpret the data as a C-String if it is -1. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:94: Status GetKeyLength(const blink::WebCryptoAlgorithm& key_length_algorithm, nit: For consistency with hdkf_openssl.cc, can you move this definition below DeserializeKeyForClone? https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/status.h (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/status.h:264: // The requested bit length for PBKDF2 key derivation was invalid. nit: Add a newline separating from previous line.
davidben@chromium.org changed reviewers: + davidben@chromium.org
BoringSSL bit lgtm with a style nit, but get Eric's answer on the two questions below. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:86: params->salt().size(), params->iterations(), digest_algorithm, Note: you'll probably end up spinning indefinitely in C++ if iterations is, say, UINT_MAX. Probably more a question for Eric, but is something WebCrypto considers okay? https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:87: keylen_bytes, derived_bytes->data()); Is std::vector::data something we can rely on in all our platforms? I haven't been paying attention to whether WebCrypto has been fine with that. (Also probably a question for Eric.) It's a library feature new with C++11. (Used here and on password. salt is a WebVector, so that one seems fine.) https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:89: if (result == 1) Style nit: PKCS5_PBKDF2_HMAC has been fixed to return only 0 and 1 in BoringSSL. You can probably just write this as if (!PKCS5_PBKDF2_HMAC(stuff)) { return Status::OperationError(); } return Status::Success();
https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:86: params->salt().size(), params->iterations(), digest_algorithm, On 2015/01/14 23:42:30, David Benjamin wrote: > Note: you'll probably end up spinning indefinitely in C++ if iterations is, say, > UINT_MAX. Probably more a question for Eric, but is something WebCrypto > considers okay? (Sorry, not indefinitely but for a rather long time.)
https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:86: params->salt().size(), params->iterations(), digest_algorithm, On 2015/01/14 23:43:03, David Benjamin wrote: > On 2015/01/14 23:42:30, David Benjamin wrote: > > Note: you'll probably end up spinning indefinitely in C++ if iterations is, > say, > > UINT_MAX. Probably more a question for Eric, but is something WebCrypto > > considers okay? > > (Sorry, not indefinitely but for a rather long time.) I don't believe it is a problem that needs to be solved -- other parts of WebCrypto can similarly DOS the renderer. At a more basic level you can write a webpage that does a while (1) {alert();} and hang the renderer, so meh, WorkingAsIntended :) That said, WebCrypto is a bit different in the following regard: The crypto operations are run on a dedicated worker thread, and the result communicated asynchronously via Promises. So what would happen in this case is all of your WebCrypto operations would stall for the renderer. This scenario does deserve some extra thought for developer debuggability however. I will file a bug and followup there. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:87: keylen_bytes, derived_bytes->data()); On 2015/01/14 23:42:30, David Benjamin wrote: > Is std::vector::data something we can rely on in all our platforms? I haven't > been paying attention to whether WebCrypto has been fine with that. (Also > probably a question for Eric.) > > It's a library feature new with C++11. (Used here and on password. salt is a > WebVector, so that one seems fine.) Yeah I raised this earlier as well. Other code in WebCrypto does not use std::vector::data() because as you say, it was unavailable. I figure if it passes trybots then great the situation has changed (the holdout IIRC was windows compilers, but the last time I observed this was months ago) If you do need to remove use of data() you will need to be careful to preserve the same semantics, since these vectors can be empty.
PTAL. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:79: base::CheckedNumeric<int> password_size = password.size(); On 2015/01/14 20:44:58, eroman wrote: > Can you add a comment that BoringSSL expects the size as an int, and will > interpret the data as a C-String if it is -1. Done. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:87: keylen_bytes, derived_bytes->data()); On 2015/01/15 01:32:06, eroman wrote: > On 2015/01/14 23:42:30, David Benjamin wrote: > > Is std::vector::data something we can rely on in all our platforms? I haven't > > been paying attention to whether WebCrypto has been fine with that. (Also > > probably a question for Eric.) > > > > It's a library feature new with C++11. (Used here and on password. salt is a > > WebVector, so that one seems fine.) > > Yeah I raised this earlier as well. Other code in WebCrypto does not use > std::vector::data() because as you say, it was unavailable. > > I figure if it passes trybots then great the situation has changed (the holdout > IIRC was windows compilers, but the last time I observed this was months ago) > > If you do need to remove use of data() you will need to be careful to preserve > the same semantics, since these vectors can be empty. Acknowledged. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:89: if (result == 1) On 2015/01/14 23:42:30, David Benjamin wrote: > Style nit: PKCS5_PBKDF2_HMAC has been fixed to return only 0 and 1 in BoringSSL. > You can probably just write this as > > if (!PKCS5_PBKDF2_HMAC(stuff)) { > return Status::OperationError(); > } > return Status::Success(); Done. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:94: Status GetKeyLength(const blink::WebCryptoAlgorithm& key_length_algorithm, On 2015/01/14 20:44:58, eroman wrote: > nit: For consistency with hdkf_openssl.cc, can you move this definition below > DeserializeKeyForClone? Done. https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... File content/child/webcrypto/status.h (right): https://codereview.chromium.org/797723006/diff/100001/content/child/webcrypto... content/child/webcrypto/status.h:264: // The requested bit length for PBKDF2 key derivation was invalid. On 2015/01/14 20:44:58, eroman wrote: > nit: Add a newline separating from previous line. Done.
patchset 7 LGTM
https://codereview.chromium.org/797723006/diff/120001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/120001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:76: const std::vector<uint8_t>& password = Note there is an issue with empty passwords: https://code.google.com/p/chromium/issues/detail?id=449409 For now I suggest throwing an OperationError if the password is empty, until we address it in BoringSSL.
https://codereview.chromium.org/797723006/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:48: return Status::ErrorPbkdf2EmptyPassword(); Actually I had envisioned this being part of deriveBits not importKey(), since that is where the data is used. Also rather than FIXME the chromium style is TODO(xxx).
https://codereview.chromium.org/797723006/diff/120001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/120001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:76: const std::vector<uint8_t>& password = On 2015/01/16 04:17:45, eroman wrote: > Note there is an issue with empty passwords: > https://code.google.com/p/chromium/issues/detail?id=449409 > > For now I suggest throwing an OperationError if the password is empty, until we > address it in BoringSSL. Done. https://codereview.chromium.org/797723006/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:48: return Status::ErrorPbkdf2EmptyPassword(); On 2015/01/16 05:10:28, eroman wrote: > Actually I had envisioned this being part of deriveBits not importKey(), since > that is where the data is used. Also rather than FIXME the chromium style is > TODO(xxx). Done.
lgtm thanks for making the changes
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797723006/160001
The CQ bit was unchecked by eroman@chromium.org
Try bot failed because std::vector::data() cannot be used.
On 2015/01/20 17:14:38, eroman wrote: > Try bot failed because std::vector::data() cannot be used. Replaced it with &vector.at(0) assuming internal representation is on continuous memory, as we do in some other places in Chromium.
lgtm after addressing these comments https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:96: if (!PKCS5_PBKDF2_HMAC(reinterpret_cast<const char*>(&password.at(0)), For consistency with other WebCrypto code, and so this works more smoothly once we remove the check on password length above, could you write this as: const char* password_ptr = password.empty() ? NULL : reinterpret_cast<const char*>(&password[0]); https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:100: &derived_bytes->at(0))) nit: could you write this as &derived_bytes->front() for consistency with other code
PTAL. https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... File content/child/webcrypto/openssl/pbkdf2_openssl.cc (right): https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:96: if (!PKCS5_PBKDF2_HMAC(reinterpret_cast<const char*>(&password.at(0)), On 2015/01/21 04:57:35, eroman wrote: > For consistency with other WebCrypto code, and so this works more smoothly once > we remove the check on password length above, could you write this as: > > const char* password_ptr = password.empty() ? NULL : reinterpret_cast<const > char*>(&password[0]); Done. https://codereview.chromium.org/797723006/diff/180001/content/child/webcrypto... content/child/webcrypto/openssl/pbkdf2_openssl.cc:100: &derived_bytes->at(0))) On 2015/01/21 04:57:35, eroman wrote: > nit: could you write this as &derived_bytes->front() for consistency with other > code Done.
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797723006/200001
On 2015/01/21 05:15:40, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/797723006/200001 Patch blocked on throttled CQ?
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/22a80e71d7199d9d51cbc1d54149cd48bc878e99 Cr-Commit-Position: refs/heads/master@{#312328} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
