Skip to content

Commit 8b84552

Browse files
peterepetere
committed
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
1 parent e7010ce commit 8b84552

File tree

9 files changed

+266
-12
lines changed

9 files changed

+266
-12
lines changed

‎src/bin/pg_dump/t/010_dump_connstr.pl‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
use strict;
2+
use warnings;
3+
4+
use PostgresNode;
5+
use TestLib;
6+
use Test::More tests => 14;
7+
8+
# In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to
9+
# interpret everything as UTF8. We're going to use byte sequences
10+
# that aren't valid UTF-8 strings, so that would fail. Use LATIN1,
11+
# which accepts any byte and has a conversion from each byte to UTF-8.
12+
$ENV{LC_ALL} = 'C';
13+
$ENV{PGCLIENTENCODING} = 'LATIN1';
14+
15+
# Create database and user names covering the range of LATIN1
16+
# characters, for use in a connection string by pg_dumpall. Skip ','
17+
# because of pg_regress --create-role, skip [\n\r] because pg_dumpall
18+
# does not allow them.
19+
my $dbname1 = generate_ascii_string(1, 9) .
20+
generate_ascii_string(11, 12) .
21+
generate_ascii_string(14, 33) .
22+
($TestLib::windows_os ? '' : '"x"') . # IPC::Run mishandles '"' on Windows
23+
generate_ascii_string(35, 43) .
24+
generate_ascii_string(45, 63); # contains '='
25+
my $dbname2 = generate_ascii_string(67, 129); # skip 64-66 to keep length to 62
26+
my $dbname3 = generate_ascii_string(130, 192);
27+
my $dbname4 = generate_ascii_string(193, 255);
28+
29+
my $node = get_new_node('main');
30+
$node->init(extra => ['--locale=C', '--encoding=LATIN1']);
31+
# prep pg_hba.conf and pg_ident.conf
32+
$node->run_log([$ENV{PG_REGRESS}, '--config-auth', $node->data_dir,
33+
'--create-role', "$dbname1,$dbname2,$dbname3,$dbname4"]);
34+
$node->start;
35+
36+
my $backupdir = $node->backup_dir;
37+
my $discard = "$backupdir/discard.sql";
38+
my $plain = "$backupdir/plain.sql";
39+
my $dirfmt = "$backupdir/dirfmt";
40+
41+
foreach my $dbname ($dbname1, $dbname2, $dbname3, $dbname4, 'CamelCase')
42+
{
43+
$node->run_log(['createdb', $dbname]);
44+
$node->run_log(['createuser', '-s', $dbname]);
45+
}
46+
47+
48+
# For these tests, pg_dumpall -r is used because it produces a short
49+
# dump.
50+
$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
51+
$node->connstr($dbname1), '-U', $dbname4],
52+
'pg_dumpall with long ASCII name 1');
53+
$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
54+
$node->connstr($dbname2), '-U', $dbname3],
55+
'pg_dumpall with long ASCII name 2');
56+
$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
57+
$node->connstr($dbname3), '-U', $dbname2],
58+
'pg_dumpall with long ASCII name 3');
59+
$node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname',
60+
$node->connstr($dbname4), '-U', $dbname1],
61+
'pg_dumpall with long ASCII name 4');
62+
$node->command_ok(['pg_dumpall', '-r', '-l', 'dbname=template1'],
63+
'pg_dumpall -l accepts connection string');
64+
65+
$node->run_log(['createdb', "foo\n\rbar"]);
66+
# not sufficient to use -r here
67+
$node->command_fails(['pg_dumpall', '-f', $discard],
68+
'pg_dumpall with \n\r in database name');
69+
$node->run_log(['dropdb', "foo\n\rbar"]);
70+
71+
72+
# make a table, so the parallel worker has something to dump
73+
$node->safe_psql($dbname1, 'CREATE TABLE t0()');
74+
# XXX no printed message when this fails, just SIGPIPE termination
75+
$node->command_ok(['pg_dump', '-Fd', '-j2', '-f', $dirfmt,
76+
'-U', $dbname1, $node->connstr($dbname1)],
77+
'parallel dump');
78+
79+
# recreate $dbname1 for restore test
80+
$node->run_log(['dropdb', $dbname1]);
81+
$node->run_log(['createdb', $dbname1]);
82+
83+
$node->command_ok(['pg_restore', '-v', '-d', 'template1', '-j2',
84+
'-U', $dbname1, $dirfmt],
85+
'parallel restore');
86+
87+
$node->run_log(['dropdb', $dbname1]);
88+
89+
$node->command_ok(['pg_restore', '-C', '-v', '-d', 'template1', '-j2',
90+
'-U', $dbname1, $dirfmt],
91+
'parallel restore with create');
92+
93+
94+
$node->command_ok(['pg_dumpall', '-f', $plain, '-U', $dbname1],
95+
'take full dump');
96+
system_log('cat', $plain);
97+
my($stderr, $result);
98+
my $bootstrap_super = 'boot';
99+
my $restore_super = qq{a'b\\c=d\\ne"f};
100+
101+
102+
# Restore full dump through psql using environment variables for
103+
# dbname/user connection parameters
104+
105+
my $envar_node = get_new_node('destination_envar');
106+
$envar_node->init(extra => ['-U', $bootstrap_super,
107+
'--locale=C', '--encoding=LATIN1']);
108+
$envar_node->run_log([$ENV{PG_REGRESS},
109+
'--config-auth', $envar_node->data_dir,
110+
'--create-role', "$bootstrap_super,$restore_super"]);
111+
$envar_node->start;
112+
113+
# make superuser for restore
114+
$envar_node->run_log(['createuser', '-U', $bootstrap_super, '-s', $restore_super]);
115+
116+
{
117+
local $ENV{PGPORT} = $envar_node->port;
118+
local $ENV{PGUSER} = $restore_super;
119+
$result = run_log(['psql', '-X', '-f', $plain], '2>', \$stderr);
120+
}
121+
ok($result, 'restore full dump using environment variables for connection parameters');
122+
is($stderr, '', 'no dump errors');
123+
124+
125+
# Restore full dump through psql using command-line options for
126+
# dbname/user connection parameters. "\connect dbname=" forgets
127+
# user/port from command line.
128+
129+
$restore_super =~ s/"//g if $TestLib::windows_os; # IPC::Run mishandles '"' on Windows
130+
my $cmdline_node = get_new_node('destination_cmdline');
131+
$cmdline_node->init(extra => ['-U', $bootstrap_super,
132+
'--locale=C', '--encoding=LATIN1']);
133+
$cmdline_node->run_log([$ENV{PG_REGRESS},
134+
'--config-auth', $cmdline_node->data_dir,
135+
'--create-role', "$bootstrap_super,$restore_super"]);
136+
$cmdline_node->start;
137+
$cmdline_node->run_log(['createuser', '-U', $bootstrap_super, '-s', $restore_super]);
138+
{
139+
$result = run_log(['psql', '-p', $cmdline_node->port, '-U', $restore_super, '-X', '-f', $plain], '2>', \$stderr);
140+
}
141+
ok($result, 'restore full dump with command-line options for connection parameters');
142+
is($stderr, '', 'no dump errors');

‎src/bin/pg_rewind/RewindTest.pm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ sub create_standby
133133
$node_standby = get_new_node('standby');
134134
$node_master->backup('my_backup');
135135
$node_standby->init_from_backup($node_master, 'my_backup');
136-
my $connstr_master = $node_master->connstr('postgres');
136+
my $connstr_master = $node_master->connstr();
137137

138138
$node_standby->append_conf(
139139
"recovery.conf", qq(

‎src/bin/scripts/t/010_clusterdb.pl‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
use PostgresNode;
55
use TestLib;
6-
use Test::More tests => 13;
6+
use Test::More tests => 14;
77

88
program_help_ok('clusterdb');
99
program_version_ok('clusterdb');
@@ -28,3 +28,6 @@
2828
[ 'clusterdb', '-t', 'test1' ],
2929
qr/statement: CLUSTER test1;/,
3030
'cluster specific table');
31+
32+
$node->command_ok([qw(clusterdb --echo --verbose dbname=template1)],
33+
'clusterdb with connection string');

‎src/bin/scripts/t/090_reindexdb.pl‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
use PostgresNode;
55
use TestLib;
6-
use Test::More tests => 20;
6+
use Test::More tests => 23;
77

88
program_help_ok('reindexdb');
99
program_version_ok('reindexdb');
@@ -42,3 +42,10 @@
4242
[ 'reindexdb', '-v', '-t', 'test1', 'postgres' ],
4343
qr/statement: REINDEX \(VERBOSE\) TABLE test1;/,
4444
'reindex with verbose output');
45+
46+
$node->command_ok([qw(reindexdb --echo --table=pg_am dbname=template1)],
47+
'reindexdb table with connection string');
48+
$node->command_ok([qw(reindexdb --echo dbname=template1)],
49+
'reindexdb database with connection string');
50+
$node->command_ok([qw(reindexdb --echo --system dbname=template1)],
51+
'reindexdb system with connection string');

‎src/bin/scripts/t/100_vacuumdb.pl‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
use PostgresNode;
55
use TestLib;
6-
use Test::More tests => 18;
6+
use Test::More tests => 19;
77

88
program_help_ok('vacuumdb');
99
program_version_ok('vacuumdb');
@@ -33,3 +33,5 @@
3333
[ 'vacuumdb', '-Z', 'postgres' ],
3434
qr/statement: ANALYZE;/,
3535
'vacuumdb -Z');
36+
$node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
37+
'vacuumdb with connection string');

