Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
75653002:
[webcrypto] Add RSAES-PKCS1-v1_5 encrypt and decrypt for NSS. (Closed)
|
Created:
7 years, 1 month ago by padolph Modified:
7 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[webcrypto] Add RSAES-PKCS1-v1_5 encrypt and decrypt for NSS.
BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237746
Patch Set 1 #
Total comments: 17
Patch Set 2 : rebase #Patch Set 3 : fixes for eroman #
Total comments: 4
Patch Set 4 : rebase #Patch Set 5 : added known-answer test #Patch Set 6 : fixed write past array end in unit test #Patch Set 7 : restored #ifdef to make android build succeed #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:268: if (!key.type() == blink::WebCryptoKeyTypePublic) I imagine you meant to use != (since as written this expression doesn't make much sense). This is an interesting candidate for a clang compiler warning; I will ask thakis about it. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:269: return false; // TODO(padolph): should be DCHECK? Can remove the TODO: should not be a DCHECK, since the blink side does not validate this aspect of the keys before calling into here. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:279: if (encrypted_length_bytes < data_size + 11) paranoia: what if data_size + 11 overflows? Could lead to PubEncryptPKCS1 writing beyond the buffer. Could you express this in an overflow safe manner, perhaps: if (encrypted_length_bytes < 11 || encrypted_length_bytes - 11 < data_size) return false; https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:320: if (!key.type() == blink::WebCryptoKeyTypePrivate) Same comment as above. You surely meant either X != Y or !(X == Y) https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:328: if (modulus_length_bytes < 0) // TODO(padolph): should be a DCHECK only? Should this check <= 0? https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:331: const unsigned& max_output_length_bytes = modulus_length_bytes; Rather than a reference, either make a copy or remove this variable all together. (No performance or readability improvement by using a reference that I can tell). https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_unittest.cc:896: // Do a passing encrypt to generate data for decrypt. Not sure what a "passing encrypt" means
https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:268: if (!key.type() == blink::WebCryptoKeyTypePublic) On 2013/11/21 00:18:26, eroman wrote: > I imagine you meant to use != (since as written this expression doesn't make > much sense). > > This is an interesting candidate for a clang compiler warning; I will ask thakis > about it. FYI: Nico showed me that clang already has a warning for this: -Wlogical-not-parentheses
https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:268: if (!key.type() == blink::WebCryptoKeyTypePublic) On 2013/11/21 00:18:26, eroman wrote: > I imagine you meant to use != (since as written this expression doesn't make > much sense). Doh! Not sure how that happened. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:268: if (!key.type() == blink::WebCryptoKeyTypePublic) On 2013/11/21 01:02:15, eroman wrote: > On 2013/11/21 00:18:26, eroman wrote: > > I imagine you meant to use != (since as written this expression doesn't make > > much sense). > > > > This is an interesting candidate for a clang compiler warning; I will ask > thakis > > about it. > > FYI: Nico showed me that clang already has a warning for this: > -Wlogical-not-parentheses Thanks. I only use clang sometimes. I find it _much_ slower than gcc on my system. I will try to run it more often, especially on fresh code like this. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:279: if (encrypted_length_bytes < data_size + 11) On 2013/11/21 00:18:26, eroman wrote: > paranoia: what if data_size + 11 overflows? Could lead to PubEncryptPKCS1 > writing beyond the buffer. > > Could you express this in an overflow safe manner, perhaps: > > if (encrypted_length_bytes < 11 || encrypted_length_bytes - 11 < data_size) > return false; Done. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:279: if (encrypted_length_bytes < data_size + 11) On 2013/11/21 00:18:26, eroman wrote: > paranoia: what if data_size + 11 overflows? Could lead to PubEncryptPKCS1 > writing beyond the buffer. > > Could you express this in an overflow safe manner, perhaps: > > if (encrypted_length_bytes < 11 || encrypted_length_bytes - 11 < data_size) > return false; Done. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:320: if (!key.type() == blink::WebCryptoKeyTypePrivate) On 2013/11/21 00:18:26, eroman wrote: > Same comment as above. You surely meant either X != Y or !(X == Y) Done. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:320: if (!key.type() == blink::WebCryptoKeyTypePrivate) On 2013/11/21 00:18:26, eroman wrote: > Same comment as above. You surely meant either X != Y or !(X == Y) Done. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:328: if (modulus_length_bytes < 0) // TODO(padolph): should be a DCHECK only? On 2013/11/21 00:18:26, eroman wrote: > Should this check <= 0? NSS returns -1 in case of error. Returning 0 is a non-error for PK11_GetPrivateModulusLen() but would indicate a programming error around the use of keys with this method. However, I see your point, this distinction is not very important for this method. Changed. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_nss.cc:331: const unsigned& max_output_length_bytes = modulus_length_bytes; On 2013/11/21 00:18:26, eroman wrote: > Rather than a reference, either make a copy or remove this variable all > together. (No performance or readability improvement by using a reference that I > can tell). Done. https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/75653002/diff/1/content/renderer/webcrypto/we... content/renderer/webcrypto/webcrypto_impl_unittest.cc:896: // Do a passing encrypt to generate data for decrypt. On 2013/11/21 00:18:26, eroman wrote: > Not sure what a "passing encrypt" means Done.
https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:261: } else if (algorithm.id() == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5) { @rsleevi: Can you review this file as well? (Since I am a crypto and NSS neophyte) https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:800: // NIST vectors, because the random data in PKCS1.5 padding makes the encryption @rsleevi: Any ideas?
https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:800: // NIST vectors, because the random data in PKCS1.5 padding makes the encryption On 2013/11/21 02:07:01, eroman wrote: > @rsleevi: Any ideas? Yeah, there's no good way here. The NSS unittests for this (bltest) are pretty gross. They don't really check the PKCS1.5 padding, just the result of the RSA raw primitive (which is less than ideal). I've improved this with the PSS and OAEP tests, but at the cost of exporting additional functions (that are meant *only* for use by bltests, not to be part of the 'public' API) A way to do a KAT would be to take a known-good message, decrypt, then re-encrypt, then decrypt again. Best I can think of short of using the bltest functions to explicitly set the seed/padding.
https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/75653002/diff/100001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:800: // NIST vectors, because the random data in PKCS1.5 padding makes the encryption On 2013/11/21 02:17:02, Ryan Sleevi wrote: > On 2013/11/21 02:07:01, eroman wrote: > > @rsleevi: Any ideas? > > Yeah, there's no good way here. The NSS unittests for this (bltest) are pretty > gross. They don't really check the PKCS1.5 padding, just the result of the RSA > raw primitive (which is less than ideal). I've improved this with the PSS and > OAEP tests, but at the cost of exporting additional functions (that are meant > *only* for use by bltests, not to be part of the 'public' API) > > A way to do a KAT would be to take a known-good message, decrypt, then > re-encrypt, then decrypt again. Best I can think of short of using the bltest > functions to explicitly set the seed/padding. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/75653002/190001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/75653002/190001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/75653002/190001
Failed to trigger a try job on mac_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/75653002/210001
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/75653002/230001
Message was sent while issue was closed.
Change committed as 237746 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
