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

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

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.

This is exacerbated by https://codereview.chromium.org/27500004 which, at least
on 4.1.2, causes the session cache and OpenSSLClientKeyStore to get dumped
every time a client auth prompt comes up (https://crbug.com/381912).

In 4.2, this change avoids the problem:
https://android.googlesource.com/platform/libcore/+/106a8928fb4249f2f3d4dba1dddbe73ca5cb3d61

BUG=381465
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278305

[davidben, 2014-06-06 22:25:01]

Steps to repro are:
1. Log in with client auth to some site.
2. Use the browser a little; you need Java's GC to fire and free the Java
wrapper objects.
3. Log in with client auth to some other site so the prompt comes up.

Printing private_key->references just before the EVP_PKEY_free in ~KeyPair()
helps to verify 2 was done enough. If you reproduced the conditions necessary
for the bug, you will see references = 1 just before calling EVP_PKEY_free. For
extra bonus points, reach into the RSA on the EVP_PKEY and print the struct_ref
of rsa->engine. The full conditions are that both those reference counts are 1.

Without this patch, freeing that key will likely crash. The key still holds a
reference to the ENGINE. Assuming nothing else still holds a reference,
libkeystore.so will be dlclose'd in RSA_free. Conveniently, this happens before
the CRYPTO_EX_DATA is freed, so the crash will happen immediately afterwards.
(Reordering those calls in OpenSSL is NOT sufficient to prevent the crash. If
they were reordered, this RSA_free invocation wouldn't crash, but the next one
would.)

[Ryan Sleevi, 2014-06-06 23:23:56]

+palmer

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

LGTM, mod nits.

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl.cc
File net/android/keystore_openssl.cc (right):

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:324: public:
indent 1 space

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:334: !ENGINE_init(rsa.get()->engine)) {
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) {
}

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:341: private:
indent 1 space

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:342: bool leaked_;
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)

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:346: static
base::LazyInstance<KeystoreEngineWorkaround> s_instance =
this should be a ::Leaky lazy instance

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

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl.cc
File net/android/keystore_openssl.cc (right):

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:324: public:
On 2014/06/17 21:55:37, Ryan Sleevi wrote:
> indent 1 space

Done.

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:334: !ENGINE_init(rsa.get()->engine)) {
On 2014/06/17 21:55:37, Ryan Sleevi wrote:
> 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) {
> }

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

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:341: private:
On 2014/06/17 21:55:37, Ryan Sleevi wrote:
> indent 1 space

Done.

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:342: bool leaked_;
On 2014/06/17 21:55:37, Ryan Sleevi wrote:
> 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)

Done.

https://codereview.chromium.org/322533003/diff/1/net/android/keystore_openssl...
net/android/keystore_openssl.cc:346: static
base::LazyInstance<KeystoreEngineWorkaround> s_instance =
On 2014/06/17 21:55:37, Ryan Sleevi wrote:
> this should be a ::Leaky lazy instance

Done.

[Kenny Root (Google), 2014-06-18 23:53:41]

lgtm

[davidben, 2014-06-19 00:04:19]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-19 00:06:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/322533003/20001

[commit-bot: I haz the power, 2014-06-19 07:55:07]

Change committed as 278305