‎src/bin/scripts/t/200_connstr.pl‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
use strict;
2+
use warnings;
3+
4+
use PostgresNode;
5+
use TestLib;
6+
use Test::More tests => 3;
7+
8+
# Tests to check connection string handling in utilities
9+
10+
# In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to
11+
# interpret everything as UTF8. We're going to use byte sequences
12+
# that aren't valid UTF-8 strings, so that would fail. Use LATIN1,
13+
# which accepts any byte and has a conversion from each byte to UTF-8.
14+
$ENV{LC_ALL} = 'C';
15+
$ENV{PGCLIENTENCODING} = 'LATIN1';
16+
17+
# Create database names covering the range of LATIN1 characters and
18+
# run the utilities' --all options over them.
19+
my $dbname1 = generate_ascii_string(1, 63); # contains '='
20+
my $dbname2 = generate_ascii_string(67, 129); # skip 64-66 to keep length to 62
21+
my $dbname3 = generate_ascii_string(130, 192);
22+
my $dbname4 = generate_ascii_string(193, 255);
23+
24+
my $node = get_new_node('main');
25+
$node->init(extra => ['--locale=C', '--encoding=LATIN1']);
26+
$node->start;
27+
28+
foreach my $dbname ($dbname1, $dbname2, $dbname3, $dbname4, 'CamelCase')
29+
{
30+
$node->run_log(['createdb', $dbname]);
31+
}
32+
33+
$node->command_ok([qw(vacuumdb --all --echo --analyze-only)],
34+
'vacuumdb --all with unusual database names');
35+
$node->command_ok([qw(reindexdb --all --echo)],
36+
'reindexdb --all with unusual database names');
37+
$node->command_ok([qw(clusterdb --all --echo --verbose)],
38+
'clusterdb --all with unusual database names');

