Fix Android cryptographic key pair storage.

net/base/openssl_private_key_store_android.cc

Index: net/base/openssl_private_key_store_android.cc
diff --git a/net/base/openssl_private_key_store_android.cc b/net/base/openssl_private_key_store_android.cc
index 3adc22238c381e482c382ba5624b3195467c05de..60ec2a4adfc61803d542d1df6e31ca931277f204 100644
--- a/net/base/openssl_private_key_store_android.cc
+++ b/net/base/openssl_private_key_store_android.cc
@@ -5,6 +5,7 @@
 #include "net/base/openssl_private_key_store.h"
 
 #include <openssl/evp.h>
+#include <openssl/x509.h>
 
 #include "base/logging.h"
 #include "base/memory/singleton.h"
@@ -19,17 +20,27 @@ class OpenSSLKeyStoreAndroid : public OpenSSLPrivateKeyStore {
  public:
   ~OpenSSLKeyStoreAndroid() {}
 
-  // TODO(joth): Use the |url| to help identify this key to the user.
-  // Currently Android has no UI to list these stored private keys (and no
-  // API to associate a name with them), so this is a non-issue.
   virtual bool StorePrivateKey(const GURL& url, EVP_PKEY* pkey) {
+    // Always clear openssl errors on exit.
+    crypto::OpenSSLErrStackTracer err_trace(FROM_HERE);
+    // Important: Do not use i2d_PublicKey() here, which returns data in

[joth, 2012/10/24 19:40:28]

nit: this may be a little easier on eyes to put a newline before each of the
Important: blocks.

[Ryan Sleevi, 2012/10/24 21:16:18]

+1

[digit1, 2012/10/25 14:34:32]

Done.

+    // PKKCS#1 format, use i2d_PUBKEY() which returns it as DER-encoded

[Ryan Sleevi, 2012/10/24 21:16:18]

typo: PKCS#1

[digit1, 2012/10/25 14:34:32]

Done.

+    // SubjectPublicKeyInfo (X.509), as expected by the platform.
     uint8* public_key = NULL;
-    int public_len = i2d_PublicKey(pkey, &public_key);
+    int public_len = i2d_PUBKEY(pkey, &public_key);

[Ryan Sleevi, 2012/10/24 21:16:18]

nit: I feel slightly better with a ScopedOpenSSL<uint8>, but not enough to
require nit

(PEDANTIC) BUG: Relying on unsigned char == uint8 is one of those pedantry
issues. It would be better to use "unsigned char*" here, rather than uint8, at
least when supplying a pointer-to-a-pointer.

[digit1, 2012/10/25 14:34:32]

That's not going to work because the ScopedOpenSSL<> template doesn't allow you
to get the address of its internal pointer, which is required by the call to
i2d_PUBKEY(). I could add a method to it to do that though if you feel it's
important enough.
Done.

+    // Important: Do not use i2d_PrivateKey() here, it returns data
+    // in a format that is incompatible with what the platform expects
+    // (i.e. this crashes the CertInstaller with an assertion error
+    // "error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag"

[Ryan Sleevi, 2012/10/24 21:16:18]

nit: This feels a leftover comment of the investigation, and suggests "We don't
know why it's doing this".

Seems like we should know why it was doing that, and comment appropriately.

[digit1, 2012/10/25 14:34:32]

When I investigated the issue, I didn't really understand the difference, even
after peeking into the OpenSSL source code. I didn't have really time to extract
the data and compare it so that was the best I could come up with. For now, I'll
just remove the above two lasts lines.

     uint8* private_key = NULL;
-    int private_len = i2d_PrivateKey(pkey, &private_key);
-
+    int private_len = 0;
+    PKCS8_PRIV_KEY_INFO* pkcs8 = EVP_PKEY2PKCS8(pkey);

[Ryan Sleevi, 2012/10/24 21:16:18]

nit: ScopedOpenSSL<EVP_PKEY2PKCS8>

[digit1, 2012/10/25 14:34:32]

Done.

+    if (pkcs8 != NULL) {
+      private_len = i2d_PKCS8_PRIV_KEY_INFO(pkcs8, &private_key);
+      PKCS8_PRIV_KEY_INFO_free(pkcs8);
+    }
     bool ret = false;
-    if (public_len && private_len) {
+    if (public_len > 0 && private_len > 0) {
       ret = net::android::StoreKeyPair(public_key, public_len, private_key,
                                        private_len);
     }