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

chrome/browser/chromeos/platform_keys/platform_keys_nss.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/platform_keys/platform_keys.h"
 
 #include <cert.h>
 #include <cryptohi.h>
 #include <keyhi.h>
 #include <secder.h>
@@ skipping 13 matching lines @@
 #include "chrome/browser/browser_process_platform_part_chromeos.h"
 #include "chrome/browser/chromeos/net/client_cert_filter_chromeos.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h"
 #include "chrome/browser/net/nss_context.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
-#include "crypto/rsa_private_key.h"
+#include "crypto/nss_key_util.h"
+#include "crypto/scoped_nss_types.h"
 #include "net/base/crypto_module.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_database.h"
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util_nss.h"
 #include "net/ssl/client_cert_store_chromeos.h"
 #include "net/ssl/ssl_cert_request_info.h"
 
 using content::BrowserContext;
@@ skipping 348 matching lines @@
     : certificate_(certificate), callback_(callback) {
 }
 
 GetTokensState::GetTokensState(const GetTokensCallback& callback)
     : callback_(callback) {
 }
 
 // Does the actual key generation on a worker thread. Used by
 // GenerateRSAKeyWithDB().
 void GenerateRSAKeyOnWorkerThread(scoped_ptr<GenerateRSAKeyState> state) {
-  scoped_ptr<crypto::RSAPrivateKey> rsa_key(
-      crypto::RSAPrivateKey::CreateSensitive(state->slot_.get(),
-                                             state->modulus_length_bits_));
-  if (!rsa_key) {
+  if (!state->slot_) {

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

This shouldn't be possible, but if token_id is empty, DidGetCertDBOnIOThread
intentionally leaves slot_ null. The only caller I found checks the token_id
parameter early, but that's not locally clear.

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

That's right.
I'll create a separate CL that adds an additional sanity check of token_id in
GenerateRSAKey.

+    LOG(ERROR) << "No slot.";
+    state->OnError(FROM_HERE, kErrorInternal);
+    return;
+  }
+
+  crypto::ScopedSECKEYPublicKey public_key;
+  crypto::ScopedSECKEYPrivateKey private_key;
+  if (!crypto::GenerateRSAKeyPairNSS(
+          state->slot_.get(), state->modulus_length_bits_, true /* permanent */,
+          &public_key, &private_key)) {
     LOG(ERROR) << "Couldn't create key.";
     state->OnError(FROM_HERE, kErrorInternal);
     return;
   }
 
-  std::vector<uint8> public_key_spki_der;
-  if (!rsa_key->ExportPublicKey(&public_key_spki_der)) {
-    // TODO(pneubeck): Remove rsa_key from storage.
+  crypto::ScopedSECItem public_key_der(
+      SECKEY_EncodeDERSubjectPublicKeyInfo(public_key.get()));
+  if (!public_key_der) {
+    // TODO(pneubeck): Remove private_key and public_key from storage.
     LOG(ERROR) << "Couldn't export public key.";
     state->OnError(FROM_HERE, kErrorInternal);
     return;
   }
   state->CallBack(
       FROM_HERE,
-      std::string(public_key_spki_der.begin(), public_key_spki_der.end()),
+      std::string(reinterpret_cast<const char*>(public_key_der->data),
+                  public_key_der->len),
       std::string() /* no error */);
 }
 
 // Continues generating a RSA key with the obtained NSSCertDatabase. Used by
 // GenerateRSAKey().
 void GenerateRSAKeyWithDB(scoped_ptr<GenerateRSAKeyState> state,
                           net::NSSCertDatabase* cert_db) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Only the slot and not the NSSCertDatabase is required. Ignore |cert_db|.
   base::WorkerPool::PostTask(
       FROM_HERE,
       base::Bind(&GenerateRSAKeyOnWorkerThread, base::Passed(&state)),
       true /*task is slow*/);
 }
 
 // Does the actual signing on a worker thread. Used by SignRSAWithDB().
 void SignRSAOnWorkerThread(scoped_ptr<SignRSAState> state) {
   const uint8* public_key_uint8 =
       reinterpret_cast<const uint8*>(state->public_key_.data());
   std::vector<uint8> public_key_vector(
       public_key_uint8, public_key_uint8 + state->public_key_.size());
 
   // TODO(pneubeck): This searches all slots. Change to look only at |slot_|.
-  scoped_ptr<crypto::RSAPrivateKey> rsa_key(
-      crypto::RSAPrivateKey::FindFromPublicKeyInfo(public_key_vector));
+  crypto::ScopedSECKEYPrivateKey rsa_key(
+      crypto::FindNSSKeyFromPublicKeyInfo(public_key_vector));
 
   // Fail if the key was not found. If a specific slot was requested, also fail
   // if the key was found in the wrong slot.
-  if (!rsa_key ||
-      (state->slot_ && rsa_key->key()->pkcs11Slot != state->slot_)) {
+  if (!rsa_key || SECKEY_GetPrivateKeyType(rsa_key.get()) != rsaKey ||
+      (state->slot_ && rsa_key->pkcs11Slot != state->slot_)) {
     state->OnError(FROM_HERE, kErrorKeyNotFound);
     return;
   }
 
   std::string signature_str;
   if (state->sign_direct_pkcs_padded_) {
     static_assert(
         sizeof(*state->data_.data()) == sizeof(char),
         "Can't reinterpret data if it's characters are not 8 bit large.");
     SECItem input = {siBuffer,
                      reinterpret_cast<unsigned char*>(
                          const_cast<char*>(state->data_.data())),
                      state->data_.size()};
 
     // Compute signature of hash.
-    int signature_len = PK11_SignatureLen(rsa_key->key());
+    int signature_len = PK11_SignatureLen(rsa_key.get());
     if (signature_len <= 0) {
       state->OnError(FROM_HERE, kErrorInternal);
       return;
     }
 
     std::vector<unsigned char> signature(signature_len);
     SECItem signature_output = {
         siBuffer, vector_as_array(&signature), signature.size()};
-    if (PK11_Sign(rsa_key->key(), &signature_output, &input) == SECSuccess)
+    if (PK11_Sign(rsa_key.get(), &signature_output, &input) == SECSuccess)
       signature_str.assign(signature.begin(), signature.end());
   } else {
     SECOidTag sign_alg_tag = SEC_OID_UNKNOWN;
     switch (state->hash_algorithm_) {
       case HASH_ALGORITHM_SHA1:
         sign_alg_tag = SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION;
         break;
       case HASH_ALGORITHM_SHA256:
         sign_alg_tag = SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION;
         break;
       case HASH_ALGORITHM_SHA384:
         sign_alg_tag = SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION;
         break;
       case HASH_ALGORITHM_SHA512:
         sign_alg_tag = SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION;
         break;
       case HASH_ALGORITHM_NONE:
         NOTREACHED();
         break;
     }
 
     SECItem sign_result = {siBuffer, nullptr, 0};
     if (SEC_SignData(
             &sign_result,
             reinterpret_cast<const unsigned char*>(state->data_.data()),
-            state->data_.size(), rsa_key->key(), sign_alg_tag) == SECSuccess) {
+            state->data_.size(), rsa_key.get(), sign_alg_tag) == SECSuccess) {
       signature_str.assign(sign_result.data,
                            sign_result.data + sign_result.len);
     }
   }
 
   if (signature_str.empty()) {
     LOG(ERROR) << "Couldn't sign.";
     state->OnError(FROM_HERE, kErrorInternal);
     return;
   }
@@ skipping 363 matching lines @@
   NSSOperationState* state_ptr = state.get();
   GetCertDatabase(std::string() /* don't get any specific slot */,
                   base::Bind(&GetTokensWithDB, base::Passed(&state)),
                   browser_context,
                   state_ptr);
 }
 
 }  // namespace platform_keys
 
 }  // namespace chromeos