Bug with OpenSSL engine initialization in tls_engine_load_dynamic_generic

[David Woodhouse]

On Tue, 2016-06-07 at 09:50 +0200, Michael Schaller wrote:
> David, thanks for the details. I think I slowly get a timeline how
> this issue came into being:
> 1) 2002: OpenSSL enables automatic loading of engines by the
> ENGINE_by_id call:
> https://github.com/openssl/openssl/commit/aae329c447025eb87dab294d909f9fbc48f7174c
> 2) 2005: WPA Supplicant gets engine support:
> https://w1.fi/cgit/hostap-history/commit/?id=e7eb09e9652dd745e1df3649b79af70426ab6bc9
> 3) 2014: engine_pkcs11 defaults to p11-kit:
> https://github.com/OpenSC/engine_pkcs11/commit/d04bccbdb8c0607038be1fa4aa802268ad5c1edd
> 4) 2016: OpenSSL engines directory autoselection got fixed in
> engine_pkcs11: https://github.com/OpenSC/engine_pkcs11/commit/d04bccbdb8c0607038be1fa4aa802268ad5c1edd
> 5) 2016: Debian fixes the packaging to install libpkcs11.so in the
> correct directory:
> https://anonscm.debian.org/cgit/pkg-opensc/engine-pkcs11.git/commit/?id=0f9adff289380caf2887276d6e979871dbe174ba
> 
> IMHO no one is really to blame for this issue but rather changes piled
> up to lead to a breakage.
> From my point of view this went wrong:
> * The OpenSSL commit to enable engine autoloading (1) should IMHO have
> used a new function for autoloading rather than extending
> ENGINE_by_id. In any case they should have updated the documentation.

Hm, wait a moment. Let's take another look at those 'pre' and 'post'
commands. That isn't an OpenSSL thing; that's purely for the benefit of
our own tls_engine_load_engine_dynamic() function.

What we call 'pre' commands are actually the commands we give to the
"dynamic" engine to get it to load the engine we want — e.g.:

	char *engine_id = "pkcs11";
	const char *pre_cmd[] = {
		"SO_PATH", NULL /* pkcs11_so_path */,
		"ID", NULL /* engine_id */,
		"LIST_ADD", "1",
		/* "NO_VCHECK", "1", */
		"LOAD", NULL,
		NULL, NULL
	};

Then the "post" commands are the commands given to that engine before
we (later, from tls_engine_init()) call ENGINE_init() on it.

It's the *post* command which tells engine_pkcs11 where to find the
actual PKCS#11 module we want it to use (which can now be unspecified
and default to p11-kit-proxy.so).

So letting engine_pkcs11 get autoloaded by ENGINE_by_id() isn't a
problem at all, is it?

The only thing is that if it *does* get autoloaded, and if we have a
'post' command, we should still give it the 'post' command instead of
assuming it's already completely set up.

We can't actually tell if it's been autoloaded or not. But it does look
like it's harmless to give it the MODULE_PATH command even if it has
already been initialised. (We could probably do with fixing the engine
to return an error to the MODULE_PATH command if it's already
initialised with a different module, but that isn't a case that worked
for us before so I can live with that.)

So how about this...

diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index c831fba..23ac64b 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -729,10 +729,16 @@ static int tls_engine_load_dynamic_generic(const char *pre[],
 
 	engine = ENGINE_by_id(id);
 	if (engine) {
-		ENGINE_free(engine);
 		wpa_printf(MSG_DEBUG, "ENGINE: engine '%s' is already "
 			   "available", id);
-		return 0;
+		/*
+		 * If it was auto-loaded by ENGINE_by_id() we might still
+		 * need to tell it which PKCS#11 module to use in legacy
+		 * (non-p11-kit) environments. Do so now; even if it was
+		 * properly initialised before, setting it again will be
+		 * harmless.
+		 */
+		goto found;
 	}
 	ERR_clear_error();
 
@@ -769,7 +775,7 @@ static int tls_engine_load_dynamic_generic(const char *pre[],
 			   id, ERR_error_string(ERR_get_error(), NULL));
 		return -1;
 	}
-
+ found:
 	while (post && post[0]) {
 		wpa_printf(MSG_DEBUG, "ENGINE: '%s' '%s'", post[0], post[1]);
 		if (ENGINE_ctrl_cmd_string(engine, post[0], post[1], 0) == 0) {



-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5760 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/hostap/attachments/20160607/1830dfb9/attachment.bin>