Reland "Don't use RSAPrivateKey in NSS integration code."

chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc

 // Copyright 2014 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 "chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h"
 
 #include <cryptohi.h>
+#include <keyhi.h>
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
+#include "crypto/nss_key_util.h"
 #include "crypto/nss_util_internal.h"
-#include "crypto/rsa_private_key.h"
 #include "crypto/scoped_nss_types.h"
 
 namespace {
 
 // The modulus length for RSA keys used by easy sign-in.
 const int kKeyModulusLength = 2048;
 
 // Relays |GetSystemSlotOnIOThread| callback to |response_task_runner|.
 void RunCallbackOnThreadRunner(
     const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
@@ skipping 13 matching lines @@
 
   crypto::ScopedPK11Slot system_slot =
       crypto::GetSystemNSSKeySlot(callback_on_origin_thread);
   if (system_slot)
     callback_on_origin_thread.Run(system_slot.Pass());
 }
 
 // Checks if a private RSA key associated with |public_key| can be found in
 // |slot|.
 // Must be called on a worker thread.
-scoped_ptr<crypto::RSAPrivateKey> GetPrivateKeyOnWorkerThread(
+crypto::ScopedSECKEYPrivateKey GetPrivateKeyOnWorkerThread(
     PK11SlotInfo* slot,
     const std::string& public_key) {
   const uint8* public_key_uint8 =
       reinterpret_cast<const uint8*>(public_key.data());
   std::vector<uint8> public_key_vector(
       public_key_uint8, public_key_uint8 + public_key.size());
 
-  scoped_ptr<crypto::RSAPrivateKey> rsa_key(
-      crypto::RSAPrivateKey::FindFromPublicKeyInfo(public_key_vector));
-  if (!rsa_key || rsa_key->key()->pkcs11Slot != slot)
-    return scoped_ptr<crypto::RSAPrivateKey>();
+  crypto::ScopedSECKEYPrivateKey rsa_key(
+      crypto::FindNSSKeyFromPublicKeyInfo(public_key_vector));
+  if (!rsa_key || rsa_key->pkcs11Slot != slot ||
+      SECKEY_GetPrivateKeyType(rsa_key.get()) != rsaKey) {
+    return nullptr;
+  }

[davidben, 2015/05/06 18:48:38]

I switched this to the slotless variant in case any existing code passed in a
null slot and to avoid behavior differences.

[pneubeck (no reviews), 2015/05/07 12:19:16]

I don't understand why all slots should be searched for the key?

Reading the above documentation, this function (GetPrivateKey...) should search
in a specific slot.
If that slot is null, it's an error (handled in  SignDataOnWorkerThread) and
should not silently return a slotless key.

I don't know if NSS can have key pairs without assigned slot.
As that is not obvious, please don't rely on it.

Instead, please do an early return instead at the beginning of the function.

(If something like slotless keys exist at all and we find the owner key to be
slotless, that should mean that we have a severe bug in the owner
implementation. The idea was that the owner key is stored in the user's key DB
on the user's encrypted partition.)

[davidben, 2015/05/07 20:51:44]

Yeah, looking through the callers, there is a CHECK so we're fine.
I intentionally want to keep this logic as similar to the original as possible
for this CL (also why there are rsaKey checks everywhere). Note that the
original does FindFromPublicKeyInfo and then checks pkcs11Slot. I'm happy to do
a follow-up to switch that to the slotted version. But it should be separate so
it can be reverted separately.

   return rsa_key.Pass();
 }
 
 // Signs |data| using a private key associated with |public_key| and stored in
 // |slot|. Once the data is signed, callback is run on |response_task_runner|.
 // In case of an error, the callback will be passed an empty string.
 void SignDataOnWorkerThread(
     crypto::ScopedPK11Slot slot,
     const std::string& public_key,
     const std::string& data,
     const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
     const base::Callback<void(const std::string&)>& callback) {
-  scoped_ptr<crypto::RSAPrivateKey> private_key(
+  crypto::ScopedSECKEYPrivateKey private_key(
       GetPrivateKeyOnWorkerThread(slot.get(), public_key));
   if (!private_key) {
     LOG(ERROR) << "Private key for signing data not found";
     response_task_runner->PostTask(FROM_HERE,
                                    base::Bind(callback, std::string()));
     return;
   }
 
   crypto::ScopedSECItem sign_result(SECITEM_AllocItem(NULL, NULL, 0));
   if (SEC_SignData(sign_result.get(),
                    reinterpret_cast<const unsigned char*>(data.data()),
-                   data.size(),
-                   private_key->key(),
+                   data.size(), private_key.get(),
                    SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION) != SECSuccess) {
     LOG(ERROR) << "Failed to sign data";
     response_task_runner->PostTask(FROM_HERE,
                                    base::Bind(callback, std::string()));
     return;
   }
 
   std::string signature(reinterpret_cast<const char*>(sign_result->data),
                         sign_result->len);
   response_task_runner->PostTask(FROM_HERE, base::Bind(callback, signature));
 }
 
 // Creates a RSA key pair in |slot|. When done, it runs |callback| with the
 // created public key on |response_task_runner|.
 // If |public_key| is not empty, a key pair will be created only if the private
 // key associated with |public_key| does not exist in |slot|. Otherwise the
 // callback will be run with |public_key|.
 void CreateTpmKeyPairOnWorkerThread(
     crypto::ScopedPK11Slot slot,
     const std::string& public_key,
     const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
     const base::Callback<void(const std::string&)>& callback) {
   if (!public_key.empty() &&
       GetPrivateKeyOnWorkerThread(slot.get(), public_key)) {
     response_task_runner->PostTask(FROM_HERE, base::Bind(callback, public_key));
     return;
   }
 
-  scoped_ptr<crypto::RSAPrivateKey> rsa_key(
-      crypto::RSAPrivateKey::CreateSensitive(slot.get(), kKeyModulusLength));
-  if (!rsa_key) {
+  crypto::ScopedSECKEYPublicKey public_key_obj;
+  crypto::ScopedSECKEYPrivateKey private_key_obj;
+  if (!crypto::GenerateRSAKeyPairNSS(slot.get(), kKeyModulusLength,
+                                     true /* permanent */, &public_key_obj,
+                                     &private_key_obj)) {
     LOG(ERROR) << "Failed to create an RSA key.";
     response_task_runner->PostTask(FROM_HERE,
                                    base::Bind(callback, std::string()));
     return;
   }
 
-  std::vector<uint8> created_public_key;
-  if (!rsa_key->ExportPublicKey(&created_public_key)) {
+  crypto::ScopedSECItem public_key_der(
+      SECKEY_EncodeDERSubjectPublicKeyInfo(public_key_obj.get()));
+  if (!public_key_der) {
     LOG(ERROR) << "Failed to export public key.";
     response_task_runner->PostTask(FROM_HERE,
                                    base::Bind(callback, std::string()));
     return;
   }
 
   response_task_runner->PostTask(
-      FROM_HERE,
-      base::Bind(callback,
-                 std::string(created_public_key.begin(),
-                             created_public_key.end())));
+      FROM_HERE, base::Bind(callback, std::string(reinterpret_cast<const char*>(
+                                                      public_key_der->data),
+                                                  public_key_der->len)));
 }
 
 }  // namespace
 
 // static
 void EasyUnlockTpmKeyManager::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kEasyUnlockLocalStateTpmKeys);
 }
 
@@ skipping 188 matching lines @@
   // If key creation failed, reset the state machine.
   create_tpm_key_state_ =
       public_key.empty() ? CREATE_TPM_KEY_NOT_STARTED : CREATE_TPM_KEY_DONE;
 }
 
 void EasyUnlockTpmKeyManager::OnDataSigned(
     const base::Callback<void(const std::string&)>& callback,
     const std::string& signature) {
   callback.Run(signature);
 }