From 87a324efcfe347265439dc24c9780eb07de7781b Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 24 Oct 2022 15:24:11 -0700 Subject: [PATCH v3 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 | 58 +++++++++++++++++++++++++++++++ src/interfaces/libpq/t/001_uri.pl | 30 ++++++++++++++-- src/test/ssl/t/001_ssltests.pl | 12 +++++++ 5 files changed, 109 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 1401b93e72..3ebbd45aa4 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1733,15 +1733,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname locations may be further modified by the SSL_CERT_DIR and SSL_CERT_FILE environment variables. - + - When using sslrootcert=system, it is critical to - also use the strongest certificate verification method, - sslmode=verify-full. In most cases it is trivial for - anyone to obtain a certificate trusted by the system for a hostname - they control, rendering the verify-ca mode useless. + When using sslrootcert=system, the default + sslmode is changed to verify-full, + 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 verify-ca and all + weaker modes useless. - + diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index 8184a3d54e..c4c4874fd0 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -2008,9 +2008,9 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433 verify-full and have the appropriate root certificate file installed (). Alternatively the system CA pool can be used using sslrootcert=system; in - this case, sslmode=verify-full must be used for safety, - since it is generally trivial to obtain certificates which are signed by a - public CA. + this case, sslmode=verify-full is forced for safety, since + it is generally trivial to obtain certificates which are signed by a public + CA. diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 746e9b4f1e..437d530ba1 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1263,6 +1263,23 @@ connectOptions2(PGconn *conn) 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; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("sslrootcert value \"%s\" invalid when SSL support is not compiled in\n"), + conn->sslrootcert); + return false; + } +#endif + /* * validate sslmode option */ @@ -1311,6 +1328,22 @@ connectOptions2(PGconn *conn) 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; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("weak sslmode \"%s\" may not be used with sslrootcert=system\n"), + conn->sslmode); + return false; + } +#endif + /* * Validate TLS protocol versions for ssl_min_protocol_version and * ssl_max_protocol_version. @@ -5876,6 +5909,7 @@ static bool conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { PQconninfoOption *option; + PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL; char *tmp; /* @@ -5892,6 +5926,9 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) */ 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 */ @@ -5936,6 +5973,13 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) } 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; } /* @@ -5969,6 +6013,20 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) } } + /* + * 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"); + } + } + return true; } diff --git a/src/interfaces/libpq/t/001_uri.pl b/src/interfaces/libpq/t/001_uri.pl index beaf13b49c..ccfcd77680 100644 --- a/src/interfaces/libpq/t/001_uri.pl +++ b/src/interfaces/libpq/t/001_uri.pl @@ -8,7 +8,9 @@ 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}, @@ -209,20 +211,44 @@ 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>', diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index b34a6f2a81..4747e72318 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -459,6 +459,12 @@ $node->connect_fails( "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; @@ -468,6 +474,12 @@ SKIP: $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 -- 2.25.1