Align OpenSSL and NSS ChannelID formats.

crypto/ec_private_key_openssl.cc

Index: crypto/ec_private_key_openssl.cc
diff --git a/crypto/ec_private_key_openssl.cc b/crypto/ec_private_key_openssl.cc
index beda29fe13b7a5966027f335855966de2e415ac7..4c417ddf5b2353ae6ad2086efa5987cc9bb34e4a 100644
--- a/crypto/ec_private_key_openssl.cc
+++ b/crypto/ec_private_key_openssl.cc
@@ -117,28 +117,31 @@ ECPrivateKey* ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
     return NULL;
 
   OpenSSLErrStackTracer err_tracer(FROM_HERE);
-  // Write the encrypted private key into a memory BIO.
-  char* private_key_data = reinterpret_cast<char*>(
-      const_cast<uint8*>(&encrypted_private_key_info[0]));
-  int private_key_data_len =
-      static_cast<int>(encrypted_private_key_info.size());
-  ScopedBIO bio(BIO_new_mem_buf(private_key_data, private_key_data_len));
-  if (!bio.get())
-    return NULL;
 
-  // Convert it, then decrypt it into a PKCS#8 object.
-  ScopedX509_SIG p8_encrypted(d2i_PKCS8_bio(bio.get(), NULL));
-  if (!p8_encrypted.get())
+  const uint8_t* data = &encrypted_private_key_info[0];
+  const uint8_t* ptr = data;
+  ScopedX509_SIG p8_encrypted(
+      d2i_X509_SIG(NULL, &ptr, encrypted_private_key_info.size()));
+  if (!p8_encrypted || ptr != data + encrypted_private_key_info.size())
     return NULL;
 
-  ScopedPKCS8_PRIV_KEY_INFO p8_decrypted(PKCS8_decrypt(
-      p8_encrypted.get(), password.c_str(), static_cast<int>(password.size())));
-  if (!p8_decrypted.get() && password.empty()) {
-    // Hack for reading keys generated by ec_private_key_nss. Passing NULL
-    // causes OpenSSL to use an empty password instead of "\0\0".
-    p8_decrypted.reset(PKCS8_decrypt(p8_encrypted.get(), NULL, 0));
+  ScopedPKCS8_PRIV_KEY_INFO p8_decrypted;
+  if (password.empty()) {
+    // Hack for reading keys generated by an older version of the OpenSSL
+    // code. OpenSSL used to use "\0\0" rather than the empty string because it
+    // would treat the password as an ASCII string to be converted to UCS-2
+    // while NSS used a byte string.
+    p8_decrypted.reset(PKCS8_decrypt_pbe(
+        p8_encrypted.get(), reinterpret_cast<const uint8_t*>("\0\0"), 2));
   }
-  if (!p8_decrypted.get())
+  if (!p8_decrypted) {
+    p8_decrypted.reset(PKCS8_decrypt_pbe(
+        p8_encrypted.get(),
+        reinterpret_cast<const uint8_t*>(password.data()),
+        password.size()));
+  }
+
+  if (!p8_decrypted)
     return NULL;
 
   // Create a new EVP_PKEY for it.
@@ -164,14 +167,21 @@ bool ECPrivateKey::ExportEncryptedPrivateKey(
   // NOTE: NSS uses SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC
   // so use NID_pbe_WithSHA1And3_Key_TripleDES_CBC which should be the OpenSSL
   // equivalent.
-  ScopedX509_SIG encrypted(PKCS8_encrypt(NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
-                                         NULL,
-                                         password.c_str(),
-                                         static_cast<int>(password.size()),
-                                         NULL,
-                                         0,
-                                         iterations,
-                                         pkcs8.get()));
+  std::string password_copy = password;
+#if defined(OS_ANDROID)
+  // On Android, Channel IDs have historically been encrypted with "\0\0"
+  // because of a difference in OpenSSL and NSS behavior.
+  if (password_copy.empty())
+    password_copy.assign(2, '\0');
+#endif

[Ryan Sleevi, 2014/08/06 02:24:48]

I'm not sure I understand this bit. That is, why do you need to preserve
encrypting them like this? We're not going back on BoringSSL on Android (we've
crossed that rubicon, have we not?)

[davidben, 2014/08/06 18:34:35]

It was more about jumping back and forth across
https://codereview.chromium.org/205333002. But it looks like that's even in
stable, and we generally don't let you downgrade profiles anyway, so it probably
doesn't matter. I've removed it.

+  ScopedX509_SIG encrypted(PKCS8_encrypt_pbe(
+      NID_pbe_WithSHA1And3_Key_TripleDES_CBC,

[Ryan Sleevi, 2014/08/06 02:24:48]

Document why this constant is what it is.

[davidben, 2014/08/06 18:34:35]

Isn't it documented up in "Encrypt the object"?

[Ryan Sleevi, 2014/08/06 21:02:19]

Oh, I'm blind because of the ifdef. Ignore :)

+      reinterpret_cast<const uint8_t*>(password_copy.data()),
+      password_copy.size(),
+      NULL,
+      0,
+      iterations,
+      pkcs8.get()));
   if (!encrypted.get())
     return false;