Bug with OpenSSL engine initialization in tls_engine_load_dynamic_generic

[Michael Schaller]

David, thank you for commit f1931ec9d3d4ccdc277c170da5bf37e912e5cba4.
I've tested the patch and it works like a charm. :-)

On Tue, Jun 7, 2016 at 11:54 AM, David Woodhouse <dwmw2 at infradead.org> wrote:
> 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