Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
May 22, 2018 10:20AM
Hello!

On Fri, May 18, 2018 at 04:36:53AM -0400, Anderson Sasaki wrote:

> Hello,
>
> > 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
>
> The root of the problem is solved in the patch you pointed out
> above. The libp11-0.4.7 release is missing this
> EVP_PKEY_set1_engine() call. Without this, the engine is not
> properly associated with the EVP_PKEY object, preventing the
> OpenSSL automatic re-initialization of the engine to take place
> when the key is used.
>
> With the inclusion of such patch, the ENGINE_finish() can be
> safely called. As long as the key keeps the structural reference
> to the engine, it will be re-initialized when needed.
>
> I've tested in Fedora, where the same problem occurs. Since I am
> currently a co-maintainer of the engine in Fedora, I can fix it
> there. But I can't fix it on Ubuntu.

Ok, so it looks like:

- With OpenSSL 1.0.x, it seems that it is not at all possible to
correctly use keys loaded from an engine without this engine
being referenced elsewhere. Trying to use "ENGINE_init();
ENGINE_load_private_key(); ENGINE_finish();" will result in
a segmentation fault unless the engine is referenced elsewhere
(and hence "ENGINE_init()" is not needed).

- With OpenSSL 1.1.x, it is now possible to use
EVP_PKEY_set1_engine() to preserve a reference, but neither
OpenSSL 1.1.x nor fixed pkcs11 engine is available on
most platforms right now.

Commiting the patch as is means that we'll change an easily
fixable "not initialized" error to a segmentation fault for most
users affected by the patch. It doesn't look like a good
solution.

I don't like the idea of not calling ENGINE_finish() either, as
this approach can eventually result in a reference counter
overflow and another segmentation fault.

A simpliest solution seems to preserve things as is, at least for
now, requiring an explicit engine initialization either via
openssl.conf or via (misuse of) the "ssl_engine" directive.

I've also tried postponing ENGINE_finish() till
ngx_ssl_cleanup_ctx(), but I can't say I like this approach. Just
for the record, patch below:

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -114,6 +114,7 @@ int ngx_ssl_certificate_index;
int ngx_ssl_next_certificate_index;
int ngx_ssl_certificate_name_index;
int ngx_ssl_stapling_index;
+int ngx_ssl_engine_index;


ngx_int_t
@@ -225,6 +226,13 @@ ngx_ssl_init(ngx_log_t *log)
return NGX_ERROR;
}

+ ngx_ssl_engine_index = X509_get_ex_new_index(0, NULL, NULL, NULL, NULL);
+
+ if (ngx_ssl_engine_index == -1) {
+ ngx_ssl_error(NGX_LOG_ALERT, log, 0, "X509_get_ex_new_index() failed");
+ return NGX_ERROR;
+ }
+
return NGX_OK;
}

@@ -534,6 +542,16 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
return NGX_ERROR;
}

+ x509 = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);
+
+ if (X509_set_ex_data(x509, ngx_ssl_engine_index, engine) == 0) {
+ ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+ "X509_set_ex_data() failed");
+ ENGINE_finish(engine);
+ ENGINE_free(engine);
+ return NGX_ERROR;
+ }
+
*last++ = ':';

pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
@@ -546,7 +564,6 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
return NGX_ERROR;
}

- ENGINE_finish(engine);
ENGINE_free(engine);

if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {
@@ -3154,11 +3171,20 @@ ngx_ssl_cleanup_ctx(void *data)
{
ngx_ssl_t *ssl = data;

- X509 *cert, *next;
+ X509 *cert, *next;
+#ifndef OPENSSL_NO_ENGINE
+ ENGINE *engine;
+#endif

cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);

while (cert) {
+#ifndef OPENSSL_NO_ENGINE
+ engine = X509_get_ex_data(cert, ngx_ssl_engine_index);
+ if (engine) {
+ ENGINE_finish(engine);
+ }
+#endif
next = X509_get_ex_data(cert, ngx_ssl_next_certificate_index);
X509_free(cert);
cert = next;

--
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 836 April 25, 2018 11:54AM

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

Anderson Sasaki 254 April 25, 2018 12:12PM

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

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

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

Anderson Sasaki 178 April 25, 2018 01:50PM

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

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

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

Anderson Sasaki 172 April 26, 2018 12:40PM

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

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

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

Maxim Dounin 242 April 27, 2018 09:42AM

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

Anderson Sasaki 295 April 27, 2018 11:30AM

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

Maxim Dounin 192 May 03, 2018 10:34AM

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

Anderson Sasaki 283 May 10, 2018 12:44PM

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

Maxim Dounin 265 May 17, 2018 04:30PM

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

Anderson Sasaki 257 May 18, 2018 04:38AM

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

Maxim Dounin 597 May 22, 2018 10:20AM

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

Maxim Dounin 199 April 26, 2018 09:34AM

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

Anderson Sasaki 200 April 26, 2018 12:56PM



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

Online Users

Guests: 57
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready