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

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On 22 jun 2009, at 18.05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm also a bit concerned about wrapping a struct
>>> field inside such an #if, as that's likely to cause hard-to-debug
>>> problems if two compilations read libpq-int.h with different #define
>>> environments.
>
>> Since it's a pointer we could just declare it as void, no? Or even #if
>> between void and "real" pointer. Wouldn't that be safe?
>
> Yeah, on reflection I think it'd be safe to do
>
> #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
>     ENGINE        *ptr;
> #else
>     /* dummy field to keep struct the same if OpenSSL version changes */
>     void        *ptr;
> #endif
>
> (probably that #if should get wrapped up in an additional #define
> instead of being duplicated in several places...)

Attached is an updated patch that does both of these.


>> (we already have a similar issue with the whole #ifdef use_ssl, don't
>> we? But having one isn't an excuse to create another of course…)
>
> That's controlled by an entry in pg_config.h, though, and won't change
> if someone happens to update their openssl install and then recompile
> only parts of libpq.

True, agreed.

--
 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
***************
*** 31,36 ****
--- 31,37 ----
  #include "libpq-fe.h"
  #include "fe-auth.h"
  #include "pqsignal.h"
+ #include "libpq-int.h"

  #ifdef WIN32
  #include "win32.h"
***************
*** 62,68 ****
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
  #include <openssl/conf.h>
  #endif
! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
  #include <openssl/engine.h>
  #endif

--- 63,69 ----
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
  #include <openssl/conf.h>
  #endif
! #ifdef USE_SSL_ENGINE
  #include <openssl/engine.h>
  #endif

***************
*** 661,667 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
       */
      if (conn->sslkey && strlen(conn->sslkey) > 0)
      {
! #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
          if (strchr(conn->sslkey, ':')
  #ifdef WIN32
              && conn->sslkey[1] != ':'
--- 662,668 ----
       */
      if (conn->sslkey && strlen(conn->sslkey) > 0)
      {
! #ifdef USE_SSL_ENGINE
          if (strchr(conn->sslkey, ':')
  #ifdef WIN32
              && conn->sslkey[1] != ':'
***************
*** 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();

--- 670,683 ----
              )
          {
              /* 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)
              {
--- 690,711 ----
                  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)
--- 715,723 ----
                                    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)
--- 1235,1249 ----
          X509_free(conn->peer);
          conn->peer = NULL;
      }
+
+ #ifdef USE_SSL_ENGINE
+     if (conn->engine)
+     {
+         ENGINE_finish(conn->engine);
+         ENGINE_free(conn->engine);
+         conn->engine = NULL;
+     }
+ #endif
  }

  /*
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
***************
*** 76,83 **** typedef struct
--- 76,88 ----
  #ifdef USE_SSL
  #include <openssl/ssl.h>
  #include <openssl/err.h>
+
+ #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
+ #define USE_SSL_ENGINE
  #endif

+ #endif /* USE_SSL */
+
  /*
   * POSTGRES backend dependent Constants.
   */
***************
*** 383,389 **** struct pg_conn
--- 388,400 ----
      X509       *peer;            /* X509 cert of server */
      char        peer_dn[256 + 1];        /* peer distinguished name */
      char        peer_cn[SM_USER + 1];    /* peer common name */
+ #ifdef USE_SSL_ENGINE
+     ENGINE       *engine;            /* SSL engine, if any */
+ #else
+     void       *engine;            /* dummy field to keep struct the same
+                                    if OpenSSL version changes */
  #endif
+ #endif /* USE_SSL */

  #ifdef ENABLE_GSS
      gss_ctx_id_t gctx;            /* GSS context */