On Mon, Oct 01, 2007 at 03:16:13PM +0200, Magnus Hagander wrote:
> On Mon, Oct 01, 2007 at 02:37:44PM +0200, Magnus Hagander wrote:
> > On Sat, Sep 29, 2007 at 09:01:16PM +0100, Dave Page wrote:
> > > Tom Lane wrote:
> > > > "Dave Page" <[EMAIL PROTECTED]> writes:
> > > >>> From: Tom Lane <[EMAIL PROTECTED]>
> > > >>> ... It's not entirely clear whether BIO_new_fp() would avoid the
> > > >>> problematic calls, but it doesn't look like it'd be hard to try.
> > > >
> > > >> The last version of the patch I posted uses BIO_new_file() in all
> > > >> cases, and (from memory) BIO_get_fp() in the non-win32 case to get a
> > > >> FILE* to pass to fstat.
> > > >
> > > > Did you manage to get rid of the bogus-error-message problem that
> > > > afflicted the first version of the patch? If so, this way is fine.
> > >
> > > No, thats still an issue.
> >
> > A guess on this - probably the BIO stuff overwrites some internal OpenSSL
> > "errno" value, causing the wrong error to be passed up. Most likely, it's
> > not save to call BIO functions from inside the callback. My bet is that
> > it'll actually break without this patch, if you stick something that's
> > invalid in there. It's just taht we picked up the "does not exist" error
> > without calling BIO functions.
> >
> > A quick peek at the OpenSSL sources seems to confirm this.
> >
> > I think we want to either attempt to load the client certificate before we
> > connect (and before it's requested) and just queue up the error to show it
> > in only if it's requested, or we want to try some magic around
> > ERR_set_mark()/ERR_pop_to_mark() to clear out any BIO errors before we hand
> > control back.
> >
> > I'll see if I can put together a poc patch - need to reproduce the problem
> > first :-)
Yes, that was the problem. Attached patch fixes the problem for me on
Windows and Linux using the error mark functionality. It seems a lot
cleaner than the other option.
Dave - can you test this one? Assuming that works, I'll go ahead and apply
it.
I also think we have the same problem in the current code, just not for
fopen(). We trap fopen() without anything happening inside OpenSSL, but if
we get an error in the OpenSSL functions we call, something similar would
likely happen.
Should we backpatch all the stuff, or just the error push/pop thing?
//Magnus
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.94
diff -c -r1.94 fe-secure.c
*** src/interfaces/libpq/fe-secure.c 16 Feb 2007 17:07:00 -0000 1.94
--- src/interfaces/libpq/fe-secure.c 1 Oct 2007 13:21:43 -0000
***************
*** 111,116 ****
--- 111,117 ----
#ifdef USE_SSL
#include <openssl/ssl.h>
+ #include <openssl/bio.h>
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
#include <openssl/conf.h>
#endif
***************
*** 567,572 ****
--- 568,577 ----
* This callback is only called when the server wants a
* client cert.
*
+ * Since BIO functions can set OpenSSL error codes, we must
+ * reset the OpenSSL error stack on *every* exit from this
+ * function once we've started using BIO.
+ *
* Must return 1 on success, 0 on no data or error.
*/
static int
***************
*** 579,586 ****
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
--- 584,592 ----
struct stat buf2;
#endif
char fnbuf[MAXPGPATH];
! FILE *fp;
! BIO *bio;
! PGconn *conn = (PGconn *) SSL_get_app_data(ssl);
char sebuf[256];
if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
***************
*** 590,605 ****
return 0;
}
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file
\"%s\": %s\n"),
fnbuf, pqStrerror(errno,
sebuf, sizeof(sebuf)));
return 0;
}
! if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 596,616 ----
return 0;
}
+ /* save OpenSSL error stack */
+ ERR_set_mark();
+
/* read the user certificate */
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open certificate file
\"%s\": %s\n"),
fnbuf, pqStrerror(errno,
sebuf, sizeof(sebuf)));
+ ERR_pop_to_mark();
return 0;
}
!
! if (PEM_read_bio_X509(bio, x509, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 607,616 ****
libpq_gettext("could not read certificate file
\"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
--- 618,629 ----
libpq_gettext("could not read certificate file
\"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! BIO_free(bio);
! ERR_pop_to_mark();
return 0;
}
!
! BIO_free(bio);
#if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
if (getenv("PGSSLKEY"))
***************
*** 625,630 ****
--- 638,644 ----
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("invalid value of PGSSLKEY
environment variable\n"));
+ ERR_pop_to_mark();
return 0;
}
***************
*** 640,647 ****
engine_str, err);
SSLerrfree(err);
free(engine_str);
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
--- 654,662 ----
engine_str, err);
SSLerrfree(err);
free(engine_str);
+ ERR_pop_to_mark();
return 0;
! }
*pkey = ENGINE_load_private_key(engine_ptr, engine_colon + 1,
NULL, NULL);
***************
*** 654,661 ****
engine_colon + 1,
engine_str, err);
SSLerrfree(err);
free(engine_str);
return 0;
! }
free(engine_str);
}
else
--- 669,677 ----
engine_colon + 1,
engine_str, err);
SSLerrfree(err);
free(engine_str);
+ ERR_pop_to_mark();
return 0;
! }
free(engine_str);
}
else
***************
*** 668,673 ****
--- 684,690 ----
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("certificate present, but not private key file \"%s\"\n"),
fnbuf);
+ ERR_pop_to_mark();
return 0;
}
#ifndef WIN32
***************
*** 677,702 ****
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("private key file \"%s\" has
wrong permissions\n"),
fnbuf);
return 0;
}
#endif
! if ((fp = fopen(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file
\"%s\": %s\n"),
fnbuf,
pqStrerror(errno, sebuf, sizeof(sebuf)));
return 0;
}
#ifndef WIN32
if (fstat(fileno(fp), &buf2) == -1 ||
buf.st_dev != buf2.st_dev || buf.st_ino != buf2.st_ino)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("private
key file \"%s\" changed during execution\n"), fnbuf);
return 0;
}
#endif
! if (PEM_read_PrivateKey(fp, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
--- 694,725 ----
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("private key file \"%s\" has
wrong permissions\n"),
fnbuf);
+ ERR_pop_to_mark();
return 0;
}
#endif
!
! if ((bio = BIO_new_file(fnbuf, "r")) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not open private key file
\"%s\": %s\n"),
fnbuf,
pqStrerror(errno, sebuf, sizeof(sebuf)));
+ ERR_pop_to_mark();
return 0;
}
#ifndef WIN32
+ BIO_get_fp(bio, &fp);
if (fstat(fileno(fp), &buf2) == -1 ||
buf.st_dev != buf2.st_dev || buf.st_ino != buf2.st_ino)
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("private
key file \"%s\" changed during execution\n"), fnbuf);
+ ERR_pop_to_mark();
return 0;
}
#endif
!
! if (PEM_read_bio_PrivateKey(bio, pkey, NULL, NULL) == NULL)
{
char *err = SSLerrmessage();
***************
*** 704,713 ****
libpq_gettext("could not read private key file
\"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
! fclose(fp);
return 0;
}
! fclose(fp);
}
/* verify that the cert and key go together */
--- 727,739 ----
libpq_gettext("could not read private key file
\"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
!
! BIO_free(bio);
! ERR_pop_to_mark();
return 0;
}
!
! BIO_free(bio);
}
/* verify that the cert and key go together */
***************
*** 719,727 ****
--- 745,756 ----
libpq_gettext("certificate
does not match private key file \"%s\": %s\n"),
fnbuf, err);
SSLerrfree(err);
+ ERR_pop_to_mark();
return 0;
}
+ ERR_pop_to_mark();
+
return 1;
}
---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
