From c822c579ea2c84c14a54c486328ac9ffed6184da Mon Sep 17 00:00:00 2001 From: Jacob Champion 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 OpenSSL in particular, the 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. - - The magic system 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 sslrootcert=./system 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 (). The TCP client must connect using sslmode=verify-ca or 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. To prevent spoofing with GSSAPI, the server must be configured to accept only hostgssenc 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