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@xxxxxxxxx Intel Corporation
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
