From 4b5fa316e9b7bc1b981537d9081874a17109497c Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 22 Nov 2016 22:21:36 +0900 Subject: [PATCH 1/2] Forbid newline and carriage return characters in database and role names --- doc/src/sgml/ref/create_database.sgml | 7 +++++++ doc/src/sgml/ref/create_role.sgml | 7 +++++++ src/backend/commands/dbcommands.c | 23 +++++++++++++++++++++++ src/backend/commands/user.c | 21 +++++++++++++++++++++ src/bin/pg_dump/t/010_dump_connstr.pl | 9 +-------- src/bin/scripts/t/020_createdb.pl | 4 +++- src/bin/scripts/t/040_createuser.pl | 4 +++- src/fe_utils/string_utils.c | 4 +--- 8 files changed, 66 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml index cf33746..2477ba7 100644 --- a/doc/src/sgml/ref/create_database.sgml +++ b/doc/src/sgml/ref/create_database.sgml @@ -260,6 +260,13 @@ CREATE DATABASE name connection slot remains for the database, it is possible that both will fail. Also, the limit is not enforced against superusers. + + + Database names cannot include newline or carriage return characters + as those could be at the origin of security breaches, particularly on + Windows where the command shell is unusable with arguments containing + such characters. + diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml index 38cd4c8..ff4b6f6 100644 --- a/doc/src/sgml/ref/create_role.sgml +++ b/doc/src/sgml/ref/create_role.sgml @@ -404,6 +404,13 @@ CREATE ROLE name [ [ WITH ] \password that can be used to safely change the password later. + + + Role names cannot include newline or carriage return characters + as those could be at the origin of security breaches, particularly on + Windows where the command shell is unusable with arguments containing + such characters. + diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 0919ad8..36c5fb4 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -77,6 +77,7 @@ typedef struct } movedb_failure_params; /* non-export function prototypes */ +static void check_db_name(const char *dbname); static void createdb_failure_callback(int code, Datum arg); static void movedb(const char *dbname, const char *tblspcname); static void movedb_failure_callback(int code, Datum arg); @@ -469,6 +470,9 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) /* Note there is no additional permission check in this path */ } + /* do sanity checks on database name */ + check_db_name(dbname); + /* * Check for db name conflict. This is just to give a more friendly error * message than "unique index violation". There's a race condition but @@ -758,6 +762,22 @@ check_encoding_locale_matches(int encoding, const char *collate, const char *cty pg_encoding_to_char(collate_encoding)))); } +/* + * Perform sanity checks on the database name. + */ +static void +check_db_name(const char *dbname) +{ + if (strchr(dbname, '\n') != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("database name cannot include newline character"))); + if (strchr(dbname, '\r') != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("database name cannot include carriage return character"))); +} + /* Error cleanup callback for createdb */ static void createdb_failure_callback(int code, Datum arg) @@ -962,6 +982,9 @@ RenameDatabase(const char *oldname, const char *newname) int npreparedxacts; ObjectAddress address; + /* check format of new name */ + check_db_name(newname); + /* * Look up the target database's OID, and get exclusive lock on it. We * need this for the same reasons as DROP DATABASE. diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index adc6b99..e3519d6 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -57,6 +57,21 @@ static void DelRoleMems(const char *rolename, Oid roleid, bool admin_opt); +/* Do sanity checks on role name */ +static void +check_role_name(const char *rolname) +{ + if (strchr(rolname, '\n') != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("role name cannot use newline character"))); + if (strchr(rolname, '\r') != NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("role name cannot use carriage return character"))); +} + + /* Check if current user has createrole privileges */ static bool have_createrole_privilege(void) @@ -111,6 +126,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) DefElem *dvalidUntil = NULL; DefElem *dbypassRLS = NULL; + /* sanity check for role name */ + check_role_name(stmt->role); + /* The defaults can vary depending on the original statement type */ switch (stmt->stmt_type) { @@ -1150,6 +1168,9 @@ RenameRole(const char *oldname, const char *newname) ObjectAddress address; Form_pg_authid authform; + /* sanity check for role name */ + check_role_name(newname); + rel = heap_open(AuthIdRelationId, RowExclusiveLock); dsc = RelationGetDescr(rel); diff --git a/src/bin/pg_dump/t/010_dump_connstr.pl b/src/bin/pg_dump/t/010_dump_connstr.pl index 2d0d1e4..475f207 100644 --- a/src/bin/pg_dump/t/010_dump_connstr.pl +++ b/src/bin/pg_dump/t/010_dump_connstr.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 14; +use Test::More tests => 13; # In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to # interpret everything as UTF8. We're going to use byte sequences @@ -62,13 +62,6 @@ $node->command_ok(['pg_dumpall', '-r', '-f', $discard, '--dbname', $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 diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl index c0f6067..765ab0e 100644 --- a/src/bin/scripts/t/020_createdb.pl +++ b/src/bin/scripts/t/020_createdb.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 13; +use Test::More tests => 14; program_help_ok('createdb'); program_version_ok('createdb'); @@ -24,3 +24,5 @@ $node->issues_sql_like( $node->command_fails([ 'createdb', 'foobar1' ], 'fails if database already exists'); +$node->command_fails([ 'createdb', "foo\n\rbar" ], + 'fails with \n\r in database name'); diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl index f4fc7ea..778c2fe 100644 --- a/src/bin/scripts/t/040_createuser.pl +++ b/src/bin/scripts/t/040_createuser.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 17; +use Test::More tests => 18; program_help_ok('createuser'); program_version_ok('createuser'); @@ -32,3 +32,5 @@ qr/statement: CREATE ROLE regress_user3 SUPERUSER CREATEDB CREATEROLE INHERIT LO $node->command_fails([ 'createuser', 'regress_user1' ], 'fails if role already exists'); +$node->command_fails([ 'createuser', "foo\n\rbar" ], + 'fails with \n\r in role name'); diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c index 61bf9e6..1c3d9b3 100644 --- a/src/fe_utils/string_utils.c +++ b/src/fe_utils/string_utils.c @@ -422,9 +422,7 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length, * * Forbid LF or CR characters, which have scant practical use beyond designing * security breaches. The Windows command shell is unusable as a conduit for - * arguments containing LF or CR characters. A future major release should - * reject those characters in CREATE ROLE and CREATE DATABASE, because use - * there eventually leads to errors here. + * arguments containing LF or CR characters. */ void appendShellString(PQExpBuffer buf, const char *str) -- 2.10.2