‎src/test/perl/PostgresNode.pm‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,13 @@ sub connstr
243243
{
244244
return "port=$pgport host=$pghost";
245245
}
246-
return "port=$pgport host=$pghost dbname=$dbname";
246+
247+
# Escape properly the database string before using it, only
248+
# single quotes and backslashes need to be treated this way.
249+
$dbname =~ s#\\#\\\\#g;
250+
$dbname =~ s#\'#\\\'#g;
251+
252+
return "port=$pgport host=$pghost dbname='$dbname'";
247253
}
248254

249255
=pod
@@ -396,7 +402,8 @@ sub init
396402
mkdir $self->backup_dir;
397403
mkdir $self->archive_dir;
398404

399-
TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N');
405+
TestLib::system_or_bail('initdb', '-D', $pgdata, '-A', 'trust', '-N',
406+
@{ $params{extra} });
400407
TestLib::system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
401408

402409
open my $conf, ">>$pgdata/postgresql.conf";
@@ -1300,6 +1307,24 @@ sub issues_sql_like
13001307

13011308
=pod
13021309
1310+
=item $node->run_log(...)
1311+
1312+
Runs a shell command like TestLib::run_log, but with PGPORT set so
1313+
that the command will default to connecting to this PostgresNode.
1314+
1315+
=cut
1316+
1317+
sub run_log
1318+
{
1319+
my $self = shift;
1320+
1321+
local $ENV{PGPORT} = $self->port;
1322+
1323+
TestLib::run_log(@_);
1324+
}
1325+
1326+
=pod
1327+
13031328
=back
13041329
13051330
=cut

