Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Maxim Dounin
May 17, 2018 04:30PM
Hello!

On Thu, May 10, 2018 at 12:42:58PM -0400, Anderson Sasaki wrote:

> Hello,
>
> Thanks again for the feedback.
>
> > In no particular order:
> >
> > - Should be "SSL: added ..." (no capital letter after a semicolon,
> > prefer past tense).
> >
> > - An empty line after the summary.
> >
> > - Please prefer double spacing.
> >
> > - "uniNItialized"
>
> The proposed changes were applied in the new patch version.
>
> > > diff -r 46c0c7ef4913 -r f5b0a7910922 src/event/ngx_event_openssl.c
> > > --- a/src/event/ngx_event_openssl.c Wed Apr 25 14:57:24 2018 +0300
> > > +++ b/src/event/ngx_event_openssl.c Fri Apr 27 16:58:16 2018 +0200
> > > @@ -527,6 +527,13 @@
> > > return NGX_ERROR;
> > > }
> > >
> > > + if (!ENGINE_init(engine)) {
> >
> > As previously noted at [1], this may need ENGINE_finish() to avoid
> > leaking loaded engines.
>
> Added ENGINE_finish() calls.
>
> >
> > > + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> > > + "ENGINE_init(engine) failed");
> >
> > There is no reason to log static string "engine" here. Consider
> > using engine name here.
>
> Logging the engine name instead.
>
> The patch follows:
>
> # HG changeset patch
> # User Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
> # Date 1525954250 -7200
> # Thu May 10 14:10:50 2018 +0200
> # Node ID e7d0387ad229ca47514f346c8b692e6f388d72e1
> # Parent ceab908790c4eb7cd01c40bd16581ef794222ca5
> SSL: added ENGINE_init() call before loading key.
>
> It is necessary to call ENGINE_init() before using an OpenSSL engine
> to get the engine functional reference. Without this, when
> ENGINE_load_private_key() is called, the engine is still uninitialized.
>
> diff -r ceab908790c4 -r e7d0387ad229 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c Tue Apr 24 14:04:59 2018 +0300
> +++ b/src/event/ngx_event_openssl.c Thu May 10 14:10:50 2018 +0200
> @@ -527,6 +527,13 @@
> return NGX_ERROR;
> }
>
> + if (!ENGINE_init(engine)) {
> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> + "ENGINE_init(\"%s\") failed", p);
> + ENGINE_free(engine);
> + return NGX_ERROR;
> + }
> +
> *last++ = ':';
>
> pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
> @@ -534,10 +541,12 @@
> if (pkey == NULL) {
> ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> "ENGINE_load_private_key(\"%s\") failed", last);
> + ENGINE_finish(engine);
> ENGINE_free(engine);
> return NGX_ERROR;
> }
>
> + ENGINE_finish(engine);
> ENGINE_free(engine);
>
> if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {

The patch looks correct to me. Though it causes a segmentation
faults within pkcs11 engine when using such loaded keys at least
on Ubuntu 18.04 (OpenSSL 1.1.0g, pkcs11 engine from libp11 0.4.7).
Segmentation faults can be reproduced with the test you've sent
earlier.

Using an explitic "init = 1" in openssl.conf resolves this, as
well as commenting out ENGINE_finish(), so it looks like it cannot
handle ENGINE_finish() while certificates loaded from the engine
are still in use.

Possible options might be:

- avoid any changes, and require "init = 1" as we effectively do
now;

- add explicit lists of engines initialized, and call
ENGINE_finish() once no longer needed (probably somewhere in
ngx_ssl_cleanup_ctx());

- avoid calling ENGINE_finish() with appropriate explanation of
the problem;

- dig further into what goes on in OpenSSL / pkcs11 engine, and
fix things (might be already resolved in [1]).

[1] https://github.com/OpenSC/libp11/commit/da725ab727342083478150a203a3c80c4551feb4

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 157 April 25, 2018 11:54AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 16 April 25, 2018 12:12PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Пичулин Дмитрий Николаевич 13 April 25, 2018 12:42PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 15 April 25, 2018 01:50PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Пичулин Дмитрий Николаевич 15 April 25, 2018 04:00PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 11 April 26, 2018 12:40PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Пичулин Дмитрий Николаевич 13 April 26, 2018 03:34PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Maxim Dounin 19 April 27, 2018 09:42AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 27 April 27, 2018 11:30AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Maxim Dounin 17 May 03, 2018 10:34AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 12 May 10, 2018 12:44PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Maxim Dounin 9 May 17, 2018 04:30PM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 8 May 18, 2018 04:38AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Maxim Dounin 17 April 26, 2018 09:34AM

Re: [PATCH] SSL: Add ENGINE_init() calls before using engines.

Anderson Sasaki 13 April 26, 2018 12:56PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 52
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 214 on March 20, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready