From 8b845520fb0aa50fea7aae44a45cee1b6d87845d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 4 Aug 2016 14:44:23 -0400
Subject: [PATCH] Add tests for various connection string issues
Add tests for consistent support of connection strings in frontend
programs as well as proper handling of unusual characters in database
and user names. These tests were developed for the issues of
CVE-2016-5424.
To allow testing of names with spaces, change the pg_regress
command-line options --create-role and --dbname to split their arguments
by comma only, not space or comma as before. Only commas were actually
used in existing uses.
Noah Misch, Michael Paquier, Peter Eisentraut
---
src/bin/pg_dump/t/010_dump_connstr.pl | 142 ++++++++++++++++++++++++++
src/bin/pg_rewind/RewindTest.pm | 2 +-
src/bin/scripts/t/010_clusterdb.pl | 5 +-
src/bin/scripts/t/090_reindexdb.pl | 9 +-
src/bin/scripts/t/100_vacuumdb.pl | 4 +-
src/bin/scripts/t/200_connstr.pl | 38 +++++++
src/test/perl/PostgresNode.pm | 29 +++++-
src/test/perl/TestLib.pm | 14 +++
src/test/regress/pg_regress.c | 35 +++++--
9 files changed, 266 insertions(+), 12 deletions(-)
create mode 100644 src/bin/pg_dump/t/010_dump_connstr.pl
create mode 100644 src/bin/scripts/t/200_connstr.pl
diff --git a/src/bin/pg_dump/t/010_dump_connstr.pl b/src/bin/pg_dump/t/010_dump_connstr.pl
new file mode 100644
index 00000000000..2d0d1e4298e
--- /dev/null
+++ b/src/bin/pg_dump/t/010_dump_connstr.pl
@@ -0,0 +1,142 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 14;
+
+# In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to
+# interpret everything as UTF8. We're going to use byte sequences
+# that aren't valid UTF-8 strings, so that would fail. Use LATIN1,
+# which accepts any byte and has a conversion from each byte to UTF-8.
+$ENV{LC_ALL} = 'C';
+$ENV{PGCLIENTENCODING} = 'LATIN1';
+
+# Create database and user names covering the range of LATIN1
+# characters, for use in a connection string by pg_dumpall. Skip ','
+# because of pg_regress --create-role, skip [\n\r] because pg_dumpall
+# does not allow them.
+my $dbname1 = generate_ascii_string(1, 9) .
+ generate_ascii_string(11, 12) .
+ generate_ascii_string(14, 33) .
+ ($TestLib::windows_os ? '' : '"x"') . # IPC::Run mishandles '"' on Windows
+ generate_ascii_string(35, 43) .
+ generate_ascii_string(45, 63); # contains '='
+my $dbname2 = generate_ascii_string(67, 129); # skip 64-66 to keep length to 62
+my $dbname3 = generate_ascii_string(130, 192);
+my $dbname4 = generate_ascii_string(193, 255);
+
+my $node = get_new_node('main');
+$node->init(extra => ['--locale=C', '--encoding=LATIN1']);
+# prep pg_hba.conf and pg_ident.conf
+$node->run_log([$ENV{PG_REGRESS}, '--config-auth', $node->data_dir,
+ '--create-role', "$dbname1,$dbname2,$dbname3,$dbname4"]);
+$node->start;
+
+my $backupdir = $node->backup_dir;
+my $discard = "$backupdir/discard.sql";
+my $plain = "$backupdir/plain.sql";
+my $dirfmt = "$backupdir/dirfmt";
+
+foreach my $dbname ($dbname1, $dbname2, $dbname3, $dbname4, 'CamelCase')
+{
+ $node->run_log(['createdb', $dbname]);
+ $node->run_log(['createuser', '-s', $dbname]);
+}
+
+
+# For these tests, pg_dumpall -r is used because it produces a short
+# dump.
+$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
+ $node->connstr($dbname1), '-U', $dbname4],
+ 'pg_dumpall with long ASCII name 1');
+$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
+ $node->connstr($dbname2), '-U', $dbname3],
+ 'pg_dumpall with long ASCII name 2');
+$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
+ $node->connstr($dbname3), '-U', $dbname2],
+ 'pg_dumpall with long ASCII name 3');
+$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
+ $node->connstr($dbname4), '-U', $dbname1],
+ 'pg_dumpall with long ASCII name 4');
+$node->command_ok(['pg_dumpall', '-r', '-l', 'dbname=template1'],
+ 'pg_dumpall -l accepts connection string');
+
+$node->run_log(['createdb', "foo\n\rbar"]);
+# not sufficient to use -r here
+$node->command_fails(['pg_dumpall', '-f', $discard],
+ 'pg_dumpall with \n\r in database name');
+$node->run_log(['dropdb', "foo\n\rbar"]);
+
+
+# make a table, so the parallel worker has something to dump
+$node->safe_psql($dbname1, 'CREATE TABLE t0()');
+# XXX no printed message when this fails, just SIGPIPE termination
+$node->command_ok(['pg_dump', '-Fd', '-j2', '-f', $dirfmt,
+ '-U', $dbname1, $node->connstr($dbname1)],
+ 'parallel dump');
+
+# recreate $dbname1 for restore test
+$node->run_log(['dropdb', $dbname1]);
+$node->run_log(['createdb', $dbname1]);
+
+$node->command_ok(['pg_restore', '-v', '-d', 'template1', '-j2',
+ '-U', $dbname1, $dirfmt],
+ 'parallel restore');
+
+$node->run_log(['dropdb', $dbname1]);
+
+$node->command_ok(['pg_restore', '-C', '-v', '-d', 'template1', '-j2',
+ '-U', $dbname1, $dirfmt],
+ 'parallel restore with create');
+
+
+$node->command_ok(['pg_dumpall', '-f', $plain, '-U', $dbname1],
+ 'take full dump');
+system_log('cat', $plain);
+my($stderr, $result);
+my $bootstrap_super = 'boot';
+my $restore_super = qq{a'b\\c=d\\ne"f};
+
+
+# Restore full dump through psql using environment variables for
+# dbname/user connection parameters
+
+my $envar_node = get_new_node('destination_envar');
+$envar_node->init(extra => ['-U', $bootstrap_super,
+ '--locale=C', '--encoding=LATIN1']);
+$envar_node->run_log([$ENV{PG_REGRESS},
+ '--config-auth', $envar_node->data_dir,
+ '--create-role', "$bootstrap_super,$restore_super"]);
+$envar_node->start;
+
+# make superuser for restore
+$envar_node->run_log(['createuser', '-U', $bootstrap_super, '-s', $restore_super]);
+
+{
+ local $ENV{PGPORT} = $envar_node->port;
+ local $ENV{PGUSER} = $restore_super;
+ $result = run_log(['psql', '-X', '-f', $plain], '2>', \$stderr);
+}
+ok($result, 'restore full dump using environment variables for connection parameters');
+is($stderr, '', 'no dump errors');
+
+
+# Restore full dump through psql using command-line options for
+# dbname/user connection parameters. "\connect dbname=" forgets
+# user/port from command line.
+
+$restore_super =~ s/"//g if $TestLib::windows_os; # IPC::Run mishandles '"' on Windows
+my $cmdline_node = get_new_node('destination_cmdline');
+$cmdline_node->init(extra => ['-U', $bootstrap_super,
+ '--locale=C', '--encoding=LATIN1']);
+$cmdline_node->run_log([$ENV{PG_REGRESS},
+ '--config-auth', $cmdline_node->data_dir,
+ '--create-role', "$bootstrap_super,$restore_super"]);
+$cmdline_node->start;
+$cmdline_node->run_log(['createuser', '-U', $bootstrap_super, '-s', $restore_super]);
+{
+ $result = run_log(['psql', '-p', $cmdline_node->port, '-U', $restore_super, '-X', '-f', $plain], '2>', \$stderr);
+}
+ok($result, 'restore full dump with command-line options for connection parameters');
+is($stderr, '', 'no dump errors');
diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm
index 135d8f0449f..c7c3a8f45c6 100644
--- a/src/bin/pg_rewind/RewindTest.pm
+++ b/src/bin/pg_rewind/RewindTest.pm
@@ -133,7 +133,7 @@ sub create_standby
$node_standby = get_new_node('standby');
$node_master->backup('my_backup');
$node_standby->init_from_backup($node_master, 'my_backup');
- my $connstr_master = $node_master->connstr('postgres');
+ my $connstr_master = $node_master->connstr();
$node_standby->append_conf(
"recovery.conf", qq(
diff --git a/src/bin/scripts/t/010_clusterdb.pl b/src/bin/scripts/t/010_clusterdb.pl
index 0e677cacf18..f3de9c016ce 100644
--- a/src/bin/scripts/t/010_clusterdb.pl
+++ b/src/bin/scripts/t/010_clusterdb.pl
@@ -3,7 +3,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 13;
+use Test::More tests => 14;
program_help_ok('clusterdb');
program_version_ok('clusterdb');
@@ -28,3 +28,6 @@ $node->issues_sql_like(
[ 'clusterdb', '-t', 'test1' ],
qr/statement: CLUSTER test1;/,
'cluster specific table');
+
+$node->command_ok([qw(clusterdb --echo --verbose dbname=template1)],
+ 'clusterdb with connection string');
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index d92896f34f6..42d6fb4eb45 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -3,7 +3,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 20;
+use Test::More tests => 23;
program_help_ok('reindexdb');
program_version_ok('reindexdb');
@@ -42,3 +42,10 @@ $node->issues_sql_like(
[ 'reindexdb', '-v', '-t', 'test1', 'postgres' ],
qr/statement: REINDEX \(VERBOSE\) TABLE test1;/,
'reindex with verbose output');
+
+$node->command_ok([qw(reindexdb --echo --table=pg_am dbname=template1)],
+ 'reindexdb table with connection string');
+$node->command_ok([qw(reindexdb --echo dbname=template1)],
+ 'reindexdb database with connection string');
+$node->command_ok([qw(reindexdb --echo --system dbname=template1)],
+ 'reindexdb system with connection string');
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index c183ccb6a19..07c6e9e7ce1 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 18;
+use Test::More tests => 19;
program_help_ok('vacuumdb');
program_version_ok('vacuumdb');
@@ -33,3 +33,5 @@ $node->issues_sql_like(
[ 'vacuumdb', '-Z', 'postgres' ],
qr/statement: ANALYZE;/,
'vacuumdb -Z');
+$node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
+ 'vacuumdb with connection string');
diff --git a/src/bin/scripts/t/200_connstr.pl b/src/bin/scripts/t/200_connstr.pl
new file mode 100644
index 00000000000..89945712d29
--- /dev/null
+++ b/src/bin/scripts/t/200_connstr.pl
@@ -0,0 +1,38 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Tests to check connection string handling in utilities
+
+# In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to
+# interpret everything as UTF8. We're going to use byte sequences
+# that aren't valid UTF-8 strings, so that would fail. Use LATIN1,
+# which accepts any byte and has a conversion from each byte to UTF-8.
+$ENV{LC_ALL} = 'C';
+$ENV{PGCLIENTENCODING} = 'LATIN1';
+
+# Create database names covering the range of LATIN1 characters and
+# run the utilities' --all options over them.
+my $dbname1 = generate_ascii_string(1, 63); # contains '='
+my $dbname2 = generate_ascii_string(67, 129); # skip 64-66 to keep length to 62
+my $dbname3 = generate_ascii_string(130, 192);
+my $dbname4 = generate_ascii_string(193, 255);
+
+my $node = get_new_node('main');
+$node->init(extra => ['--locale=C', '--encoding=LATIN1']);
+$node->start;
+
+foreach my $dbname ($dbname1, $dbname2, $dbname3, $dbname4, 'CamelCase')
+{
+ $node->run_log(['createdb', $dbname]);
+}
+
+$node->command_ok([qw(vacuumdb --all --echo --analyze-only)],
+ 'vacuumdb --all with unusual database names');
+$node->command_ok([qw(reindexdb --all --echo)],
+ 'reindexdb --all with unusual database names');
+$node->command_ok([qw(clusterdb --all --echo --verbose)],
+ 'clusterdb --all with unusual database names');
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index fede1e601b9..afbdb6332bd 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -243,7 +243,13 @@ sub connstr
{
return "port=$pgport host=$pghost";
}
- return "port=$pgport host=$pghost dbname=$dbname";
+
+ # Escape properly the database string before using it, only
+ # single quotes and backslashes need to be treated this way.
+ $dbname =~ s#\\#\\\\#g;
+ $dbname =~ s#\'#\\\'#g;
+
+ return "port=$pgport host=$pghost dbname='$dbname'";
}
=pod
@@ -396,7 +402,8 @@ sub init
mkdir $self->backup_dir;
mkdir $self->archive_dir;
- TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N');
+ TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
+ @{ $params{extra} });
TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
open my $conf, ">>$pgdata/postgresql.conf";
@@ -1300,6 +1307,24 @@ sub issues_sql_like
=pod
+=item $node->run_log(...)
+
+Runs a shell command like TestLib::run_log, but with PGPORT set so
+that the command will default to connecting to this PostgresNode.
+
+=cut
+
+sub run_log
+{
+ my $self = shift;
+
+ local $ENV{PGPORT} = $self->port;
+
+ TestLib::run_log(@_);
+}
+
+=pod
+
=back
=cut
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 51b533e08cd..31e7acd4dae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -20,6 +20,7 @@ use SimpleTee;
use Test::More;
our @EXPORT = qw(
+ generate_ascii_string
slurp_dir
slurp_file
append_to_file
@@ -166,6 +167,19 @@ sub run_log
return IPC::Run::run(@_);
}
+# Generate a string made of the given range of ASCII characters
+sub generate_ascii_string
+{
+ my ($from_char, $to_char) = @_;
+ my $res;
+
+ for my $i ($from_char .. $to_char)
+ {
+ $res .= sprintf("%c", $i);
+ }
+ return $res;
+}
+
sub slurp_dir
{
my ($dir) = @_;
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 14c87c91abd..1154d4c300e 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -876,6 +876,29 @@ initialize_environment(void)
load_resultmap();
}
+pg_attribute_unused()
+static const char *
+fmtHba(const char *raw)
+{
+ static char *ret;
+ const char *rp;
+ char *wp;
+
+ wp = ret = realloc(ret, 3 + strlen(raw) * 2);
+
+ *wp++ = '"';
+ for (rp = raw; *rp; rp++)
+ {
+ if (*rp == '"')
+ *wp++ = '"';
+ *wp++ = *rp;
+ }
+ *wp++ = '"';
+ *wp++ = '\0';
+
+ return ret;
+}
+
#ifdef ENABLE_SSPI
/*
* Get account and domain/realm names for the current user. This is based on
@@ -1037,11 +1060,11 @@ config_sspi_auth(const char *pgdata)
* '#'. Windows forbids the double-quote character itself, so don't
* bother escaping embedded double-quote characters.
*/
- CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
- accountname, domainname, username) >= 0);
+ CW(fprintf(ident, "regress \"%s@%s\" %s\n",
+ accountname, domainname, fmtHba(username)) >= 0);
for (sl = extraroles; sl; sl = sl->next)
- CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
- accountname, domainname, sl->str) >= 0);
+ CW(fprintf(ident, "regress \"%s@%s\" %s\n",
+ accountname, domainname, fmtHba(sl->str)) >= 0);
CW(fclose(ident) == 0);
}
#endif
@@ -2064,7 +2087,7 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
* before we add the specified one.
*/
free_stringlist(&dblist);
- split_to_stringlist(optarg, ", ", &dblist);
+ split_to_stringlist(optarg, ",", &dblist);
break;
case 2:
debug = true;
@@ -2114,7 +2137,7 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
dlpath = pg_strdup(optarg);
break;
case 18:
- split_to_stringlist(optarg, ", ", &extraroles);
+ split_to_stringlist(optarg, ",", &extraroles);
break;
case 19:
add_stringlist_item(&temp_configs, optarg);
--
GitLab