Re: BUG #4869: No proper initialization of OpenSSL-Engine in libpq

Lars Kanis wrote:
> The following bug has been logged online:
>
> Bug reference:      4869
> Logged by:          Lars Kanis
> Email address:      kanis@comcard.de
> PostgreSQL version: 8.4rc1
> Operating system:   Linux c1170lx 2.6.24-23-generic #1 SMP Wed Apr 1
> 21:47:28 UTC 2009 i686 GNU/Linux
> Description:        No proper initialization of OpenSSL-Engine in libpq
> Details:
>
> When using OpenSSL-engine pkcs11 with PGSSLKEY=pkcs11:id_45 the
> authentication to the PG-server fails with "engine not initialized".
>
> According to the OpenSSL-docs
> (http://www.openssl.org/docs/crypto/engine.html) the structural reference
> returned by ENGINE_by_id needs to be initialized first before use. The
> buildin engine doesn't need this, but most of external engines don't work
> otherwise.
>
> Moreover the structural and functional references should be freed in any
> case.
>
>
> The following patch solves the problem:

This looks good in generael to me. I remember looking at the engine code
wondering why we didn't do that, but since I don't have a good
environment to test that part in, I forgot about it :(

Shouldn't there be an ENGINE_free() in the error path of ENGINE_init()?

Should we not also call ENGINE_finish() and ENGINE_free() in the success
path of this code? Your patch adds it to the case where we didn't get
the private key, but what if we did? I assume they should also go
outside the error path, per the attached patch - or will that break
their usage?

Can you test that and verify that it doesn't break for you?


--
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index bfc0965..53ac526 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -690,6 +690,20 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
                 return 0;
             }

+            if (ENGINE_init(engine_ptr) == 0)
+            {
+                char       *err = SSLerrmessage();
+
+                printfPQExpBuffer(&conn->errorMessage,
+                     libpq_gettext("could not initialize SSL engine \"%s\": %s\n"),
+                                  engine_str, err);
+                SSLerrfree(err);
+                ENGINE_free(engine_ptr);
+                free(engine_str);
+                ERR_pop_to_mark();
+                return 0;
+            }
+
             *pkey = ENGINE_load_private_key(engine_ptr, engine_colon,
                                             NULL, NULL);
             if (*pkey == NULL)
@@ -700,10 +714,14 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
                                   libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"),
                                   engine_colon, engine_str, err);
                 SSLerrfree(err);
+                ENGINE_finish(engine_ptr);
+                ENGINE_free(engine_ptr);
                 free(engine_str);
                 ERR_pop_to_mark();
                 return 0;
             }
+            ENGINE_finish(engine_ptr);
+            ENGINE_free(engine_ptr);
             free(engine_str);

             fnbuf[0] = '\0';    /* indicate we're not going to load from a