Re: Improve errors when setting incorrect bounds for SSL protocols

--juZjCTNxrMaZdGZC
Content-Type: multipart/mixed; boundary="/2994txjAzEdQwm5"
Content-Disposition: inline


--/2994txjAzEdQwm5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote:
> On 14 Jan 2020, at 04:54, Michael Paquier <[email protected]> wrote:
>> Please note that OpenSSL 1.1.0 has added two routines to be able to
>> get the min/max protocols set in a context, called
>> SSL_CTX_get_min/max_proto_version.  Thinking about older versions of
>> OpenSSL I think that it is better to use
>> ssl_protocol_version_to_openssl to do the parsing work.  I also found
>> that it is easier to check for compatible versions after setting both
>> bounds in the SSL context, so as there is no need to worry about
>> invalid values depending on the build of OpenSSL used.
>=20
> I'm not convinced that it's a good idea to check for incompatible protocol
> range in the OpenSSL backend.  We've spent a lot of energy to make the TL=
S code
> library agnostic and pluggable, and since identifying a basic configurati=
on
> error isn't OpenSSL specific I think it should be in the guc code.  That =
would
> keep the layering as well as ensure that we don't mistakenly treat this
> differently should we get a second TLS backend.

Good points.  And the get routines are not that portable in OpenSSL
either even if HEAD supports 1.0.1 and newer versions...  Attached is
an updated patch which uses a GUC check for both parameters, and
provides a hint on top of the original error message.  The SSL context
does not get reloaded if there is an error, so the errors from OpenSSL
cannot be triggered as far as I checked (after mixing a couple of
corrent and incorrect combinations manually).
--
Michael

--/2994txjAzEdQwm5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="ssl-proto-context-v2.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e5f8a1301f..d97fe3beb2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -204,6 +204,10 @@ static const char *show_log_file_mode(void);
 static const char *show_data_directory_mode(void);
 static bool check_backtrace_functions(char **newval, void **extra, GucSour=
ce source);
 static void assign_backtrace_functions(const char *newval, void *extra);
+static bool check_ssl_min_protocol_version(int *newval, void **extra,
+										   GucSource source);
+static bool check_ssl_max_protocol_version(int *newval, void **extra,
+										   GucSource source);
 static bool check_recovery_target_timeline(char **newval, void **extra, Gu=
cSource source);
 static void assign_recovery_target_timeline(const char *newval, void *extr=
a);
 static bool check_recovery_target(char **newval, void **extra, GucSource s=
ource);
@@ -4594,7 +4598,7 @@ static struct config_enum ConfigureNamesEnum[] =3D
 		&ssl_min_protocol_version,
 		PG_TLS1_2_VERSION,
 		ssl_protocol_versions_info + 1, /* don't allow PG_TLS_ANY */
-		NULL, NULL, NULL
+		check_ssl_min_protocol_version, NULL, NULL
 	},
=20
 	{
@@ -4606,7 +4610,7 @@ static struct config_enum ConfigureNamesEnum[] =3D
 		&ssl_max_protocol_version,
 		PG_TLS_ANY,
 		ssl_protocol_versions_info,
-		NULL, NULL, NULL
+		check_ssl_max_protocol_version, NULL, NULL
 	},
=20
 	/* End-of-list marker */
@@ -11603,6 +11607,49 @@ assign_backtrace_functions(const char *newval, voi=
d *extra)
 	backtrace_symbol_list =3D (char *) extra;
 }
