pgsql: Use BIO functions to avoid passing FILE * pointers to OpenSSL
Log Message:
-----------
Use BIO functions to avoid passing FILE * pointers to OpenSSL functions.
This fixes potential crashes on old versions of OpenSSL and the requirement on
"Applink" in new versions when building with MSVC and using different
runtimes.
Dave Page with fixes from me.
Modified Files:
--------------
pgsql/src/interfaces/libpq:
fe-secure.c (r1.94 -> r1.95)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/fe-secure.c?r1=1.94&r2=1.95)
"Magnus Hagander" <mha@postgresql.org> writes:
Log Message:
-----------
Use BIO functions to avoid passing FILE * pointers to OpenSSL functions.
This fixes potential crashes on old versions of OpenSSL and the requirement on
"Applink" in new versions when building with MSVC and using different
runtimes.
Several buildfarm machines are failing:
ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/et -c -o initdb.o initdb.c
ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g initdb.o -L../../../src/port -lpgport -L../../../src/interfaces/libpq -lpq -L../../../src/port -Wl,-rpath,'/home/gevik/pgfarmbuild/build/HEAD/inst/lib' -lpgport -lssl -lcrypto -lkrb5 -lz -lreadline -ltermcap -lcrypt -ldl -lm -o initdb
../../../src/interfaces/libpq/libpq.so: undefined reference to `ERR_set_mark'
../../../src/interfaces/libpq/libpq.so: undefined reference to `ERR_pop_to_mark'
collect2: ld returned 1 exit status
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
"Magnus Hagander" <mha@postgresql.org> writes:
Use BIO functions to avoid passing FILE * pointers to OpenSSL functions.
Several buildfarm machines are failing:
http://www.openssl.org/docs/crypto/ERR_set_mark.html
says
ERR_set_mark() and ERR_pop_to_mark() were added in OpenSSL 0.9.8.
Ooops. Back to the drawing board.
regards, tom lane
I wrote:
http://www.openssl.org/docs/crypto/ERR_set_mark.html
says
ERR_set_mark() and ERR_pop_to_mark() were added in OpenSSL 0.9.8.
Ooops. Back to the drawing board.
To get the buildfarm going again, I applied a patch that turns these
calls into no-ops if the local OpenSSL hasn't got the functions.
I'm not entirely sure if the net result is a regression for pre-0.9.8
OpenSSLs or not --- Magnus, any thoughts on that?
regards, tom lane
http://www.openssl.org/docs/crypto/ERR_set_mark.html
says
ERR_set_mark() and ERR_pop_to_mark() were added in OpenSSL 0.9.8.Ooops. Back to the drawing board.
To get the buildfarm going again, I applied a patch that turns these
calls into no-ops if the local OpenSSL hasn't got the functions.
I'm not entirely sure if the net result is a regression for pre-0.9.8
OpenSSLs or not --- Magnus, any thoughts on that?
I think it is. With Dave's part of the patch and not mine, you get the incorrect error message. It requires that you set sslmode to required which I did't
originally note, but if you do you'll get the wrong error.
Not sure what's the least evil fix.
We could ifdef the whole fix and use the old code for earlier openssl but bio for 0.9.8. Or we could implement my other idea to load the certificate earlier.
Or we could just say live with the error message on older openssl. Or someone has another idea?
/Magnus
http://www.openssl.org/docs/crypto/ERR_set_mark.html
says
ERR_set_mark() and ERR_pop_to_mark() were added in OpenSSL 0.9.8.Ooops. Back to the drawing board.
To get the buildfarm going again, I applied a patch that turns these
calls into no-ops if the local OpenSSL hasn't got the functions.
I'm not entirely sure if the net result is a regression for pre-0.9.8
OpenSSLs or not --- Magnus, any thoughts on that?
I thought of a compromise. We can put back a check if the file exists without using bio. That would cover some 99 percent of the messages coming out of that
routine, I bet. And things would still work correct in 0.9.8.
/Magnus
On Tue, Oct 02, 2007 at 08:42:28AM +0200, Magnus Hagander wrote:
http://www.openssl.org/docs/crypto/ERR_set_mark.html
says
ERR_set_mark() and ERR_pop_to_mark() were added in OpenSSL 0.9.8.Ooops. Back to the drawing board.
To get the buildfarm going again, I applied a patch that turns these
calls into no-ops if the local OpenSSL hasn't got the functions.
I'm not entirely sure if the net result is a regression for pre-0.9.8
OpenSSLs or not --- Magnus, any thoughts on that?I thought of a compromise. We can put back a check if the file exists without using bio. That would cover some 99 percent of the messages coming out of that
routine, I bet. And things would still work correct in 0.9.8.
Here's an example of what I meant.
I think this can be reasonable - OpenSSL 0.9.8 is from 2005 after all, so
it's not like we're requiring something extremely new..
//Magnus
Attachments:
openssl_extracheck.patchtext/plain; charset=us-asciiDownload
Index: fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.96
diff -c -r1.96 fe-secure.c
*** fe-secure.c 2 Oct 2007 00:25:20 -0000 1.96
--- fe-secure.c 2 Oct 2007 09:04:16 -0000
***************
*** 607,612 ****
--- 607,626 ----
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
+
+ /*
+ * OpenSSL <= 0.8.2 lacks error stack handling. Do a separate check
+ * for the existance of the file without using BIO functions to make
+ * it pick up the majority of the cases with the old versions.
+ */
+ if (stat(fnbuf, &buf) == -1)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not open certificate file \"%s\": %s\n"),
+ fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
+ return 0;
+ }
+
if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
Magnus Hagander <magnus@hagander.net> writes:
I thought of a compromise. We can put back a check if the file exists without using bio. That would cover some 99 percent of the messages coming out of that
routine, I bet. And things would still work correct in 0.9.8.
Here's an example of what I meant.
+1, but I think you should "#ifndef HAVE_ERR_SET_MARK" the added code.
Also, maybe try to fopen rather than just fstat?
regards, tom lane
On Tue, Oct 02, 2007 at 10:04:19AM -0400, Tom Lane wrote:
Magnus Hagander <magnus@hagander.net> writes:
I thought of a compromise. We can put back a check if the file exists without using bio. That would cover some 99 percent of the messages coming out of that
routine, I bet. And things would still work correct in 0.9.8.Here's an example of what I meant.
+1, but I think you should "#ifndef HAVE_ERR_SET_MARK" the added code.
Also, maybe try to fopen rather than just fstat?
Done on both, and applied.
//Magnus