Leak a reference to the ENGINE in the legacy client auth codepath.

net/android/keystore_openssl.cc

Index: net/android/keystore_openssl.cc
diff --git a/net/android/keystore_openssl.cc b/net/android/keystore_openssl.cc
index afdca30de0d772d51261d9dfa7dbda242374e0bf..38fa46b846ac9e15861226d166bb9cad9ff52ff6 100644
--- a/net/android/keystore_openssl.cc
+++ b/net/android/keystore_openssl.cc
@@ -310,6 +310,44 @@ bool GetRsaPkeyWrapper(jobject private_key, EVP_PKEY* pkey) {
   return true;
 }
 
+// On Android < 4.2, the libkeystore.so ENGINE uses CRYPTO_EX_DATA and is not
+// added to the global engine list. If all references to it are dropped, OpenSSL
+// will dlclose the module, leaving a dangling function pointer in the RSA
+// CRYPTO_EX_DATA class. To work around this, leak an extra reference to the
+// ENGINE we extract in GetRsaLegacyKey.
+//
+// In 4.2, this change avoids the problem:
+// https://android.googlesource.com/platform/libcore/+/106a8928fb4249f2f3d4dba1dddbe73ca5cb3d61
+//
+// https://crbug.com/381465
+class KeystoreEngineWorkaround {
+public:

[Ryan Sleevi, 2014/06/17 21:55:37]

indent 1 space

[davidben, 2014/06/17 23:43:18]

Done.

+  KeystoreEngineWorkaround() : leaked_(false) {}
+
+  void LeakRsaEngine(EVP_PKEY* pkey) {
+    if (leaked_)
+      return;
+    ScopedRSA rsa(EVP_PKEY_get1_RSA(pkey));
+    if (!rsa.get() ||
+        !rsa.get()->engine ||
+        strcmp(ENGINE_get_id(rsa.get()->engine), "keystore") ||
+        !ENGINE_init(rsa.get()->engine)) {

[Ryan Sleevi, 2014/06/17 21:55:37]

Don't use get()-> for operator->
Don't use get() for boolean

if (!rsa || !rsa->engine || strcmp(ENGINE_get_id(rsa->engine), "keystore") ||
!ENGINE_init(rsa->engine) {
}

[davidben, 2014/06/17 23:43:17]

ScopedOpenSSL doesn't actually provide a whole lot:
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/openssl_uti...

+      NOTREACHED();
+      return;
+    }
+    leaked_ = true;
+  }
+
+private:

[Ryan Sleevi, 2014/06/17 21:55:37]

indent 1 space

[davidben, 2014/06/17 23:43:17]

Done.

+  bool leaked_;

[Ryan Sleevi, 2014/06/17 21:55:37]

s/leaked_/leaked_engine_/ perhaps?

Just so it's clear what you leaked (eg: you're not leaking an EVP_PKEY to keep
the engine alive)

[davidben, 2014/06/17 23:43:17]

Done.

+};
+
+void LeakRsaEngine(EVP_PKEY* pkey) {
+  static base::LazyInstance<KeystoreEngineWorkaround> s_instance =

[Ryan Sleevi, 2014/06/17 21:55:37]

this should be a ::Leaky lazy instance

[davidben, 2014/06/17 23:43:18]

Done.

+      LAZY_INSTANCE_INITIALIZER;
+  s_instance.Get().LeakRsaEngine(pkey);
+}
+
 // Setup an EVP_PKEY to wrap an existing platform RSA PrivateKey object
 // for Android 4.0 to 4.1.x. Must only be used on Android < 4.2.
 // |private_key| is a JNI reference (local or global) to the object.
@@ -320,6 +358,7 @@ EVP_PKEY* GetRsaLegacyKey(jobject private_key) {
       GetOpenSSLSystemHandleForPrivateKey(private_key);
   if (sys_pkey != NULL) {
     CRYPTO_add(&sys_pkey->references, 1, CRYPTO_LOCK_EVP_PKEY);
+    LeakRsaEngine(sys_pkey);
   } else {
     // GetOpenSSLSystemHandleForPrivateKey() will fail on Android
     // 4.0.3 and earlier. However, it is possible to get the key