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

Lars Kanis wrote:
>>> 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()?
> In the patch it is already there, isn't it?

Eh. So it is. Don't know where I got the idea that it didn't :-)


>> 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?
> That's right. I thought about it, but I don't know where the right place is.
>
>> Can you test that and verify that it doesn't break for you?
> It breaks with Segmentation fault in my smartcard-based setup. The pkey-handle
> is all we have from the engine, when client_cert_cb() is finished. Obviously
> the ref-counting of openssl does not take the pkey-handle into account, so we
> need to keep the engine_ptr for later freeing.

So we need to keep the engine initialized during this time? Ugh. We
don't currently carry around the engine pointer. I guess we have to.

> close_SSL() should be the right place for ENGINE_finish() and ENGINE_free() ?

Yup.

How about the attached patch? Does it work for you?

A question from that then, for others, is it Ok to add a field to the
PGconn structure during RC? :-) It's only in libpq-int.h, but? Comments?
Tom, perhaps?

--
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 669,683 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
              )
          {
              /* Colon, but not in second character, treat as engine:key */
-             ENGINE       *engine_ptr;
              char       *engine_str = strdup(conn->sslkey);
              char       *engine_colon = strchr(engine_str, ':');

              *engine_colon = '\0';        /* engine_str now has engine name */
              engine_colon++;        /* engine_colon now has key name */

!             engine_ptr = ENGINE_by_id(engine_str);
!             if (engine_ptr == NULL)
              {
                  char       *err = SSLerrmessage();

--- 669,682 ----
              )
          {
              /* Colon, but not in second character, treat as engine:key */
              char       *engine_str = strdup(conn->sslkey);
              char       *engine_colon = strchr(engine_str, ':');

              *engine_colon = '\0';        /* engine_str now has engine name */
              engine_colon++;        /* engine_colon now has key name */

!             conn->engine = ENGINE_by_id(engine_str);
!             if (conn->engine == NULL)
              {
                  char       *err = SSLerrmessage();

***************
*** 690,696 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
                  return 0;
              }

!             *pkey = ENGINE_load_private_key(engine_ptr, engine_colon,
                                              NULL, NULL);
              if (*pkey == NULL)
              {
--- 689,710 ----
                  return 0;
              }

!             if (ENGINE_init(conn->engine) == 0)
!             {
!                 char       *err = SSLerrmessage();
!
!                 printfPQExpBuffer(&conn->errorMessage,
!                      libpq_gettext("could not initialize SSL engine \"%s\": %s\n"),
!                                   engine_str, err);
!                 SSLerrfree(err);
!                 ENGINE_free(conn->engine);
!                 conn->engine = NULL;
!                 free(engine_str);
!                 ERR_pop_to_mark();
!                 return 0;
!             }
!
!             *pkey = ENGINE_load_private_key(conn->engine, engine_colon,
                                              NULL, NULL);
              if (*pkey == NULL)
              {
***************
*** 700,705 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
--- 714,722 ----
                                    libpq_gettext("could not read private SSL key \"%s\" from engine \"%s\": %s\n"),
                                    engine_colon, engine_str, err);
                  SSLerrfree(err);
+                 ENGINE_finish(conn->engine);
+                 ENGINE_free(conn->engine);
+                 conn->engine = NULL;
                  free(engine_str);
                  ERR_pop_to_mark();
                  return 0;
***************
*** 1217,1222 **** close_SSL(PGconn *conn)
--- 1234,1246 ----
          X509_free(conn->peer);
          conn->peer = NULL;
      }
+
+     if (conn->engine)
+     {
+         ENGINE_finish(conn->engine);
+         ENGINE_free(conn->engine);
+         conn->engine = NULL;
+     }
  }

  /*
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
***************
*** 383,388 **** struct pg_conn
--- 383,389 ----
      X509       *peer;            /* X509 cert of server */
      char        peer_dn[256 + 1];        /* peer distinguished name */
      char        peer_cn[SM_USER + 1];    /* peer common name */
+     ENGINE       *engine;            /* SSL engine, if any */
  #endif

  #ifdef ENABLE_GSS