Don't use RSAPrivateKey in NSS integration code.

crypto/nss_key_util.cc

Index: crypto/nss_key_util.cc
diff --git a/crypto/nss_key_util.cc b/crypto/nss_key_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..08912c55615ff29b146a4080b2a5153079dd9ddc
--- /dev/null
+++ b/crypto/nss_key_util.cc
@@ -0,0 +1,169 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "crypto/nss_key_util.h"
+
+#include <cryptohi.h>
+#include <keyhi.h>
+#include <pk11pub.h>
+
+#include "base/logging.h"
+#include "crypto/nss_util.h"
+
+#if defined(USE_NSS_CERTS)
+#include <secmod.h>
+#include "crypto/nss_util_internal.h"
+#endif
+
+namespace crypto {
+
+namespace {
+
+#if defined(USE_NSS_CERTS)
+
+struct PublicKeyInfoDeleter {
+  inline void operator()(CERTSubjectPublicKeyInfo* spki) {
+    SECKEY_DestroySubjectPublicKeyInfo(spki);
+  }
+};
+
+typedef scoped_ptr<CERTSubjectPublicKeyInfo, PublicKeyInfoDeleter>
+    ScopedPublicKeyInfo;
+
+// Decodes |input| as a SubjectPublicKeyInfo and returns a SECItem containing
+// the CKA_ID of that public key or nullptr on error.
+ScopedSECItem MakeIDFromSPKI(const std::vector<uint8_t> input) {
+  // First, decode and save the public key.
+  SECItem key_der;
+  key_der.type = siBuffer;
+  key_der.data = const_cast<unsigned char*>(&input[0]);

[pneubeck (no reviews), 2015/04/28 09:56:26]

note that this fails if size is null.
probably (depending on what the policy for crypto/ is) replace by
vector_as_array from base/stl_util.h .

or handle empty input explicitly.

[davidben, 2015/04/28 16:27:46]

Done.

+  key_der.len = input.size();
+
+  ScopedPublicKeyInfo spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&key_der));
+  if (!spki)
+    return nullptr;
+
+  ScopedSECKEYPublicKey result(SECKEY_ExtractPublicKey(spki.get()));
+  if (!result)
+    return nullptr;
+
+  // See pk11_MakeIDFromPublicKey from NSS.
+  SECItem* pub_key_index;
+  switch (SECKEY_GetPublicKeyType(result.get())) {
+    case rsaKey:
+      pub_key_index = &result->u.rsa.modulus;
+      break;
+    default:
+      // Unsupported key type.
+      return nullptr;
+  }
+
+  return ScopedSECItem(PK11_MakeIDFromPubKey(pub_key_index));

[pneubeck (no reviews), 2015/04/28 09:56:26]

you could move this to line 55 and remove the temporary pub_key_index .

(if not, initialize pub_key_index to nullptr pls)

[davidben, 2015/04/28 16:27:45]

Done.

+}
+
+#endif  // defined(USE_NSS_CERTS)
+
+}  // namespace
+
+bool GenerateRSAKeyPairNSS(PK11SlotInfo* slot,

[pneubeck (no reviews), 2015/04/28 09:56:26]

it's unclear (or at least subtle) how PK11_GenerateKeyPair handles a
slot==nulltptr.
you could add a CHECK for that.

same for all other functions that take a slot argument.

[davidben, 2015/04/28 16:27:46]

NSS generally checks for NULL inputs (and does here). I've added DCHECKs, but
the intent was that these functions can only called on non-NULL slots. It's sad
that the typesystem doesn't check this, but I don't think our code usually
allows for any random T* parameter to be NULL.

[pneubeck (no reviews), 2015/04/28 16:40:27]

That's right. Usually we drop a DCHECK if we are confident that it crashes in a
reproducible / debugging-friendly way.
I was just not sure whether that is the case here.

+                           uint16_t num_bits,
+                           bool permanent,
+                           ScopedSECKEYPublicKey* out_public_key,
+                           ScopedSECKEYPrivateKey* out_private_key) {
+  PK11RSAGenParams param;
+  param.keySizeInBits = num_bits;
+  param.pe = 65537L;
+  SECKEYPublicKey* public_key_raw;

[pneubeck (no reviews), 2015/04/28 09:56:27]

should be intialized to nulltpr.

[davidben, 2015/04/28 16:27:46]

Done.

+  ScopedSECKEYPrivateKey private_key(PK11_GenerateKeyPair(

[pneubeck (no reviews), 2015/04/28 09:56:26]

could directly assign to out_private_key
and rename out_private_key to private_key.
and similarly, out_public_key to public_key

[davidben, 2015/04/28 16:27:45]

I prefer explicitly annotating output parameters in functions (the out_ prefix
is required in BoringSSL style), but done since it's not Chromium style.

+      slot, CKM_RSA_PKCS_KEY_PAIR_GEN, &param, &public_key_raw, permanent,
+      permanent /* sensitive */, nullptr));
+  if (!private_key)
+    return false;
+
+  out_public_key->reset(public_key_raw);
+  *out_private_key = private_key.Pass();
+  return true;
+}
+
+ScopedSECKEYPrivateKey ImportNSSKeyFromPrivateKeyInfo(

[pneubeck (no reviews), 2015/04/28 09:56:26]

i think this function did an EnsureNSSInit before and doesn't do anymore.

I do not know whether it's actually required.

[davidben, 2015/04/28 16:27:46]

I didn't put in EnsureNSSInit for functions that take a PK11SlotInfo because, if
you already have a slot, you must have initialized NSS already.

[pneubeck (no reviews), 2015/04/28 16:40:27]

Acknowledged.

+    PK11SlotInfo* slot,

[pneubeck (no reviews), 2015/04/28 09:56:26]

ditto, add check for slot

[davidben, 2015/04/28 16:27:46]

Added DCHECK.

+    const std::vector<uint8_t>& input,
+    bool permanent) {
+  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  if (!arena) {
+    NOTREACHED();

[pneubeck (no reviews), 2015/04/28 09:56:26]

these NOTREACHED (which equals a DCHECK) + return are not following the chrome
policy:

A DCHECK() means "this condition must always be true", ...
A consequence of this is that you should not handle DCHECK() failures, even if
failure would result in a crash.

If these are indeed invariants, then change to DCHECK(arena).
If these are not invariants, these should be handled with the return and
probably a LOG(ERROR) or a CHECK(arena).

[davidben, 2015/04/28 16:27:45]

This came from the old code. Invariant is really not a well-defined word here.
This is a function that normally can fail due to malloc failure, and NSS tries
to be malloc-failure-safe. However our malloc cannot fail. But, being a library
that handles malloc failures (and an external one at that!), it's strictly
speaking not promised anywhere that only malloc failures are the only ways it
will ever fail. I think NOTREACHED() + return is a fine way to handle such a
thing.

Regardless, switched to a DCHECK. We vary elsewhere between never checking and
DCHECK, so DCHECK it is.

+    return nullptr;
+  }
+
+  // Excess data is illegal, but NSS silently accepts it, so first ensure that
+  // |input| consists of a single ASN.1 element.
+  SECItem input_item;
+  input_item.data = const_cast<unsigned char*>(&input.front());

[pneubeck (no reviews), 2015/04/28 09:56:26]

ditto, fails if input is empty.

[davidben, 2015/04/28 16:27:46]

Done.

+  input_item.len = input.size();
+  SECItem der_private_key_info;
+  SECStatus rv =
+      SEC_QuickDERDecodeItem(arena.get(), &der_private_key_info,
+                             SEC_ASN1_GET(SEC_AnyTemplate), &input_item);
+  if (rv != SECSuccess)
+    return nullptr;
+
+  // Allow the private key to be used for key unwrapping, data decryption,
+  // and signature generation.
+  const unsigned int key_usage =
+      KU_KEY_ENCIPHERMENT | KU_DATA_ENCIPHERMENT | KU_DIGITAL_SIGNATURE;
+  SECKEYPrivateKey* key_raw;

[pneubeck (no reviews), 2015/04/28 09:56:26]

should be initialized to nullptr.

[davidben, 2015/04/28 16:27:45]

Done.

+  rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
+      slot, &der_private_key_info, nullptr, nullptr, permanent,
+      permanent /* sensitive */, key_usage, &key_raw, nullptr);
+  if (rv != SECSuccess)
+    return nullptr;
+  return ScopedSECKEYPrivateKey(key_raw);
+}
+
+#if defined(USE_NSS_CERTS)
+
+ScopedSECKEYPrivateKey FindNSSKeyFromPublicKeyInfo(
+    const std::vector<uint8_t>& input) {
+  EnsureNSSInit();
+
+  ScopedSECItem cka_id(MakeIDFromSPKI(input));
+  if (!cka_id) {
+    NOTREACHED();

[pneubeck (no reviews), 2015/04/28 09:56:26]

ditto, wrong NOTREACHED

[davidben, 2015/04/28 16:27:45]

Removed the NOTREACHED. I probably got it from the old code. It's wrong.

+    return nullptr;
+  }
+
+  // Search all slots in all modules for the key with the given ID.
+  AutoSECMODListReadLock auto_lock;
+  SECMODModuleList* head = SECMOD_GetDefaultModuleList();

[pneubeck (no reviews), 2015/04/28 09:56:26]

+ const

[davidben, 2015/04/28 16:27:45]

Done.

+  for (SECMODModuleList* item = head; item != nullptr; item = item->next) {
+    int slot_count = item->module->loaded ? item->module->slotCount : 0;

[pneubeck (no reviews), 2015/04/28 09:56:26]

+ const

[davidben, 2015/04/28 16:27:46]

I'm assume this meant to be the previous line? Done.

[pneubeck (no reviews), 2015/04/28 16:40:27]

then both lines ;-)

[davidben, 2015/04/29 21:51:32]

Do you mean the local variable? I didn't think we usually consted those. It's
not a pointer.

[pneubeck (no reviews), 2015/04/30 07:45:55]

Yes.
as you prefer. I don't think there is a clear rule for that. I tend to const
such as well.

+    for (int i = 0; i < slot_count; i++) {
+      // Look for the key in slot |i|.
+      ScopedSECKEYPrivateKey key(
+          PK11_FindKeyByKeyID(item->module->slots[i], cka_id.get(), nullptr));
+      if (key)
+        return key.Pass();
+    }
+  }
+
+  // The key wasn't found in any module.
+  return nullptr;
+}
+
+ScopedSECKEYPrivateKey FindNSSKeyFromPublicKeyInfoInSlot(

[pneubeck (no reviews), 2015/04/28 09:56:26]

i think this function did an EnsureNSSInit before and doesn't do anymore.

I do not know whether it's actually required.

[davidben, 2015/04/28 16:27:46]

Ditto about slot parameter.

+    const std::vector<uint8_t>& input,
+    PK11SlotInfo* slot) {

[pneubeck (no reviews), 2015/04/28 09:56:26]

ditto, add check for slot

[davidben, 2015/04/28 16:27:45]

Done.

+  ScopedSECItem cka_id(MakeIDFromSPKI(input));
+  if (!cka_id) {
+    NOTREACHED();

[pneubeck (no reviews), 2015/04/28 09:56:26]

ditto, wrong NOTREACHED

[davidben, 2015/04/28 16:27:45]

Done.

+    return nullptr;
+  }
+
+  return ScopedSECKEYPrivateKey(
+      PK11_FindKeyByKeyID(slot, cka_id.get(), nullptr));
+}
+
+#endif  // defined(USE_NSS_CERTS)
+
+}  // namespace crypto