v10-0002-libpq-force-sslmode-verify-full-for-system-CAs.patch

From c822c579ea2c84c14a54c486328ac9ffed6184da Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchampion@timescale.com>
Date: Mon, 24 Oct 2022 15:24:11 -0700
Subject: [PATCH v10 2/2] libpq: force sslmode=verify-full for system CAs

Weaker verification modes don't make much sense for public CAs.
---
 doc/src/sgml/libpq.sgml           | 15 ++++----
 doc/src/sgml/runtime.sgml         |  6 +--
 src/interfaces/libpq/fe-connect.c | 63 +++++++++++++++++++++++++++++++
 src/interfaces/libpq/t/001_uri.pl | 30 ++++++++++++++-
 src/test/ssl/t/001_ssltests.pl    | 12 ++++++
 5 files changed, 113 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 29ef0ae75d..faa8aa3187 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1882,20 +1882,19 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
         locations of these root certificates differ by SSL implementation and
         platform. For <productname>OpenSSL</productname> in particular, the
         locations may be further modified by the <envar>SSL_CERT_DIR</envar>
         and <envar>SSL_CERT_FILE</envar> environment variables.
        </para>
-       <warning>
+       <note>
         <para>
-         When using <literal>sslrootcert=system</literal>, it is critical to
-         also use the strongest certificate verification method,
-         <literal>sslmode=verify-full</literal>. In most cases it is trivial for
-         anyone to obtain a certificate trusted by the system for a hostname
-         they control, rendering the <literal>verify-ca</literal> mode useless.
+         When using <literal>sslrootcert=system</literal>, the default
+         <literal>sslmode</literal> is changed to <literal>verify-full</literal>,
+         and any weaker setting will result in an error. In most cases it is
+         trivial for anyone to obtain a certificate trusted by the system for a
+         hostname they control, rendering <literal>verify-ca</literal> and all
+         weaker modes useless.
         </para>
-       </warning>
-       <note>
         <para>
          The magic <literal>system</literal> value will take precedence over a
          local certificate file with the same name. If for some reason you find
          yourself in this situation, use an alternative path like
          <literal>sslrootcert=./system</literal> instead.
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index b93184537a..dbe23db54f 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2007,13 +2007,13 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
    (<xref linkend="ssl-tcp"/>). The TCP client must connect using
    <literal>sslmode=verify-ca</literal> or
    <literal>verify-full</literal> and have the appropriate root certificate
    file installed (<xref linkend="libq-ssl-certificates"/>). Alternatively the
    system CA pool can be used using <literal>sslrootcert=system</literal>; in
-   this case, <literal>sslmode=verify-full</literal> must be used for safety,
-   since it is generally trivial to obtain certificates which are signed by a
-   public CA.
+   this case, <literal>sslmode=verify-full</literal> is forced for safety, since
+   it is generally trivial to obtain certificates which are signed by a public
+   CA.
   </para>
 
   <para>
     To prevent spoofing with GSSAPI, the server must be configured to accept
     only <literal>hostgssenc</literal> connections
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index bb7347cb0c..16a5105d8b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1463,10 +1463,26 @@ connectOptions2(PGconn *conn)
 		conn->channel_binding = strdup(DefaultChannelBinding);
 		if (!conn->channel_binding)
 			goto oom_error;
 	}
 
+#ifndef USE_SSL
+	/*
+	 * sslrootcert=system is not supported. Since setting this changes the
+	 * default sslmode, check this _before_ we validate sslmode, to avoid
+	 * confusing the user with errors for an option they may not have set.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0)
+	{
+		conn->status = CONNECTION_BAD;
+		libpq_append_conn_error(conn, "sslrootcert value \"%s\" invalid when SSL support is not compiled in",
+								conn->sslrootcert);
+		return false;
+	}
+#endif
+
 	/*
 	 * validate sslmode option
 	 */
 	if (conn->sslmode)
 	{
@@ -1509,10 +1525,25 @@ connectOptions2(PGconn *conn)
 		conn->sslmode = strdup(DefaultSSLMode);
 		if (!conn->sslmode)
 			goto oom_error;
 	}
 
+#ifdef USE_SSL
+	/*
+	 * If sslrootcert=system, make sure our chosen sslmode is compatible.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0
+		&& strcmp(conn->sslmode, "verify-full") != 0)
+	{
+		conn->status = CONNECTION_BAD;
+		libpq_append_conn_error(conn, "weak sslmode \"%s\" may not be used with sslrootcert=system (use verify-full)",
+								conn->sslmode);
+		return false;
+	}
+#endif
+
 	/*
 	 * Validate TLS protocol versions for ssl_min_protocol_version and
 	 * ssl_max_protocol_version.
 	 */
 	if (!sslVerifyProtocolVersion(conn->ssl_min_protocol_version))