=20
+static bool
+check_ssl_min_protocol_version(int *newval, void **extra, GucSource source)
+{
+	int  new_ssl_min_protocol_version =3D *newval;
+
+	/* PG_TLS_ANY is not supported for the minimum bound */
+	Assert(new_ssl_min_protocol_version > PG_TLS_ANY);
+
+	if (ssl_max_protocol_version &&
+		new_ssl_min_protocol_version > ssl_max_protocol_version)
+	{
+		GUC_check_errhint("\"%s\" cannot be higher than \"%s\".",
+						 "ssl_min_protocol_version",
+						 "ssl_max_protocol_version");
+		GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
+		return false;
+	}
+
+	return true;
+}
+
+static bool
+check_ssl_max_protocol_version(int *newval, void **extra, GucSource source)
+{
+	int  new_ssl_max_protocol_version =3D *newval;
+
+	/* if PG_TLS_ANY, there is no need to check the bounds */
+	if (new_ssl_max_protocol_version =3D=3D PG_TLS_ANY)
+		return true;
+
+	if (ssl_min_protocol_version &&
+		ssl_min_protocol_version > new_ssl_max_protocol_version)
+	{
+		GUC_check_errhint("\"%s\" cannot be lower than \"%s\".",
+						 "ssl_max_protocol_version",
+						 "ssl_min_protocol_version");
+		GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
+		return false;
+	}
+
+	return true;
+}
+
 static bool
 check_recovery_target_timeline(char **newval, void **extra, GucSource sour=
ce)
 {
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 83fcd5e839..7931ebc067 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
=20
 if ($ENV{with_openssl} eq 'yes')
 {
-	plan tests =3D> 84;
+	plan tests =3D> 86;
 }
 else
 {
@@ -97,6 +97,22 @@ command_ok(
 	'restart succeeds with password-protected key file');
 $node->_update_pid(1);
=20
+# Test compatibility of SSL protocols.
+# TLSv1.1 is lower than TLSv1.2, so it won't work.
+$node->append_conf('postgresql.conf',
+		   qq{ssl_min_protocol_version=3D'TLSv1.2'
+ssl_max_protocol_version=3D'TLSv1.1'});
+command_fails(
+	[ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+	'restart fails with incorrect SSL protocol bounds');
+# Go back to the defaults, this works.
+$node->append_conf('postgresql.conf',
+		   qq{ssl_min_protocol_version=3D'TLSv1.2'
+ssl_max_protocol_version=3D''});
+command_ok(
+	[ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+	'restart succeeds with correct SSL protocol bounds');
+
 ### Run client-side tests.
 ###
 ### Test that libpq accepts/rejects the connection correctly, depending

--/2994txjAzEdQwm5--

--juZjCTNxrMaZdGZC
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4eeLUACgkQnvQgOdby
QH1YTg/+K0n2dd7TDlqXwZzN/ShykZsbi7uVfoSuhZrEK/HCyoYEnapOZf91pvvz
pWD+pPqcMXnsLv923MD20McGPpWvzJYIGqpZ3Y4IWLtJakgaJBu3KdslH3RyUQn3
v6RRXRhY8hT+74zFtP+MDuj4bfe2s52t9YAp0Gq8t/lUx9+/+sHzsANjrhQHbUWj
ATBewkdWc9I+UBRmdUObkGRthmcA6WKz6X8deA03MrEydXDkHRjO/IpLMAfEG6f8
DycTXzMFWpRoikvnH56OHLAQ8A4+PH5H6OU8mvmzgZRqY8bncFpPAppn/69mMUAs
fIxDE6fNtQ2hJb+Aw23V0pTI2Yqd0qbCGu2Y3yjYEFIr35gRBFYkxjGpq1BdTRR6
hVB8fZYUUARvZ9PsfeGI5NvFEUxSnN3+or8+/i6tiYFY8yLpzjDMQueF+9j3keQP
B0M57jbFpxz6pzGK0elkxkUvgMjQjSxANctUbZc8N8ethrpwleCn7m3YbnDOSV5Z
U8DM560TAFpTft0IPsjM/8NRSKo0XOG9SOA0MGdkDdMm63lWD8hy28+By+xQihBF
ru8drDsRemhnxrP/5Le7qI5TXsVmWfOFHvt5aFIYwHoVezRTb6d9B9XCwP78K1hV
IV9u7IxUZuPfzJDDaY+fmOHDvQS6w/CofhpVjaEt/hlR11ULxYE=
=w0yI
-----END PGP SIGNATURE-----

--juZjCTNxrMaZdGZC--