BUG #4869: No proper initialization of OpenSSL-Engine in libpq
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:
diff -ur postgresql-8.4rc1.orig/src/interfaces/libpq/fe-secure.c
postgresql-8.4rc1/src/interfaces/libpq/fe-secure.c
--- postgresql-8.4rc1.orig/src/interfaces/libpq/fe-secure.c 2009-06-11
16:49:14.000000000 +0200
+++ postgresql-8.4rc1/src/interfaces/libpq/fe-secure.c 2009-06-22
10:56:38.000000000 +0200
@@ -689,6 +689,20 @@
ERR_pop_to_mark();
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);
@@ -700,6 +714,8 @@
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;
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/
Attachments:
sslengine.patchtext/x-diff; name=sslengine.patchDownload+18-0
Hi Magnus,
thanks for reply.
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?
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.
close_SSL() should be the right place for ENGINE_finish() and ENGINE_free() ?
--
Mit freundlichen Grüßen,
Lars Kanis
Bereichsleiter IT
Tel +49 3745 769 -422
Fax +49 3745 769 -334
eMail: kanis@comcard.de
Sie können sich unter http://www.comcard.de unseren Newsletter abonnieren!
ComCard GmbH
Hammerbrücker Straße 3
08223 Falkenstein
Geschäftsführer: Dipl.-Ing. Ralph Siegel
Amtgericht Chemnitz HRB 3255
Ust.ID DE811118514
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/
Attachments:
sslengine.patchtext/x-diff; name=sslengine.patchDownload+32-22
Magnus Hagander <magnus@hagander.net> writes:
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?
Changing PGconn internals doesn't bother me, but ...
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not really
excited about trying to fix it in RC at all. The chances of breaking
something seem much higher than the usefulness of the fix would warrant.
I'd suggest holding the matter until 8.5 development opens.
regards, tom lane
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
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?Changing PGconn internals doesn't bother me, but ...
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not really
excited about trying to fix it in RC at all. The chances of breaking
something seem much higher than the usefulness of the fix would warrant.I'd suggest holding the matter until 8.5 development opens.
I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why Lars is
using it now, but wasn't using it before. Correct, Lars?
That would be the argument for doing it now. We previously supported
engines for client certificates, but using client certificates at all
wasn't very useful in pre-8.4, and that's why it wasn't used almost at
all. While I don't expect a huge number of users of it in 8.4, I think
it is a *much* more useful feature now, and thus will be used a lot more.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Am Montag, 22. Juni 2009 15:55:58 schrieben Sie:
Lars Kanis wrote:
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?
Yes, it works perfect. Now, if I close the connection, the engine is closed
too, so I have to type the PIN another time for a next connection. That
should be the correct behaviour.
Show quoted text
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?
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
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?Changing PGconn internals doesn't bother me, but ...
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not really
excited about trying to fix it in RC at all. The chances of breaking
something seem much higher than the usefulness of the fix would warrant.I'd suggest holding the matter until 8.5 development opens.
I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why Lars is
using it now, but wasn't using it before. Correct, Lars?
That's right. Because clientside crypto engines and proper certificate
authentication is supported now, I would like to use a strong smartcard-based
login in our high security environment.
That would be the argument for doing it now. We previously supported
engines for client certificates, but using client certificates at all
wasn't very useful in pre-8.4, and that's why it wasn't used almost at
all. While I don't expect a huge number of users of it in 8.4, I think
it is a *much* more useful feature now, and thus will be used a lot more.
I could live with the patch during 8.4 cycle. But if we support crypto engines
now, we may do it the way that it really works.
regards
Lars
Lars Kanis <kanis@comcard.de> writes:
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
Tom Lane wrote:
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not really
excited about trying to fix it in RC at all. The chances of breaking
something seem much higher than the usefulness of the fix would warrant.
I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why Lars is
using it now, but wasn't using it before. Correct, Lars?
That's right. Because clientside crypto engines and proper certificate
authentication is supported now, I would like to use a strong smartcard-based
login in our high security environment.
OK, but I'm still worried about making a change of this sort (ie,
modifying our interface to code that we don't control) so late in the
release cycle. It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
How about the attached patch? Does it work for you?
The existing references to typedef ENGINE and associated functions are
wrapped in
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
I rather imagine that this patch will cause complete failure in builds
where that's not true. 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.
regards, tom lane
On 22 jun 2009, at 18.05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
How about the attached patch? Does it work for you?
The existing references to typedef ENGINE and associated functions are
wrapped in#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined
(OPENSSL_NO_ENGINE)I rather imagine that this patch will cause complete failure in builds
where that's not true.
Yeah, that will have to be fixed.
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?
(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…)
/Magnus
On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Lars Kanis <kanis@comcard.de> writes:
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
Tom Lane wrote:
IIUC this is a pre-existing bug/limitation in an extremely seldom-
used
feature that we don't have any very good way to test. So I'm not
really
excited about trying to fix it in RC at all. The chances of
breaking
something seem much higher than the usefulness of the fix would
warrant.I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why
Lars is
using it now, but wasn't using it before. Correct, Lars?That's right. Because clientside crypto engines and proper
certificate
authentication is supported now, I would like to use a strong
smartcard-based
login in our high security environment.OK, but I'm still worried about making a change of this sort (ie,
modifying our interface to code that we don't control) so late in the
release cycle. It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?
I share your concerns in general. But I think we nee to take into
account that this simply does not work without the patch. So nobody
should rely on the previous behaviour - how would their application
work there... (I guess there is always a risk I get to eat those words
later if someone did, but I don't see the scenario...) in fact, this
is a but in an advertised feature in previous versions, so should
maybe even consider backpatching it base on that....
/Magnus
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...)
(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.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?
I share your concerns in general. But I think we nee to take into
account that this simply does not work without the patch.
Frankly, I don't much care whether openssl-with-an-external-engine
works; we never supported that before, and if it doesn't work now,
that's not a regression. What I'm worried about at this stage of the
release cycle is regressions, eg, does the code still compile at all
with openssl version 0.9.random-version-that-used-to-work.
regards, tom lane
Am Montag, 22. Juni 2009 17:46:22 schrieben Sie:
Lars Kanis <kanis@comcard.de> writes:
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
Tom Lane wrote:
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not
really excited about trying to fix it in RC at all. The chances of
breaking something seem much higher than the usefulness of the fix
would warrant.I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why Lars is
using it now, but wasn't using it before. Correct, Lars?That's right. Because clientside crypto engines and proper certificate
authentication is supported now, I would like to use a strong
smartcard-based login in our high security environment.OK, but I'm still worried about making a change of this sort (ie,
modifying our interface to code that we don't control) so late in the
release cycle. It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?
The 3 different versions of the engine-code seems to me like this:
1. unpatched libpq
engines aren't initialized (so some don't work), engines aren't freed at the
end of connection (memory leak), no changes to PGconn
2. the initial patch I posted
engines are initialized (so can work), engines aren't freed (memory leak), no
changes to PGconn only in engine code path
3. the current patch
engines are initialized, engines are freed, but problematic changes to PGconn
Maybe version 2 (my initial patch) could be an alternative ?
regards
Lars Kanis
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
On 22 jun 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?I share your concerns in general. But I think we nee to take into
account that this simply does not work without the patch.Frankly, I don't much care whether openssl-with-an-external-engine
works; we never supported that before, and if it doesn't work now,
Sure we did. It's in 8.3.
It may not have ever worked properly, but that's a different thing.
According do our code and documentation, the feature is there and supported.
that's not a regression. What I'm worried about at this stage of the
release cycle is regressions, eg, does the code still compile at all
with openssl version 0.9.random-version-that-used-to-work.
Right, and we don't cover *that* many different versions with the
buildfarm...
So how about this:
1) We add a note to the documentation that states explicitly that this
only works for engines that don't require intialization.
2) We add a section on the wiki for patches that fix known issues in
8.4, but that may cause behavioral changes and thus don't get applied to
the main code until 8.5. I'm sure there will be other patches on this
list eventually (after release time).
Perhaps even a reference from #1 to #2?
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Lars Kanis wrote:
Am Montag, 22. Juni 2009 17:46:22 schrieben Sie:
Lars Kanis <kanis@comcard.de> writes:
Am Montag, 22. Juni 2009 16:38:32 schrieben Sie:
Tom Lane wrote:
IIUC this is a pre-existing bug/limitation in an extremely seldom-used
feature that we don't have any very good way to test. So I'm not
really excited about trying to fix it in RC at all. The chances of
breaking something seem much higher than the usefulness of the fix
would warrant.I think we'll see this feature used a lot more now, since we support
client certificate authentication. I bet that's the reason why Lars is
using it now, but wasn't using it before. Correct, Lars?That's right. Because clientside crypto engines and proper certificate
authentication is supported now, I would like to use a strong
smartcard-based login in our high security environment.OK, but I'm still worried about making a change of this sort (ie,
modifying our interface to code that we don't control) so late in the
release cycle. It seems like there is large potential for failure in
contexts other than the one or two you are going to be able to test
right now. Is there anything that can be done to reduce the risk?The 3 different versions of the engine-code seems to me like this:
1. unpatched libpq
engines aren't initialized (so some don't work), engines aren't freed at the
end of connection (memory leak), no changes to PGconn2. the initial patch I posted
engines are initialized (so can work), engines aren't freed (memory leak), no
changes to PGconn only in engine code path3. the current patch
engines are initialized, engines are freed, but problematic changes to PGconnMaybe version 2 (my initial patch) could be an alternative ?
Well, based on the "we don't know which different versions of openssl
it'll break with", version 2 is no better than version 3 :(
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
Lars Kanis wrote:
Maybe version 2 (my initial patch) could be an alternative ?
Well, based on the "we don't know which different versions of openssl
it'll break with", version 2 is no better than version 3 :(
Yeah, if we do anything I think it should be more like #3.
I think all or most back releases of openssl are available from
openssl.org. If someone had time to do a test compile of the proposed
patch against all of 'em (or at least all the ones we still claim to
support), it would salve my worries at least a bit. I'm not sure if
that's a big task or not, though.
regards, tom lane
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
Lars Kanis wrote:
Maybe version 2 (my initial patch) could be an alternative ?
Well, based on the "we don't know which different versions of openssl
it'll break with", version 2 is no better than version 3 :(Yeah, if we do anything I think it should be more like #3.
I think all or most back releases of openssl are available from
openssl.org. If someone had time to do a test compile of the proposed
patch against all of 'em (or at least all the ones we still claim to
support), it would salve my worries at least a bit. I'm not sure if
that's a big task or not, though.
Well, if it's just to compile it, I guess it should even be scriptable :-)
I'm not going to volunteer for that myself since I'm not sure I'll have
the time. I can try to, but it's better if someone else has the time.
--
Magnus Hagander
Self: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
Lars Kanis wrote:
Maybe version 2 (my initial patch) could be an alternative ?
Well, based on the "we don't know which different versions of openssl
it'll break with", version 2 is no better than version 3 :(Yeah, if we do anything I think it should be more like #3.
I think all or most back releases of openssl are available from
openssl.org. If someone had time to do a test compile of the proposed
patch against all of 'em (or at least all the ones we still claim to
support), it would salve my worries at least a bit. I'm not sure if
that's a big task or not, though.
I ran a build with OpenSSL 0.9.8 (default on my machine) and 0.9.7.
AFAICS, only 0.9.8 is supported at all these days. There is no point in
running against <0.9.7, since there was no engine support in that.
I tried 0.9.7m, 0.9.7c, 0.9.8g (Ubuntu) and 0.9.8b just to be sure. But
they do seem to have a policy similar to our own - no features in
backbranches.
I only tested building (make clean, re-configure, make in pg) and it
worked fine. Default configuration on OpenSSL.
This is certainly not an exhaustive test, but I think it does a fair job
of bracketing the possible versions with issues.
//Magnus