From 7506677b62c823d12d27a7c8ff93c69155a2f107 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 20 Nov 2004 00:18:18 +0000 Subject: [PATCH] Improve error reporting for SSL connection failures. Remove redundant free operations in client_cert_cb --- openssl will also attempt to free these structures, resulting in core dumps. --- src/backend/libpq/be-secure.c | 85 ++++++++++++++++++-------------- src/interfaces/libpq/fe-secure.c | 34 ++++++++----- 2 files changed, 69 insertions(+), 50 deletions(-) diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index 44218c1aa49..f0d1a52e95c 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/libpq/be-secure.c,v 1.53 2004/11/17 04:05:42 neilc Exp $ + * $PostgreSQL: pgsql/src/backend/libpq/be-secure.c,v 1.54 2004/11/20 00:18:13 tgl Exp $ * * Since the server static private key ($DataDir/server.key) * will normally be stored unencrypted so that the database @@ -727,37 +727,6 @@ initialize_SSL(void) return 0; } -#ifdef WIN32 -/* - * Win32 socket code uses nonblocking sockets. We ned to deal with that - * by waiting on the socket if the SSL accept operation didn't complete - * right away. - */ -static int pgwin32_SSL_accept(SSL *ssl) -{ - int r; - - while (1) - { - int rc; - int waitfor; - - r = SSL_accept(ssl); - if (r == 1) - return 1; - - rc = SSL_get_error(ssl, r); - if (rc != SSL_ERROR_WANT_READ && rc != SSL_ERROR_WANT_WRITE) - return r; - - waitfor = (rc == SSL_ERROR_WANT_READ)?FD_READ|FD_CLOSE|FD_ACCEPT:FD_WRITE|FD_CLOSE; - if (pgwin32_waitforsinglesocket(SSL_get_fd(ssl), waitfor) == 0) - return -1; - } -} -#define SSL_accept(ssl) pgwin32_SSL_accept(ssl) -#endif - /* * Destroy global SSL context. */ @@ -777,7 +746,9 @@ destroy_SSL(void) static int open_server_SSL(Port *port) { - int r; + int r; + int err; + Assert(!port->ssl); Assert(!port->peer); @@ -799,12 +770,50 @@ open_server_SSL(Port *port) close_SSL(port); return -1; } - if ((r=SSL_accept(port->ssl)) <= 0) + +aloop: + r = SSL_accept(port->ssl); + if (r <= 0) { - ereport(COMMERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("could not accept SSL connection: %i", - SSL_get_error(port->ssl,r)))); + err = SSL_get_error(port->ssl, r); + switch (err) + { + case SSL_ERROR_WANT_READ: + case SSL_ERROR_WANT_WRITE: +#ifdef WIN32 + pgwin32_waitforsinglesocket(SSL_get_fd(port->ssl), + (err==SSL_ERROR_WANT_READ) ? + FD_READ|FD_CLOSE|FD_ACCEPT : FD_WRITE|FD_CLOSE); +#endif + goto aloop; + case SSL_ERROR_SYSCALL: + if (r < 0) + ereport(COMMERROR, + (errcode_for_socket_access(), + errmsg("could not accept SSL connection: %m"))); + else + ereport(COMMERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("could not accept SSL connection: EOF detected"))); + break; + case SSL_ERROR_SSL: + ereport(COMMERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("could not accept SSL connection: %s", + SSLerrmessage()))); + break; + case SSL_ERROR_ZERO_RETURN: + ereport(COMMERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("could not accept SSL connection: EOF detected"))); + break; + default: + ereport(COMMERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("unrecognized SSL error code: %d", + err))); + break; + } close_SSL(port); return -1; } diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 637d93e43dd..0e09a9941e0 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.55 2004/10/16 03:26:43 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.56 2004/11/20 00:18:18 tgl Exp $ * * NOTES * [ Most of these notes are wrong/obsolete, but perhaps not all ] @@ -267,6 +267,11 @@ pqsecure_open_client(PGconn *conn) close_SSL(conn); return PGRES_POLLING_FAILED; } + /* + * Initialize errorMessage to empty. This allows open_client_SSL() + * to detect whether client_cert_cb() has stored a message. + */ + resetPQExpBuffer(&conn->errorMessage); } /* Begin or continue the actual handshake */ return open_client_SSL(conn); @@ -797,7 +802,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) printfPQExpBuffer(&conn->errorMessage, libpq_gettext("certificate present, but not private key file \"%s\"\n"), fnbuf); - X509_free(*x509); return 0; } if (!S_ISREG(buf.st_mode) || (buf.st_mode & 0077) || @@ -806,7 +810,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) printfPQExpBuffer(&conn->errorMessage, libpq_gettext("private key file \"%s\" has wrong permissions\n"), fnbuf); - X509_free(*x509); return 0; } if ((fp = fopen(fnbuf, "r")) == NULL) @@ -814,7 +817,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) printfPQExpBuffer(&conn->errorMessage, libpq_gettext("could not open private key file \"%s\": %s\n"), fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf))); - X509_free(*x509); return 0; } if (fstat(fileno(fp), &buf2) == -1 || @@ -822,7 +824,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) { printfPQExpBuffer(&conn->errorMessage, libpq_gettext("private key file \"%s\" changed during execution\n"), fnbuf); - X509_free(*x509); return 0; } if (PEM_read_PrivateKey(fp, pkey, cb, NULL) == NULL) @@ -833,7 +834,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) libpq_gettext("could not read private key file \"%s\": %s\n"), fnbuf, err); SSLerrfree(err); - X509_free(*x509); fclose(fp); return 0; } @@ -848,8 +848,6 @@ client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey) libpq_gettext("certificate does not match private key file \"%s\": %s\n"), fnbuf, err); SSLerrfree(err); - X509_free(*x509); - EVP_PKEY_free(*pkey); return 0; } @@ -1045,11 +1043,23 @@ open_client_SSL(PGconn *conn) } case SSL_ERROR_SSL: { - char *err = SSLerrmessage(); + /* + * If there are problems with the local certificate files, + * these will be detected by client_cert_cb() which is + * called from SSL_connect(). We want to return that + * error message and not the rather unhelpful error that + * OpenSSL itself returns. So check to see if an error + * message was already stored. + */ + if (conn->errorMessage.len == 0) + { + char *err = SSLerrmessage(); - printfPQExpBuffer(&conn->errorMessage, - libpq_gettext("SSL error: %s\n"), err); - SSLerrfree(err); + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("SSL error: %s\n"), + err); + SSLerrfree(err); + } close_SSL(conn); return PGRES_POLLING_FAILED; }