Bug with OpenSSL engine initialization in tls_engine_load_dynamic_generic

[Michael Schaller]

Jouni, thank you for committing the patches.
David, Jouni, how about adding a log message that states that the
pkcs11 engine and module path usage is deprecated and that they should
switch to p11-kit URIs?

FYI: I've opened a bug with Debian to include the patch in their
packaging: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=827253

On Tue, Jun 7, 2016 at 4:49 PM, Michael Schaller <misch at google.com> wrote:
> 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