From 217d934d0c252e312610a21cd75fb01082bb9bad Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 1 Feb 2017 13:25:51 +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 | 11 +---------- 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(+), 15 deletions(-) diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml index cf33746c1e..2477ba72bd 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 a3b8ed9ab4..28e0435731 100644 --- a/doc/src/sgml/ref/create_role.sgml +++ b/doc/src/sgml/ref/create_role.sgml @@ -398,6 +398,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 c3eb3c79df..f76af26c64 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -78,6 +78,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); @@ -470,6 +471,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 @@ -756,6 +760,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) @@ -976,6 +996,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 4422fadd52..52d53e8e11 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) { @@ -1146,6 +1164,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 81b5779248..74b5539691 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 @@ -77,15 +77,6 @@ $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()'); diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl index c0f6067a92..765ab0ea6f 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 f4fc7ea3a4..778c2feea7 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 08fe765b5a..876c877373 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.11.0