[webcrypto] Fix a bug with JWK private key import.

content/child/webcrypto/platform_crypto_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 "content/child/webcrypto/platform_crypto.h"
 
 #include <cryptohi.h>
 #include <pk11pub.h>
 #include <secerr.h>
 #include <sechash.h>
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/child/webcrypto/crypto_data.h"
 #include "content/child/webcrypto/status.h"
 #include "content/child/webcrypto/webcrypto_util.h"
 #include "crypto/nss_util.h"
+#include "crypto/random.h"
 #include "crypto/scoped_nss_types.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 #include "third_party/WebKit/public/platform/WebCryptoKeyAlgorithm.h"
 
 #if defined(USE_NSS)
 #include <dlfcn.h>
 #include <secoid.h>
 #endif
 
@@ skipping 1679 matching lines @@
   AddAttribute(CKA_KEY_TYPE, &key_type, sizeof(key_type), &key_template);
   AddAttribute(CKA_TOKEN, &ck_false, sizeof(ck_false), &key_template);
   AddAttribute(CKA_SENSITIVE, &ck_false, sizeof(ck_false), &key_template);
   AddAttribute(CKA_PRIVATE, &ck_false, sizeof(ck_false), &key_template);
 
   // Required properties.
   AddOptionalAttribute(CKA_MODULUS, modulus, &key_template);
   AddOptionalAttribute(CKA_PUBLIC_EXPONENT, public_exponent, &key_template);
   AddOptionalAttribute(CKA_PRIVATE_EXPONENT, private_exponent, &key_template);
 
+  // Manufacture a CKA_ID so the created key can be retrieved later as a
+  // SECKEYPrivateKey using FindKeyByKeyID(). Unfortunately there isn't a more
+  // direct way to do this in NSS.
+  //
+  // A randomly generated value is used for the ID.
+  //
+  // NSS assigns the CKA_ID to PK11_MakeIDFromPubKey() for the other key
+  // creation paths. However it is incorrect to try and use that as the CKA_ID
+  // here because:
+  //
+  //  (1) CreateGenericObject does not validate its parameters. It is therefore
+  //      possible to construct a key using the known public modulus, and all
+  //      the other parameters are bogus. FindKeyByKeyID() returns the first
+  //      key matching the ID. So this would effectively allow an attacker to
+  //      retrieve a private key of there choice.

[Ryan Sleevi, 2014/06/02 22:57:05]

1) Please update this comment, as of NSS 3.16.2 it does.
2) there -> their
3) It's entirely possible to check this, so your problem as stated does not
argue the necessity of your approach.

You could just as well do checking yourself by retrieving (PRIME_1, PRIME_2,
EXPONENT_1, EXPONENT_2, CKA_COEFF) and seeing that they all equal the supplied
(input) values if they were present. Likewise, even if they're absent, you can
see if the private exponent and public exponent match what was supplied.

Note that we can detect this programatically on *all* versions of NSS.

[eroman, 2014/06/02 23:40:18]

Done.
Checking the parameters (without validating their meaning) has a different set
of problems:

 - Person A could import a bogus key via JWK, with modulus X. We verified that
the parameters of the created key are what was passed in.

 - Person B tries to import a valid key with modulus X, and gets back the one
created by person A. They are prevented from being able to import their key.

[Ryan Sleevi, 2014/06/02 23:55:57]

This won't succeed on Windows/Mac/ChromeOS. And we can ensure it won't succeed
on Linux (mod NSS 3.16.2).
This also is with respect to multiple origins sharing the same renderer process,
which is something we try to avoid as a matter of practice (on Desktop/CrOS).
This would present as a DoS bug, and the temporary risk seems low.

+  //
+  //  (2) The ID space is shared by different key types. So even if parameters
+  //      were validated by NSS, FindKeyByKeyID() could still return the wrong
+  //      key. For instance it might return a DH key that happens to have same
+  //      values for its public data.

[Ryan Sleevi, 2014/06/02 22:57:05]

Please add a note that, as explained, this is not an issue in practice.

[eroman, 2014/06/02 23:40:18]

Added a comment.

+  //
+  //  (3) Even if the above two problems were non-issues, there is no guarantee
+  //      that FindKeyByKeyID() returns the object just created. If the same
+  //      private key were imported multiple times, with different flags, using
+  //      the wrong one could lead to problems.

[Ryan Sleevi, 2014/06/02 22:57:05]

I still fail to see why (3) is an issue at all. The NSS flags should be
irrelevant for our operation here. If they aren't, it's a bug in our
implementation.

[eroman, 2014/06/02 23:40:18]

See https://codereview.chromium.org/213423007 and
https://codereview.chromium.org/269313004 for examples of where this was an
issue.

[Ryan Sleevi, 2014/06/02 23:55:57]

I'm still having trouble with this.

All WebCrypto keys within the renderer should have the same flags. Both of those
bugs seem to be issues where we weren't setting the *right* NSS flags, but your
description is where flags(key-a) != flags (key-b)

+  std::vector<uint8> cka_id(20);
+  crypto::RandBytes(&cka_id[0], cka_id.size());
+  AddOptionalAttribute(CKA_ID, CryptoData(cka_id), &key_template);
+  SECItem object_id = MakeSECItemForBuffer(CryptoData(cka_id));
+
   // Optional properties (all of these will have been specified or none).
   AddOptionalAttribute(CKA_PRIME_1, prime1, &key_template);
   AddOptionalAttribute(CKA_PRIME_2, prime2, &key_template);
   AddOptionalAttribute(CKA_EXPONENT_1, exponent1, &key_template);
   AddOptionalAttribute(CKA_EXPONENT_2, exponent2, &key_template);
   AddOptionalAttribute(CKA_COEFFICIENT, coefficient, &key_template);
 
   crypto::ScopedPK11Slot slot(PK11_GetInternalSlot());
 
   ScopedPK11GenericObject key_object(PK11_CreateGenericObject(
       slot.get(), &key_template[0], key_template.size(), PR_FALSE));
 
   if (!key_object)
     return Status::OperationError();
 
-  // The ID isn't guaranteed to be set by PKCS#11. However it is by softtoken so
-  // this should work.
-  SECItem object_id = {};
-  if (PK11_ReadRawAttribute(
-          PK11_TypeGeneric, key_object.get(), CKA_ID, &object_id) != SECSuccess)
-    return Status::OperationError();
-
   crypto::ScopedSECKEYPrivateKey private_key(
       PK11_FindKeyByKeyID(slot.get(), &object_id, NULL));
 
-  SECITEM_FreeItem(&object_id, PR_FALSE);
-
   if (!private_key)
     return Status::OperationError();
 
   blink::WebCryptoKeyAlgorithm key_algorithm;
   if (!CreatePrivateKeyAlgorithm(algorithm, private_key.get(), &key_algorithm))
     return Status::ErrorUnexpected();
 
   scoped_ptr<PrivateKey> key_handle;
   Status status =
       PrivateKey::Create(private_key.Pass(), key_algorithm, &key_handle);
@@ skipping 106 matching lines @@
   buffer->assign(key_data->data, key_data->data + key_data->len);
 
   return Status::Success();
 }
 
 }  // namespace platform
 
 }  // namespace webcrypto
 
 }  // namespace content