Don't allow trailing data when creating an RSAPrivateKey.

crypto/rsa_private_key_nss.cc

 // Copyright (c) 2011 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/rsa_private_key.h"
 
 #include <cryptohi.h>
 #include <keyhi.h>
 #include <pk11pub.h>
 #include <secmod.h>
@@ skipping 260 matching lines @@
 RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfoWithParams(
     PK11SlotInfo* slot,
     const std::vector<uint8>& input,
     bool permanent,
     bool sensitive) {
   if (!slot)
     return NULL;
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
 
-  SECItem der_private_key_info;
-  der_private_key_info.data = const_cast<unsigned char*>(&input.front());
-  der_private_key_info.len = input.size();
+  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  if (!arena) {
+    NOTREACHED();
+    return NULL;
+  }
+
+  // Excess data is illegal, but NSS silently accepts it, so first ensure that
+  // |input| consists of a single ASN.1 element.
+  SECItem input_item, der_private_key_info;

[Ryan Sleevi, 2014/11/22 01:13:57]

nit: I tend to prefer we don't use this style in //net, as it can make it
visually difficult to find the type of a given variable.

[davidben, 2014/11/24 20:23:59]

Done.

+  input_item.data = const_cast<unsigned char*>(&input.front());
+  input_item.len = input.size();
+  SECStatus rv = SEC_QuickDERDecodeItem(arena.get(), &der_private_key_info,
+                                        SEC_ASN1_GET(SEC_AnyTemplate),
+                                        &input_item);
+  if (rv != SECSuccess)
+    return NULL;
+
   // 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;
-  // TODO(davidben): PK11_ImportDERPrivateKeyInfoAndReturnKey calls NSS's
-  // SEC_ASN1DecodeItem which does not enforce that there is no trailing
-  // data.
-  SECStatus rv =  PK11_ImportDERPrivateKeyInfoAndReturnKey(
+  rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
       slot, &der_private_key_info, NULL, NULL, permanent, sensitive,
       key_usage, &result->key_, NULL);
-  if (rv != SECSuccess) {
-    NOTREACHED();
+  if (rv != SECSuccess)
     return NULL;
-  }
 
   result->public_key_ = SECKEY_ConvertToPublicKey(result->key_);
-  if (!result->public_key_) {
-    NOTREACHED();
+  if (!result->public_key_)
     return NULL;
-  }
 
   return result.release();
 }
 
 #if defined(USE_NSS)
 // static
 RSAPrivateKey* RSAPrivateKey::InitPublicPart(const std::vector<uint8>& input) {
   EnsureNSSInit();
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey());
   result->public_key_ = GetRSAPublicKey(input).release();
   if (!result->public_key_) {
     NOTREACHED();
     return NULL;
   }
 
   return result.release();
 }
 #endif  // defined(USE_NSS)
 
 }  // namespace crypto