Bug with OpenSSL engine initialization in tls_engine_load_dynamic_generic

[Michael Schaller]

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.
* The author(s) of the WPA Supplicant engine support (2) probably
didn't know about the autoloading attempt / side effect of
ENGINE_by_id and because of the lack of documentation and because the
pkcs11 engine typically wasn't autoloadable they assumed that it could
be used to determine if an engine is already loaded.

The really tricky question is how to fix this issue. IMHO the
ENGINE_by_id behavior can't be fixed because the autoloading is part
of the API. This behavior should at least be documented though.
That leaves us with fixing WPA Supplicant and here I don't know what's
the best way to fix it. First of all the pkcs11 engine gets
initialized as part of the tls_init call and I don't know how often
tls_init is called and if it makes sense to skip the engine
initialization if it has been already initialized previously. Secondly
I also don't know if the configuration step should be skipped if the
pkcs11 engine is already loaded as the engine could be differently
configured to what is needed. Lastly some kind of check if the pkcs11
engine is already loaded is probably sensible as reinitialization
doesn't work without prior removal/unload if the engine.

David, what you propose with regards to skipping the first
ENGINE_by_id call if pre commands are given would work as long as the
pkcs11 engine isn't initialized yet for whatever reason. IMHO it would
be sensible to have a check if the engine is already loaded, as
proposed in my patch, and then the engine should be probably unloaded
so that it can be reloaded with the proper configuration. What do you
think?

On Tue, Jun 7, 2016 at 8:51 AM, David Woodhouse <dwmw2 at infradead.org> wrote:
> On Mon, 2016-06-06 at 17:56 +0200, Michael Schaller wrote:
>>
>> For me only remains one topic then. If specifying the pkcs11 engine
>> and module path is on the way of deprecation (but IMHO not quite there
>> yet) is it then worth fixing this issue? If yes, what about the
>> proposed patch to not use ENGINE_by_id to check if an engine has been
>> already loaded?
>
> If I broke something last year (actually, I think it was December 2014)
> when I cleaned up the auto-load code paths, then sure — we should
> probably fix that.
>
> If it's something that basically never worked, then there's no point in
> fixing it now.
>
> I suppose that if we take the holistic view, I really did break it last
> year — not by anything I changed in hostap/wpa_supplicant, but by
> changing engine_pkcs11 to install into the standand engine directory so
> that ENGINE_by_id() *can* now find it.
>
> Perhaps just avoid the ENGINE_by_id() *if* there are explicit pre
> commands. In that case we'll do it through the dynamic engine anyway,
> and we don't need the fallback of iterating over the list?
>
> (That code path is broken if you need to use the dynamic engine to load
> the one you want, and if there are *no* 'pre' commands, right?...)
>
> --
> dwmw2