Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
396463004:
Implement NetworkingPrivateCrypto for OpenSSL. (Closed)
|
Created:
6 years, 5 months ago by davidben Modified:
6 years, 5 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionImplement NetworkingPrivateCrypto for OpenSSL.
Based on the NSS implementation and loosely on the original OpenSSL code at
https://chromium.googlesource.com/chromiumos/platform/shill/+/master/shims/crypto_util.cc
BUG=393023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285494
Patch Set 1 #
Total comments: 12
Patch Set 2 : rsleevi comments #
Total comments: 13
Patch Set 3 : sleevi comments #
Total comments: 6
Patch Set 4 : Check for zero-length RSA keys to avoid new[0]. #
Total comments: 2
Patch Set 5 : Update for BoringSSL. #Patch Set 6 : rebase #Patch Set 7 : Rebase across gyp/gn changes. #
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/396463004/diff/1/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/396463004/diff/1/chrome/chrome_common.gypi#ne... chrome/chrome_common.gypi:356: '../third_party/openssl/openssl.gyp:openssl', Without this it silently includes the system OpenSSL headers or something on my machine, which is kinda scary. (Makes me wonder if we accidentally did that anywhere for the Android build...) https://codereview.chromium.org/396463004/diff/1/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (left): https://codereview.chromium.org/396463004/diff/1/chrome/chrome_tests_unit.gyp... chrome/chrome_tests_unit.gypi:410: 'common/extensions/api/networking_private/networking_private_crypto_unittest.cc', Test doesn't seem to run when listed here. I'm guessing the targets got reorganized and the test was lost.
https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:105: common_name_length + 1); Use base::Writeinto if you're going to subvert the STL (it's fine, but let's keep our subversions to a minimum :P) https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:140: #endif // OPENSSL_IS_BORINGSSL Please extract this into a variable, set up on line 126, rather than being embedded in the call site. Even if it is going away soon. https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:157: const uint8* ptr = &pub_key_der[0]; CRASH BUG: if pub_key_der is .empty(). The NSS code is safe against this. (Then again, the NSS code is using vector<...>.data(), which isn't defined until C++11... Subtle but "yay") https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:167: &(*encrypted_output)[0], &encrypted_output->front() ? https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:203: decrypted_output->resize(RSA_size(rsa.get())); base::WriteInto() Please note this is not preserving the same invariants as the NSS code, namely that |decrypted_output| is not modified in case of error. You're potentially leaving unverified data in there, even after returning false. We normally try to avoid this in APIs (hecne the NSS code making the extra copy). Ditto line 174.
https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:105: common_name_length + 1); On 2014/07/14 20:29:30, Ryan Sleevi wrote: > Use base::Writeinto if you're going to subvert the STL (it's fine, but let's > keep our subversions to a minimum :P) Handy. I didn't realize that existed. Although it doesn't seem to handle the empty case either. :-/ https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:140: #endif // OPENSSL_IS_BORINGSSL On 2014/07/14 20:29:30, Ryan Sleevi wrote: > Please extract this into a variable, set up on line 126, rather than being > embedded in the call site. > > Even if it is going away soon. Done. https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:157: const uint8* ptr = &pub_key_der[0]; On 2014/07/14 20:29:30, Ryan Sleevi wrote: > CRASH BUG: if pub_key_der is .empty(). The NSS code is safe against this. > > (Then again, the NSS code is using vector<...>.data(), which isn't defined until > C++11... Subtle but "yay") Done. (Verified experimentally that OpenSSL is fine with NULL, 0 and, after much digging, found the place where it does that check... this ASN.1 code is insane.) https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:167: &(*encrypted_output)[0], On 2014/07/14 20:29:30, Ryan Sleevi wrote: > &encrypted_output->front() ? Done. https://codereview.chromium.org/396463004/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:203: decrypted_output->resize(RSA_size(rsa.get())); On 2014/07/14 20:29:30, Ryan Sleevi wrote: > base::WriteInto() > > Please note this is not preserving the same invariants as the NSS code, namely > that |decrypted_output| is not modified in case of error. > > You're potentially leaving unverified data in there, even after returning false. > We normally try to avoid this in APIs (hecne the NSS code making the extra > copy). > > Ditto line 174. Well, this is the last operation, so only assuming OpenSSL itself doesn't preserve that invariant. I believe the default implementation actually does. *shrug* Anyway, done.
https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gyp... chrome/chrome_common.gypi:344: 'common/extensions/api/networking_private/networking_private_crypto.cc', GN updates? https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto.h:55: static const size_t kTrustedCAPublicKeyDERLength; Document, if it's in a public header. https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:53: std::vector<uint8> cert_data; uint8 (Chromium-specific) is deprecated. Use uint8_t - C99 - (and include stdint.h) (Yes, it's a bug in this existing interface). https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:62: if (!cert || ptr != (&cert_data[0] + cert_data.size())) { still unsafe here. Just const uint8* ptr = cert_data.empty() ? NULL : &cert_data[0]; const uint8* end = ptr + cert_data.size(); if (!cert || ptr != end) { https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:104: common_name_length + 1); I'm guessing this is only safe because of line 85. IIRC, OpenSSL doesn't validate the length of subtypes until it attempts to deserialize, meaning an attacker might be able to spoof a large length field (but small actual data) https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:160: if (!rsa || ptr != (&pub_key_der[0] + pub_key_der.size())) { ditto BUG
https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gyp... chrome/chrome_common.gypi:344: 'common/extensions/api/networking_private/networking_private_crypto.cc', On 2014/07/17 17:26:29, Ryan Sleevi wrote: > GN updates? Looks like this doesn't exist yet IN. I'll fill out the GN target soon.
https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/396463004/diff/20001/chrome/chrome_common.gyp... chrome/chrome_common.gypi:344: 'common/extensions/api/networking_private/networking_private_crypto.cc', On 2014/07/17 19:44:38, brettw wrote: > On 2014/07/17 17:26:29, Ryan Sleevi wrote: > > GN updates? > > Looks like this doesn't exist yet IN. I'll fill out the GN target soon. In the meantime, I've left this for now. Probably going to be holding on this CL in case the next BoringSSL land goes through. If the blocks get added to BUILD.gn by the time this lands, I'll update it. https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto.h:55: static const size_t kTrustedCAPublicKeyDERLength; On 2014/07/17 17:26:29, Ryan Sleevi wrote: > Document, if it's in a public header. Done. https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:53: std::vector<uint8> cert_data; On 2014/07/17 17:26:29, Ryan Sleevi wrote: > uint8 (Chromium-specific) is deprecated. Use uint8_t - C99 - (and include > stdint.h) > > (Yes, it's a bug in this existing interface). Done. https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:62: if (!cert || ptr != (&cert_data[0] + cert_data.size())) { On 2014/07/17 17:26:29, Ryan Sleevi wrote: > still unsafe here. > > Just > const uint8* ptr = cert_data.empty() ? NULL : &cert_data[0]; > const uint8* end = ptr + cert_data.size(); > > if (!cert || ptr != end) { Done. (Well, size = 0 => cert = NULL, but sure.) https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:104: common_name_length + 1); On 2014/07/17 17:26:29, Ryan Sleevi wrote: > I'm guessing this is only safe because of line 85. IIRC, OpenSSL doesn't > validate the length of subtypes until it attempts to deserialize, meaning an > attacker might be able to spoof a large length field (but small actual data) For this function, the length is correct the first time. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss... But I've put the resize back in to be safe. https://codereview.chromium.org/396463004/diff/20001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:160: if (!rsa || ptr != (&pub_key_der[0] + pub_key_der.size())) { On 2014/07/17 17:26:29, Ryan Sleevi wrote: > ditto BUG Done.
LGTM % SECURITY nit. https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:10: #endif Platform includes go at the end (line 21), even for C headers http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:106: DCHECK_EQ((int)common_name.size(), common_name_length); What's this a DCHECK against? Developer/library error? Doesn't seem like it; seems like attacker data. Meaning either we CHECK or, like on line 107, we handle it and fail gracefully. https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:168: scoped_ptr<uint8_t[]> rsa_output(new uint8_t[RSA_size(rsa.get())]); SECURITY: Can RSA_size() return 0 for this key? Seems like it (potentially attacker data?). If so, new uint8_t[0] would be no bueno.
https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:10: #endif On 2014/07/17 22:24:07, Ryan Sleevi wrote: > Platform includes go at the end (line 21), even for C headers > > http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code Leaving this for now since I'm not planning on landing this after BoringSSL anyway (probably best to minimize turbulence there and just let that get through), so the ifdefs will be gone. And splitting up the {Open,Boring}SSL includes looks silly. https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:106: DCHECK_EQ((int)common_name.size(), common_name_length); On 2014/07/17 22:24:07, Ryan Sleevi wrote: > What's this a DCHECK against? > > Developer/library error? Doesn't seem like it; seems like attacker data. Meaning > either we CHECK or, like on line 107, we handle it and fail gracefully. I added it for library error, not attacker data. X509_NAME_get_text_by_NID cannot return a different value. If it does, we behave gracefully. But it shouldn't happen. I don't particularly care... I can just remove it altogether if you prefer. Neither line 107 nor 111 matters, but it handles it all the same. See the implementation. By the time it does the buf == NULL check, it has already determined the return value. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss... https://codereview.chromium.org/396463004/diff/40001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:168: scoped_ptr<uint8_t[]> rsa_output(new uint8_t[RSA_size(rsa.get())]); On 2014/07/17 22:24:07, Ryan Sleevi wrote: > SECURITY: Can RSA_size() return 0 for this key? > > Seems like it (potentially attacker data?). If so, new uint8_t[0] would be no > bueno. Lovely. d2i_RSAPublicKey does not validate its input in the slightest. It just uses the generic ASN.1 support and happily parses out anything with two BIGNUMs. It'll never give back negative values though, so that's something. (Quoth OpenSSL: "Currently it ignores the sign which isn't a problem since all BIGNUMs used are non negative and anything that looks negative is normally due to an encoding error.") But zero is possible. Added a check for that. Anything else, ideally RSA_public_encrypt would handle correctly (well, "correctly"; if the key is garbage, the output may be garbage, but it shouldn't explode). I actually see one place where it doesn't, but I'll go fix that and audit the rest within Boring. (More reason this should wait for the cutover.)
Updated for BoringSSL, so there's no more #ifdef. Also made it use a crypto::ScopedEVP_MD_CTX. +kalman for OWNERS. Won't land this until Monday so we can be sure the BoringSSL switchover stuck this time.
lgtm
lgtm lgtm https://codereview.chromium.org/396463004/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/60001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:96: if (common_name_length < 0) { a bit weird that common_name_length == 0 is excluded here.
https://codereview.chromium.org/396463004/diff/60001/chrome/common/extensions... File chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc (right): https://codereview.chromium.org/396463004/diff/60001/chrome/common/extensions... chrome/common/extensions/api/networking_private/networking_private_crypto_openssl.cc:96: if (common_name_length < 0) { On 2014/07/19 17:12:16, mef wrote: > a bit weird that common_name_length == 0 is excluded here. If you're just trying to get the common name out, -1 is how it signals error. I guess 0 would also be bogus, but it'd still properly give back an empty string; the EndsWith check will fail later.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/396463004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32212) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/37155)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/32277)
+jochen for chrome/ OWNERS. Build-related changes.
lgtm
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/396463004/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/396463004/120001
Message was sent while issue was closed.
Change committed as 285494 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
