Inconsistent error handling in the openssl init code
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.
cheers ./daniel
Attachments:
openssl_tlsver.patchapplication/octet-stream; name=openssl_tlsver.patch; x-unix-mode=0644Download
From 5160a67f2603b37da3cc912d6dae78a4daef15f3 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 6 Feb 2019 15:14:00 +0100
Subject: [PATCH] Set elevel based on isServerStart in TLS version check
be_tls_init() should error out with an elevel based on isServerStart
in order for the SSL context to be properly cleaned up. Fix the TLS
version setup to also consider isServerStart rather than erroring out
with ERROR unconditionally.
---
src/backend/libpq/be-secure-openssl.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index a2779543ec..85507fa2f2 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -67,7 +67,8 @@ static bool SSL_initialized = false;
static bool dummy_ssl_passwd_cb_called = false;
static bool ssl_is_server_start;
-static int ssl_protocol_version_to_openssl(int v, const char *guc_name);
+static int ssl_protocol_version_to_openssl(int v, const char *guc_name,
+ bool isServerStart);
#ifndef SSL_CTX_set_min_proto_version
static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
@@ -190,13 +191,24 @@ be_tls_init(bool isServerStart)
}
if (ssl_min_protocol_version)
- SSL_CTX_set_min_proto_version(context,
- ssl_protocol_version_to_openssl(ssl_min_protocol_version,
- "ssl_min_protocol_version"));
+ {
+ int tls_ver = ssl_protocol_version_to_openssl(ssl_min_protocol_version,
+ "ssl_min_protocol_version",
+ isServerStart);
+ if (tls_ver == -1)
+ goto error;
+ SSL_CTX_set_min_proto_version(context, tls_ver);
+ }
+
if (ssl_max_protocol_version)
- SSL_CTX_set_max_proto_version(context,
- ssl_protocol_version_to_openssl(ssl_max_protocol_version,
- "ssl_max_protocol_version"));
+ {
+ int tls_ver = ssl_protocol_version_to_openssl(ssl_max_protocol_version,
+ "ssl_max_protocol_version",
+ isServerStart);
+ if (tls_ver == -1)
+ goto error;
+ SSL_CTX_set_max_proto_version(context, tls_ver);
+ }
/* disallow SSL session tickets */
#ifdef SSL_OP_NO_TICKET /* added in OpenSSL 0.9.8f */
@@ -1262,8 +1274,10 @@ X509_NAME_to_cstring(X509_NAME *name)
* working with a supported version.
*/
static int
-ssl_protocol_version_to_openssl(int v, const char *guc_name)
+ssl_protocol_version_to_openssl(int v, const char *guc_name, bool isServerStart)
{
+ int loglevel = isServerStart ? FATAL : LOG;
+
switch (v)
{
case PG_TLS_ANY:
@@ -1292,7 +1306,7 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name)
error:
pg_attribute_unused();
- ereport(ERROR,
+ ereport(loglevel,
(errmsg("%s setting %s not supported by this build",
guc_name,
GetConfigOption(guc_name, false, false))));
--
2.14.1.145.gb3622a4ee
On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.
(Adding Peter Eisentraut in CC)
Good catch, this is an oversight from commit e73e67c7, which affects
only HEAD. The comment at the top of ssl_protocol_version_to_openssl
becomes incorrect as the function would not throw an error in a reload
context.
The current comment is that:
* If a version is passed that is not supported by the current OpenSSL
* version, then we throw an error, so that subsequent code can assume it's
* working with a supported version.
Which we could change to that:
..., then we throw an error as FATAL if isServerStart is true so as it
won't return. Otherwise, errors are logged as LOG level and return -1
to indicate trouble, preserving the old SSL state if any.
Peter, could you take care of it? Or should I?
--
Michael
On 7 Feb 2019, at 05:12, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.(Adding Peter Eisentraut in CC)
Good catch, this is an oversight from commit e73e67c7, which affects
only HEAD. The comment at the top of ssl_protocol_version_to_openssl
becomes incorrect as the function would not throw an error in a reload
context.
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!
cheers ./daniel
Attachments:
openssl_tlsver-v2.patchapplication/octet-stream; name=openssl_tlsver-v2.patch; x-unix-mode=0644Download
From 40a5206109472b79ad0a9f02e4f88de23a5ec009 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 6 Feb 2019 15:14:00 +0100
Subject: [PATCH] Set elevel based on isServerStart in TLS version check
be_tls_init() should error out with an elevel based on isServerStart
in order for the SSL context to be properly cleaned up. Fix the TLS
version setup to also consider isServerStart rather than erroring out
with ERROR unconditionally.
---
src/backend/libpq/be-secure-openssl.c | 37 ++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index a2779543ec..9635abdec7 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -67,7 +67,8 @@ static bool SSL_initialized = false;
static bool dummy_ssl_passwd_cb_called = false;
static bool ssl_is_server_start;
-static int ssl_protocol_version_to_openssl(int v, const char *guc_name);
+static int ssl_protocol_version_to_openssl(int v, const char *guc_name,
+ bool isServerStart);
#ifndef SSL_CTX_set_min_proto_version
static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
@@ -190,13 +191,24 @@ be_tls_init(bool isServerStart)
}
if (ssl_min_protocol_version)
- SSL_CTX_set_min_proto_version(context,
- ssl_protocol_version_to_openssl(ssl_min_protocol_version,
- "ssl_min_protocol_version"));
+ {
+ int tls_ver = ssl_protocol_version_to_openssl(ssl_min_protocol_version,
+ "ssl_min_protocol_version",
+ isServerStart);
+ if (tls_ver == -1)
+ goto error;
+ SSL_CTX_set_min_proto_version(context, tls_ver);
+ }
+
if (ssl_max_protocol_version)
- SSL_CTX_set_max_proto_version(context,
- ssl_protocol_version_to_openssl(ssl_max_protocol_version,
- "ssl_max_protocol_version"));
+ {
+ int tls_ver = ssl_protocol_version_to_openssl(ssl_max_protocol_version,
+ "ssl_max_protocol_version",
+ isServerStart);
+ if (tls_ver == -1)
+ goto error;
+ SSL_CTX_set_max_proto_version(context, tls_ver);
+ }
/* disallow SSL session tickets */
#ifdef SSL_OP_NO_TICKET /* added in OpenSSL 0.9.8f */
@@ -1258,12 +1270,15 @@ X509_NAME_to_cstring(X509_NAME *name)
* guc.c independent of OpenSSL availability and version.
*
* If a version is passed that is not supported by the current OpenSSL
- * version, then we throw an error, so that subsequent code can assume it's
- * working with a supported version.
+ * version, then we log with the appropriate elevel given the isServerStart
+ * parameter, and will error out. Subsequent code can assume it's working
+ * with a supported version.
*/
static int
-ssl_protocol_version_to_openssl(int v, const char *guc_name)
+ssl_protocol_version_to_openssl(int v, const char *guc_name, bool isServerStart)
{
+ int loglevel = isServerStart ? FATAL : LOG;
+
switch (v)
{
case PG_TLS_ANY:
@@ -1292,7 +1307,7 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name)
error:
pg_attribute_unused();
- ereport(ERROR,
+ ereport(loglevel,
(errmsg("%s setting %s not supported by this build",
guc_name,
GetConfigOption(guc_name, false, false))));
--
2.14.1.145.gb3622a4ee
On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!
That looks fine to me. Could you just add it to the next CF as a bug
fix so as we don't forget? I am pretty sure that Peter will look at
that soon.
--
Michael
On 8 Feb 2019, at 01:10, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!That looks fine to me. Could you just add it to the next CF as a bug
fix so as we don't forget? I am pretty sure that Peter will look at
that soon.
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.
cheers ./daniel
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.
Thanks. Please note that I can take care of the patch in a couple of
days if need be.
--
Michael
On 08/02/2019 11:01, Michael Paquier wrote:
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.Thanks. Please note that I can take care of the patch in a couple of
days if need be.
Fixed, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8 Feb 2019, at 12:01, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 08/02/2019 11:01, Michael Paquier wrote:
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.Thanks. Please note that I can take care of the patch in a couple of
days if need be.Fixed, thanks.
Thanks, I’ve closed it in the commitfest with you as committer.
cheers ./daniel