Don't allow trailing data when creating an RSAPrivateKey.

CreateFromPrivateKeyInfo should not silently discard data after the
PrivateKeyInfo.

BUG=432279

Review URL: https://codereview.chromium.org/713523004

Cr-Commit-Position: refs/heads/master@{#305503}

crypto/rsa_private_key_nss.cc

@@ -278,29 +278,37 @@ RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfoWithParams(
 
   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();
-  // 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(
-      slot, &der_private_key_info, NULL, NULL, permanent, sensitive,
-      key_usage, &result->key_, NULL);
-  if (rv != SECSuccess) {
+  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  if (!arena) {
     NOTREACHED();
     return NULL;
   }
 
-  result->public_key_ = SECKEY_ConvertToPublicKey(result->key_);
-  if (!result->public_key_) {
-    NOTREACHED();
+  // 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());
+  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 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;
+  rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
+      slot, &der_private_key_info, NULL, NULL, permanent, sensitive,
+      key_usage, &result->key_, NULL);
+  if (rv != SECSuccess)
+    return NULL;
+
+  result->public_key_ = SECKEY_ConvertToPublicKey(result->key_);
+  if (!result->public_key_)
     return NULL;
-  }
 
   return result.release();
 }

crypto/rsa_private_key_openssl.cc

@@ -83,13 +83,10 @@ RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfo(
   // Importing is a little more involved than exporting, as we must first
   // PKCS#8 decode the input, and then import the EVP_PKEY from Private Key
   // Info structure returned.
-  //
-  // TODO(davidben): This should check that |ptr| advanced to the end of |input|
-  // to ensure there is no trailing data.
   const uint8_t* ptr = &input[0];
   ScopedPKCS8_PRIV_KEY_INFO p8inf(
       d2i_PKCS8_PRIV_KEY_INFO(nullptr, &ptr, input.size()));
-  if (!p8inf.get())
+  if (!p8inf.get() || ptr != &input[0] + input.size())
     return NULL;
 
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);

crypto/rsa_private_key_unittest.cc

@@ -150,6 +150,20 @@ TEST(RSAPrivateKeyUnitTest, CopyTest) {
   ASSERT_EQ(input, privkey_copy);
 }
 
+// Test that CreateFromPrivateKeyInfo fails if there is extra data after the RSA
+// key.
+TEST(RSAPrivateKeyUnitTest, ExtraData) {
+  std::vector<uint8> input(
+      kTestPrivateKeyInfo, kTestPrivateKeyInfo + sizeof(kTestPrivateKeyInfo));
+  input.push_back(0);
+
+  scoped_ptr<crypto::RSAPrivateKey> key(
+      crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
+
+  // Import should fail.
+  EXPECT_FALSE(key);
+}
+
 
 // Verify that generated public keys look good. This test data was generated
 // with the openssl command line tool.