‎src/test/perl/TestLib.pm‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use SimpleTee;
2020
use Test::More;
2121

2222
our @EXPORT = qw(
23+
generate_ascii_string
2324
slurp_dir
2425
slurp_file
2526
append_to_file
@@ -166,6 +167,19 @@ sub run_log
166167
return IPC::Run::run(@_);
167168
}
168169

170+
# Generate a string made of the given range of ASCII characters
171+
sub generate_ascii_string
172+
{
173+
my ($from_char, $to_char) = @_;
174+
my $res;
175+
176+
for my $i ($from_char .. $to_char)
177+
{
178+
$res .= sprintf("%c", $i);
179+
}
180+
return $res;
181+
}
182+
169183
sub slurp_dir
170184
{
171185
my ($dir) = @_;

‎src/test/regress/pg_regress.c‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -876,6 +876,29 @@ initialize_environment(void)
876876
load_resultmap();
877877
}
878878

879+
pg_attribute_unused()
880+
static const char *
881+
fmtHba(const char *raw)
882+
{
883+
static char *ret;
884+
const char *rp;
885+
char *wp;
886+
887+
wp = ret = realloc(ret, 3 + strlen(raw) * 2);
888+
889+
*wp++ = '"';
890+
for (rp = raw; *rp; rp++)
891+
{
892+
if (*rp == '"')
893+
*wp++ = '"';
894+
*wp++ = *rp;
895+
}
896+
*wp++ = '"';
897+
*wp++ = '\0';
898+
899+
return ret;
900+
}
901+
879902
#ifdef ENABLE_SSPI
880903
/*
881904
* Get account and domain/realm names for the current user. This is based on
@@ -1037,11 +1060,11 @@ config_sspi_auth(const char *pgdata)
10371060
* '#'. Windows forbids the double-quote character itself, so don't
10381061
* bother escaping embedded double-quote characters.
10391062
*/
1040-
CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
1041-
accountname, domainname, username) >= 0);
1063+
CW(fprintf(ident, "regress \"%s@%s\" %s\n",
1064+
accountname, domainname, fmtHba(username)) >= 0);
10421065
for (sl = extraroles; sl; sl = sl->next)
1043-
CW(fprintf(ident, "regress \"%s@%s\" \"%s\"\n",
1044-
accountname, domainname, sl->str) >= 0);
1066+
CW(fprintf(ident, "regress \"%s@%s\" %s\n",
1067+
accountname, domainname, fmtHba(sl->str)) >= 0);
10451068
CW(fclose(ident) == 0);
10461069
}
10471070
#endif
@@ -2064,7 +2087,7 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
20642087
* before we add the specified one.
20652088
*/
20662089
free_stringlist(&dblist);
2067-
split_to_stringlist(optarg, ", ", &dblist);
2090+
split_to_stringlist(optarg, ",", &dblist);
20682091
break;
20692092
case 2:
20702093
debug = true;
@@ -2114,7 +2137,7 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
21142137
dlpath = pg_strdup(optarg);
21152138
break;
21162139
case 18:
2117-
split_to_stringlist(optarg, ", ", &extraroles);
2140+
split_to_stringlist(optarg, ",", &extraroles);
21182141
break;
21192142
case 19:
21202143
add_stringlist_item(&temp_configs, optarg);

0 commit comments

Comments
 (0)