@@ -6234,10 +6265,11 @@ conninfo_array_parse(const char *const *keywords, const char *const *values,
  */
 static bool
 conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 {
 	PQconninfoOption *option;
+	PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL;
 	char	   *tmp;
 
 	/*
 	 * If there's a service spec, use it to obtain any not-explicitly-given
 	 * parameters.  Ignore error if no error message buffer is passed because
@@ -6250,10 +6282,13 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 	 * Get the fallback resources for parameters not specified in the conninfo
 	 * string nor the service.
 	 */
 	for (option = options; option->keyword != NULL; option++)
 	{
+		if (strcmp(option->keyword, "sslrootcert") == 0)
+			sslrootcert = option; /* save for later */
+
 		if (option->val != NULL)
 			continue;			/* Value was in conninfo or service */
 
 		/*
 		 * Try to get the environment variable fallback
@@ -6292,10 +6327,17 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 						libpq_append_error(errorMessage, "out of memory");
 					return false;
 				}
 				continue;
 			}
+
+			/*
+			 * sslmode is not specified. Let it be filled in with the compiled
+			 * default for now, but if sslrootcert=system, we'll override the
+			 * default later before returning.
+			 */
+			sslmode_default = option;
 		}
 
 		/*
 		 * No environment variable specified or the variable isn't set - try
 		 * compiled-in default
@@ -6324,10 +6366,31 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 			option->val = pg_fe_getauthname(NULL);
 			continue;
 		}
 	}
 
+	/*
+	 * Special handling for sslrootcert=system with no sslmode explicitly
+	 * defined. In this case we want to strengthen the default sslmode to
+	 * verify-full.
+	 */
+	if (sslmode_default && sslrootcert)
+	{
+		if (sslrootcert->val && strcmp(sslrootcert->val, "system") == 0)
+		{
+			free(sslmode_default->val);
+
+			sslmode_default->val = strdup("verify-full");
+			if (!sslmode_default->val)
+			{
+				if (errorMessage)
+					libpq_append_error(errorMessage, "out of memory");
+				return false;
+			}
+		}
+	}
+
 	return true;
 }
 
 /*
  * Subroutine for parse_connection_string
diff --git a/src/interfaces/libpq/t/001_uri.pl b/src/interfaces/libpq/t/001_uri.pl
index 2ab537f97f..cd659bc1b0 100644
--- a/src/interfaces/libpq/t/001_uri.pl
+++ b/src/interfaces/libpq/t/001_uri.pl
@@ -6,11 +6,13 @@ use PostgreSQL::Test::Utils;
 use Test::More;
 use IPC::Run;
 
 
 # List of URIs tests. For each test the first element is the input string, the
-# second the expected stdout and the third the expected stderr.
+# second the expected stdout and the third the expected stderr. Optionally,
+# additional arguments may specify key/value pairs which will override
+# environment variables for the duration of the test.
 my @tests = (
 	[
 		q{postgresql://uri-user:secret@host:12345/db},
 		q{user='uri-user' password='secret' dbname='db' host='host' port='12345' (inet)},
 		q{},
@@ -207,24 +209,48 @@ my @tests = (
 	],
 	[
 		q{postgres://%2Fvar%2Flib%2Fpostgresql/dbname},
 		q{dbname='dbname' host='/var/lib/postgresql' (local)},
 		q{},
+	],
+	# Usually the default sslmode is 'prefer' (for libraries with SSL) or
+	# 'disable' (for those without). This default changes to 'verify-full' if
+	# the system CA store is in use.
+	[
+		q{postgresql://host?sslmode=disable},
+		q{host='host' sslmode='disable' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
+	],
+	[
+		q{postgresql://host?sslmode=prefer},
+		q{host='host' sslmode='prefer' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
+	],
+	[
+		q{postgresql://host?sslmode=verify-full},
+		q{host='host' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
 	]);
 
 # test to run for each of the above test definitions
 sub test_uri
 {
 	local $Test::Builder::Level = $Test::Builder::Level + 1;
+	local %ENV = %ENV;
 
 	my $uri;
 	my %expect;
+	my %envvars;
 	my %result;
 
-	($uri, $expect{stdout}, $expect{stderr}) = @$_;
+	($uri, $expect{stdout}, $expect{stderr}, %envvars) = @$_;
 
 	$expect{'exit'} = $expect{stderr} eq '';
+	%ENV = (%ENV, %envvars);
 
 	my $cmd = [ 'libpq_uri_regress', $uri ];
 	$result{exit} = IPC::Run::run $cmd, '>', \$result{stdout}, '2>',
 	  \$result{stderr};
 
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 3781a25e39..c8fbb3d06c 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -479,19 +479,31 @@ $common_connstr =
 $node->connect_fails(
 	"$common_connstr sslmode=verify-full host=common-name.pg-ssltest.test",
 	"sslrootcert=system does not connect with private CA",
 	expected_stderr => qr/SSL error: certificate verify failed/);
 
+# Modes other than verify-full cannot be mixed with sslrootcert=system.
+$node->connect_fails(
+	"$common_connstr sslmode=verify-ca host=common-name.pg-ssltest.test",
+	"sslrootcert=system only accepts sslmode=verify-full",
+	expected_stderr => qr/weak sslmode "verify-ca" may not be used with sslrootcert=system/);
+
 SKIP:
 {
 	skip "SSL_CERT_FILE is not supported with LibreSSL" if $libressl;
 
 	# We can modify the definition of "system" to get it trusted again.
 	local $ENV{SSL_CERT_FILE} = $node->data_dir . "/root_ca.crt";
 	$node->connect_ok(
 		"$common_connstr sslmode=verify-full host=common-name.pg-ssltest.test",
 		"sslrootcert=system connects with overridden SSL_CERT_FILE");
+
+	# verify-full mode should be the default for system CAs.
+	$node->connect_fails(
+		"$common_connstr host=common-name.pg-ssltest.test.bad",
+		"sslrootcert=system defaults to sslmode=verify-full",
+		expected_stderr => qr/server certificate for "common-name.pg-ssltest.test" does not match host name "common-name.pg-ssltest.test.bad"/);
 }
 
 # Test that the CRL works
 switch_server_cert($node, certfile => 'server-revoked');
 
-- 
2.25.1