Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
66213002:
NSS: {EC,RSA}PrivateKey shouldn't call crypto::GetPublicNSSKeySlot or GetPrivateNSSKeySlot. (Closed)
|
Created:
7 years, 1 month ago by mattm Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Sleevi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNSS: {EC,RSA}PrivateKey shouldn't call crypto::GetPublicNSSKeySlot or GetPrivateNSSKeySlot.
Make ECPrivateKey use PK11_GetInternalKeySlot for temporary keys.
Make ECPrivateKey and RSAPrivateKey "sensitive" functions take slot as parameter.
This avoids calling non-thread-safe functions in nss_util on arbitrary threads.
Also removes the ANNOTATE_SCOPED_MEMORY_LEAK from RSAPrivateKey which should no longer be necessary.
BUG=125848, 34742
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234726
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : gyp fixes #
Total comments: 54
Patch Set 5 : review changes up to comment #6 #Patch Set 6 : compile fix #Patch Set 7 : rebase #Patch Set 8 : remove the checks again #
Messages
Total messages: 14 (0 generated)
I started looking at some of the thread safety issues with nss_util (see also https://codereview.chromium.org/64723006). In order to avoid calling GetPublicNSSKeySlot or GetPrivateNSSKeySlot on worker threads or whatnot, we need to refactor things so that we can get the slot on the IO thread and then pass it along to whatever needs to use it on other threads. (In this case, nothing actually calls the "sensitive" methods on ECPrivateKey or RSAPrivateKey, so I didn't need to update any call sites for those.)
https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); Pretty sure ChromeOS is relying on this for TPM provisioning
https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); On 2013/11/09 02:26:47, Ryan Sleevi wrote: > Pretty sure ChromeOS is relying on this for TPM provisioning Hm, I found "platform/login_manager" uses RSAPrivateKey, though not the CreateSensitive functions: https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... However, a few test related things do use CreateSensitive: https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... I guess a 3-sided patch would be needed to update those..
Patch set 4 LGTM. IMPORTANT: is this CL complete? I expected to see changes to Chrome OS or Chrome files that create permanent keys because now they need to pass the new |slot| argument. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h File crypto/ec_private_key.h (right): https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:17: #endif Move this to the #else block on lines 22-27. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:50: // The created key is permanent and is not exportable in plaintext form. Nit: mention the new |slot| argument. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:52: // NOTE: Currently only available if USE_NSS is defined. Nit: perhaps we can delete this comment now that the function is inside #if defined(USE_NSS). https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:72: // NOTE: Currently only available if USE_NSS is defined. Nit: perhaps we can delete this comment now that the function is inside #if defined(USE_NSS). https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... File crypto/ec_private_key_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:36: crypto::ScopedPK11Slot slot(PK11_GetInternalKeySlot()); It may be a good idea to keep the GetKeySlot() function, so that we know the slot we test for PKCS #11 mechanism support here will be the slot we use in the rest of this file. We can change GetKeySlot() to return PK11_GetInternalKeySlot() if crypto::GetPublicNSSKeySlot() is not appropriate. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:89: return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()), IMPORTANT: I think this should be PK11_GetInternalSlot(). I explained this issue in rsa_private_key_nss.cc. But I am less sure in this file because the original code in this file uses crypto::GetPublicNSSKeySlot(), which is defined as: PK11SlotInfo* GetPublicNSSKeySlot() { if (test_slot_) return PK11_ReferenceSlot(test_slot_); if (software_slot_) return PK11_ReferenceSlot(software_slot_); return PK11_GetInternalKeySlot(); } So PK11_GetInternalKeySlot() may be more appropriate in this file. Also, why don't we need to the overrides by test_slot_ and software_slot_? https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:91: PR_FALSE /* not sensitive */); Nit: these should be 'false' instead of 'PR_FALSE' because they are of the C++ bool type. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:98: slot.Pass(), PR_TRUE /* permanent */, PR_TRUE /* sensitive */); Nit: these should be 'true' instead of 'PR_TRUE'. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:110: ScopedPK11Slot(PK11_GetInternalKeySlot()), IMPORTANT: I think this should be PK11_GetInternalSlot(). https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:115: PR_FALSE /* not sensitive */); Nit: these should be 'false' instead of 'PR_FALSE'. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:131: PR_TRUE /* sensitive */); Nit: these should be 'true' instead of 'PR_TRUE'. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h File crypto/rsa_private_key.h (right): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:27: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) Optional: This can become #else. Then you can move the #include "crypto/scoped_nss_types.h" into this block. Note: there are two other instances of this in this file. I pointed them out below. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:188: // The created key is permanent and is not exportable in plaintext form. We should mention the new |slot| argument. I guess only the methods that create permanent keys need a |slot| argument. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:189: static RSAPrivateKey* CreateSensitive(ScopedPK11Slot slot, uint16 num_bits); Is it necessary to pass ScopedPK11Slot rather than a plain PK11Slot* pointer? What does it imply to pass ScopedPK11Slot? That the function is given a reference to the PK11Slot? https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:215: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) This can become #else. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:261: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) This can become #else. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:211: #endif BUG?: it seems wrong to remove this check. In this file, !defined(USE_NSS) is equivalent to defined(OS_WIN) || defined(OS_MACOSX), where we use the NSS library but not the NSS cert and key databases. On those two platforms, we should still check that |permanent| is false. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:246: #endif BUG?: it seems wrong to remove this check. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:56: return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()), IMPORTANT: this should be PK11_GetInternalSlot(). See lines 217-218 in the original code. Note: when NSS cert and key databases are used, PK11_GetInternalKeySlot() and PK11_GetInternalSlot() are different slots. PK11_GetInternalKeySlot() is the slot that is backed by the NSS cert and key databases, so it can create permanent keys if necessary. PK11_GetInternalSlot() is a slot that is usually NOT backed by the NSS cert and key databases, so it should only be used for temporary keys. When NSS cert and key databases are not used, PK11_GetInternalKeySlot() and PK11_GetInternalSlot() are the same slot. Please let me know if this is not clear. I know it is confusing. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:68: ScopedPK11Slot(PK11_GetInternalKeySlot()), IMPORTANT: this should be PK11_GetInternalSlot(). See lines 255-256 in the original code. https://codereview.chromium.org/66213002/diff/120001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2362: crypto::ScopedPK11Slot(PK11_GetInternalKeySlot()), IMPORTANT: this should be PK11_GetInternalSlot(). https://codereview.chromium.org/66213002/diff/120001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/66213002/diff/120001/remoting/remoting.gyp#ne... remoting/remoting.gyp:2787: '../crypto/crypto.gyp:crypto', Question: can we satisfy the dependency on '../crypto/crypto.gyp:crypto' via 'remoting_base' (line 2795)?
https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); On 2013/11/09 03:44:32, mattm wrote: > > However, a few test related things do use CreateSensitive: > https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... > https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... > > I guess a 3-sided patch would be needed to update those.. Good. You already found the files that need to be updated.
https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... File crypto/ec_private_key_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:338: bool success = ECPrivateKey::ImportFromEncryptedPrivateKeyInfo( Could you take the opportunity to remove "ECPrivateKey::" from this line? It is not necessary because we are inside a ECPrivateKey method.
Updated CL. One question: So I don't know what the procedure is for updating the libchrome_crypto package in chromeos... it looks like it's still way back on r180245. Do you think it's acceptable to leave it up to whoever rolls that package to update the few chromeos-side callers at that time? https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h File crypto/ec_private_key.h (right): https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:17: #endif On 2013/11/11 20:56:25, wtc wrote: > > Move this to the #else block on lines 22-27. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:50: // The created key is permanent and is not exportable in plaintext form. On 2013/11/11 20:56:25, wtc wrote: > > Nit: mention the new |slot| argument. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:52: // NOTE: Currently only available if USE_NSS is defined. On 2013/11/11 20:56:25, wtc wrote: > > Nit: perhaps we can delete this comment now that the function is inside #if > defined(USE_NSS). Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key.h#... crypto/ec_private_key.h:72: // NOTE: Currently only available if USE_NSS is defined. On 2013/11/11 20:56:25, wtc wrote: > > Nit: perhaps we can delete this comment now that the function is inside #if > defined(USE_NSS). Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... File crypto/ec_private_key_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:36: crypto::ScopedPK11Slot slot(PK11_GetInternalKeySlot()); On 2013/11/11 20:56:25, wtc wrote: > > It may be a good idea to keep the GetKeySlot() function, so that we know the > slot we test for PKCS #11 mechanism support here will be the slot we use in the > rest of this file. > > We can change GetKeySlot() to return PK11_GetInternalKeySlot() if > crypto::GetPublicNSSKeySlot() is not appropriate. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:89: return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()), On 2013/11/11 20:56:25, wtc wrote: > > IMPORTANT: I think this should be PK11_GetInternalSlot(). Done. > I explained this issue in rsa_private_key_nss.cc. But I am less sure in this > file because the original code in this file uses crypto::GetPublicNSSKeySlot(), > which is defined as: > > PK11SlotInfo* GetPublicNSSKeySlot() { > if (test_slot_) > return PK11_ReferenceSlot(test_slot_); > if (software_slot_) > return PK11_ReferenceSlot(software_slot_); > return PK11_GetInternalKeySlot(); > } > > So PK11_GetInternalKeySlot() may be more appropriate in this file. > > Also, why don't we need to the overrides by test_slot_ and software_slot_? I believe that for temporary keys, it shouldn't matter which slot is used. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:91: PR_FALSE /* not sensitive */); On 2013/11/11 20:56:25, wtc wrote: > > Nit: these should be 'false' instead of 'PR_FALSE' because they are of the C++ > bool type. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:98: slot.Pass(), PR_TRUE /* permanent */, PR_TRUE /* sensitive */); On 2013/11/11 20:56:25, wtc wrote: > > Nit: these should be 'true' instead of 'PR_TRUE'. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:110: ScopedPK11Slot(PK11_GetInternalKeySlot()), On 2013/11/11 20:56:25, wtc wrote: > > IMPORTANT: I think this should be PK11_GetInternalSlot(). Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:115: PR_FALSE /* not sensitive */); On 2013/11/11 20:56:25, wtc wrote: > > Nit: these should be 'false' instead of 'PR_FALSE'. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:131: PR_TRUE /* sensitive */); On 2013/11/11 20:56:25, wtc wrote: > > Nit: these should be 'true' instead of 'PR_TRUE'. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/ec_private_key_ns... crypto/ec_private_key_nss.cc:338: bool success = ECPrivateKey::ImportFromEncryptedPrivateKeyInfo( On 2013/11/11 22:18:54, wtc wrote: > > Could you take the opportunity to remove "ECPrivateKey::" from this line? It is > not necessary because we are inside a ECPrivateKey method. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h File crypto/rsa_private_key.h (right): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:27: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) On 2013/11/11 20:56:25, wtc wrote: > > Optional: This can become #else. Then you can move the > #include "crypto/scoped_nss_types.h" > into this block. > > Note: there are two other instances of this in this file. I pointed them out > below. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:188: // The created key is permanent and is not exportable in plaintext form. On 2013/11/11 20:56:25, wtc wrote: > > We should mention the new |slot| argument. I guess only the methods that create > permanent keys need a |slot| argument. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:189: static RSAPrivateKey* CreateSensitive(ScopedPK11Slot slot, uint16 num_bits); On 2013/11/11 20:56:25, wtc wrote: > > Is it necessary to pass ScopedPK11Slot rather than a plain PK11Slot* pointer? > > What does it imply to pass ScopedPK11Slot? That the function is given a > reference to the PK11Slot? Yeah, however it isn't really necessary for this case. The "pass a bare pointer and the function can take a ref if it wants one" convention is pretty clear I think. Changed it so the callers just use a ScopedPK11Slot locally to hold the reference while they call this. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:215: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) On 2013/11/11 20:56:25, wtc wrote: > > This can become #else. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key.h... crypto/rsa_private_key.h:261: #elif defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX) On 2013/11/11 20:56:25, wtc wrote: > > This can become #else. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:211: #endif On 2013/11/11 20:56:25, wtc wrote: > > BUG?: it seems wrong to remove this check. > > In this file, !defined(USE_NSS) is equivalent to defined(OS_WIN) || > defined(OS_MACOSX), where we use the NSS library but not the NSS cert and key > databases. On those two platforms, we should still check that |permanent| is > false. Hm, yeah. I guess I was thinking that since CreateSensitive functions are only being defined when USE_NSS is defined that it shouldn't be possible to trigger the check. But it does seem good to leave the check since there could still be a bug internal to the class, or to catch problems in future modifications. Re-added. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); On 2013/11/09 03:44:32, mattm wrote: > On 2013/11/09 02:26:47, Ryan Sleevi wrote: > > Pretty sure ChromeOS is relying on this for TPM provisioning > > Hm, I found "platform/login_manager" uses RSAPrivateKey, though not the > CreateSensitive functions: > https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... > > However, a few test related things do use CreateSensitive: > https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... > https://chromium.googlesource.com/chromiumos/platform/login_manager/+/master/... > > I guess a 3-sided patch would be needed to update those.. So I don't know what the procedure is for rolling the libchrome_crypto package in chromeos... it looks like it's still way back on r180245. Do you think it's acceptable to leave it up to whoever rolls that package to update the few chromeos-side callers at that time? https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:246: #endif On 2013/11/11 20:56:25, wtc wrote: > > BUG?: it seems wrong to remove this check. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:56: return CreateWithParams(ScopedPK11Slot(PK11_GetInternalKeySlot()), On 2013/11/11 20:56:25, wtc wrote: > > IMPORTANT: this should be PK11_GetInternalSlot(). > > See lines 217-218 in the original code. > > Note: when NSS cert and key databases are used, PK11_GetInternalKeySlot() and > PK11_GetInternalSlot() are different slots. PK11_GetInternalKeySlot() is the > slot that is backed by the NSS cert and key databases, so it can create > permanent keys if necessary. > > PK11_GetInternalSlot() is a slot that is usually NOT backed by the NSS cert and > key databases, so it should only be used for temporary keys. > > When NSS cert and key databases are not used, PK11_GetInternalKeySlot() and > PK11_GetInternalSlot() are the same slot. > > Please let me know if this is not clear. I know it is confusing. Thanks, fixed. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:68: ScopedPK11Slot(PK11_GetInternalKeySlot()), On 2013/11/11 20:56:25, wtc wrote: > > IMPORTANT: this should be PK11_GetInternalSlot(). > > See lines 255-256 in the original code. Done. https://codereview.chromium.org/66213002/diff/120001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/66213002/diff/120001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2362: crypto::ScopedPK11Slot(PK11_GetInternalKeySlot()), On 2013/11/11 20:56:25, wtc wrote: > > IMPORTANT: this should be PK11_GetInternalSlot(). Done. https://codereview.chromium.org/66213002/diff/120001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/66213002/diff/120001/remoting/remoting.gyp#ne... remoting/remoting.gyp:2787: '../crypto/crypto.gyp:crypto', On 2013/11/11 20:56:25, wtc wrote: > > Question: can we satisfy the dependency on '../crypto/crypto.gyp:crypto' via > 'remoting_base' (line 2795)? It seemed like crypto dep has to be added directly for the include path to get set. However, I switched to not using ScopedPK11Slot in the public interface, so these changes aren't necessary anymore (there were more that I had missed too).
Patch set 6 LGTM. Thanks. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:211: #endif On 2013/11/12 02:42:44, mattm wrote: > I guess I was thinking that since CreateSensitive functions are only > being defined when USE_NSS is defined that it shouldn't be possible to trigger > the check. I see. Yes, you are right, we can conclude the check won't fail by code inspection. > But it does seem good to leave the check since there could still be a > bug internal to the class, or to catch problems in future modifications. > Re-added. The NSS function (PK11_GenerateKeyPair, etc.) to create a permanent key will fail if there is no NSS database, so this check isn't really necessary. It is merely an advance warning for something that is expected to fail, which should make debugging easier. Now that I understand the background, I am fine with omitting the check. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); On 2013/11/12 02:42:44, mattm wrote: > > So I don't know what the procedure is for rolling the libchrome_crypto package > in chromeos... it looks like it's still way back on r180245. I don't know what the procedure is, either. > Do you think it's > acceptable to leave it up to whoever rolls that package to update the few > chromeos-side callers at that time? I think we should at least send a patch to the owner of the libchrome_crypto package. Can you write the patch?
https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... File crypto/rsa_private_key_nss.cc (left): https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:211: #endif On 2013/11/12 18:49:42, wtc wrote: > > On 2013/11/12 02:42:44, mattm wrote: > > I guess I was thinking that since CreateSensitive functions are only > > being defined when USE_NSS is defined that it shouldn't be possible to trigger > > the check. > > I see. Yes, you are right, we can conclude the check won't fail by code > inspection. > > > But it does seem good to leave the check since there could still be a > > bug internal to the class, or to catch problems in future modifications. > > Re-added. > > The NSS function (PK11_GenerateKeyPair, etc.) to create a permanent key will > fail if there is no NSS database, so this check isn't really necessary. It is > merely an advance warning for something that is expected to fail, which should > make debugging easier. > > Now that I understand the background, I am fine with omitting the check. Done. https://codereview.chromium.org/66213002/diff/120001/crypto/rsa_private_key_n... crypto/rsa_private_key_nss.cc:218: PK11_GetInternalSlot()); On 2013/11/12 18:49:42, wtc wrote: > > On 2013/11/12 02:42:44, mattm wrote: > > > > So I don't know what the procedure is for rolling the libchrome_crypto package > > in chromeos... it looks like it's still way back on r180245. > > I don't know what the procedure is, either. > > > Do you think it's > > acceptable to leave it up to whoever rolls that package to update the few > > chromeos-side callers at that time? > > I think we should at least send a patch to the owner of the libchrome_crypto > package. Can you write the patch? Checked with cmasone and sent him an example patch. He said to go ahead and he'll deal with it when he rolls the libchrome_crypto.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/66213002/540001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/66213002/540001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/66213002/540001
Message was sent while issue was closed.
Change committed as 234726 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
