Disallow CR and LF in database, role, and tablespace names

Previously, these characters could cause problems when passed through
shell commands, and were flagged with a comment in string_utils.c
suggesting they be rejected in a future major release.

The affected commands are CREATE DATABASE, CREATE ROLE, CREATE TABLESPACE,
ALTER DATABASE RENAME, ALTER ROLE RENAME, and ALTER TABLESPACE RENAME.

Also add a pg_upgrade check to detect these invalid names in clusters
being upgraded from pre-v19 versions, producing a report file listing
any offending objects that must be renamed before upgrading.

Tests have been modified accordingly.

Author: Mahendra Singh Thalor <mahi6run@gmail.com>
Reviewed-By: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-By: Andrew Dunstan <andrew@dunslane.net>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-By: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-By: Srinath Reddy <srinath2133@gmail.com>

Discussion: https://postgr.es/m/CAKYtNApkOi4FY0S7+3jpTqnHVyyZ6Tbzhtbah-NBbY-mGsiKAQ@mail.gmail.com

src/backend/commands/dbcommands.c

@@ -743,6 +743,12 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
 	CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
 	createdb_failure_params fparms;
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(dbname, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("database name \"%s\" contains a newline or carriage return character", dbname)));
+
 	/* Extract options from the statement node tree */
 	foreach(option, stmt->options)
 	{
@@ -1911,6 +1917,12 @@ RenameDatabase(const char *oldname, const char *newname)
 	int			npreparedxacts;
 	ObjectAddress address;
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(newname, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("database name \"%s\" contains a newline or carriage return character", newname)));
+
 	/*
 	 * Look up the target database's OID, and get exclusive lock on it. We
 	 * need this for the same reasons as DROP DATABASE.

src/backend/commands/tablespace.c

@@ -242,6 +242,12 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
 				(errcode(ERRCODE_INVALID_NAME),
 				 errmsg("tablespace location cannot contain single quotes")));
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(stmt->tablespacename, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("tablespace name \"%s\" contains a newline or carriage return character", stmt->tablespacename)));
+
 	in_place = allow_in_place_tablespaces && strlen(location) == 0;
 
 	/*
@@ -971,6 +977,12 @@ RenameTableSpace(const char *oldname, const char *newname)
 				 errmsg("unacceptable tablespace name \"%s\"", newname),
 				 errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(newname, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("tablespace name \"%s\" contains a newline or carriage return character", newname)));
+
 	/*
 	 * If built with appropriate switch, whine when regression-testing
 	 * conventions for tablespace names are violated.

src/backend/commands/user.c

@@ -171,6 +171,12 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	DefElem    *dbypassRLS = NULL;
 	GrantRoleOptions popt;
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(stmt->role, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("role name \"%s\" contains a newline or carriage return character", stmt->role)));
+
 	/* The defaults can vary depending on the original statement type */
 	switch (stmt->stmt_type)
 	{
@@ -1348,6 +1354,12 @@ RenameRole(const char *oldname, const char *newname)
 	ObjectAddress address;
 	Form_pg_authid authform;
 
+	/* Report error if name has \n or \r character. */
+	if (strpbrk(newname, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("role name \"%s\" contains a newline or carriage return character", newname)));
+
 	rel = table_open(AuthIdRelationId, RowExclusiveLock);
 	dsc = RelationGetDescr(rel);
 

src/bin/pg_dump/t/002_pg_dump.pl

@@ -2048,13 +2048,8 @@ my %tests = (
 		},
 	},
 
-	'newline of role or table name in comment' => {
-		create_sql => qq{CREATE ROLE regress_newline;
-						 ALTER ROLE regress_newline SET enable_seqscan = off;
-						 ALTER ROLE regress_newline
-							RENAME TO "regress_newline\nattack";
-
-						 -- meet getPartitioningInfo() "unsafe" condition
+	'newline of table name in comment' => {
+		create_sql => qq{-- meet getPartitioningInfo() "unsafe" condition
 						 CREATE TYPE pp_colors AS
 							ENUM ('green', 'blue', 'black');
 						 CREATE TABLE pp_enumpart (a pp_colors)

src/bin/pg_dump/t/003_pg_dump_with_server.pl

@@ -16,22 +16,6 @@ my $port = $node->port;
 $node->init;
 $node->start;
 
-#########################################
-# pg_dumpall: newline in database name
-
-$node->safe_psql('postgres', qq{CREATE DATABASE "regress_\nattack"});
-
-my (@cmd, $stdout, $stderr);
-@cmd = ("pg_dumpall", '--port' => $port, '--exclude-database=postgres');
-print("# Running: " . join(" ", @cmd) . "\n");
-my $result = IPC::Run::run \@cmd, '>' => \$stdout, '2>' => \$stderr;
-ok(!$result, "newline in dbname: exit code not 0");
-like(
-	$stderr,
-	qr/shell command argument contains a newline/,
-	"newline in dbname: stderr matches");
-unlike($stdout, qr/^attack/m, "newline in dbname: no comment escape");
-
 #########################################
 # Verify that dumping foreign data includes only foreign tables of
 # matching servers

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

@@ -153,20 +153,6 @@ $node->command_ok(
 	],
 	'pg_dumpall --dbname accepts connection string');
 
-$node->run_log(
-	[ 'createdb', '--username' => $src_bootstrap_super, "foo\n\rbar" ]);
-
-# not sufficient to use --roles-only here
-$node->command_fails(
-	[
-		'pg_dumpall', '--no-sync',
-		'--username' => $src_bootstrap_super,
-		'--file' => $discard,
-	],
-	'pg_dumpall with \n\r in database name');
-$node->run_log(
-	[ 'dropdb', '--username' => $src_bootstrap_super, "foo\n\rbar" ]);
-
 
 # make a table, so the parallel worker has something to dump
 $node->safe_psql(

src/bin/pg_upgrade/check.c

@@ -34,6 +34,7 @@ static void check_new_cluster_replication_slots(void);
 static void check_new_cluster_subscription_configuration(void);
 static void check_old_cluster_for_valid_slots(void);
 static void check_old_cluster_subscription_state(void);
+static void check_old_cluster_global_names(ClusterInfo *cluster);
 
 /*
  * DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -600,6 +601,14 @@ check_and_dump_old_cluster(void)
 	 */
 	check_for_connection_status(&old_cluster);
 
+	/*
+	 * Validate database, user, role and tablespace names from the old cluster.
+	 * No need to check in 19 or newer as newline and carriage return are
+	 * not allowed at the creation time of the object.
+	 */
+	if (GET_MAJOR_VERSION(old_cluster.major_version) < 1900)
+		check_old_cluster_global_names(&old_cluster);
+
 	/*
 	 * Extract a list of databases, tables, and logical replication slots from
 	 * the old cluster.
@@ -2500,3 +2509,73 @@ check_old_cluster_subscription_state(void)
 	else
 		check_ok();
 }
+
+/*
+ * check_old_cluster_global_names()
+ *
+ * Raise an error if any database, role, or tablespace name contains a newline
+ * or carriage return character.  Such names are not allowed in v19 and later.
+ */
+static void
+check_old_cluster_global_names(ClusterInfo *cluster)
+{
+	int			i;
+	PGconn		*conn_template1;
+	PGresult	*res;
+	int			ntups;
+	FILE		*script = NULL;
+	char		output_path[MAXPGPATH];
+	int			count = 0;
+
+	prep_status("Checking names of databases, roles and tablespaces");
+
+	snprintf(output_path, sizeof(output_path), "%s/%s",
+			log_opts.basedir,
+			"db_role_tablespace_invalid_names.txt");
+
+	conn_template1 = connectToServer(cluster, "template1");
+
+	/*
+	 * Get database, user/role and tablespacenames from cluster.  Can't use
+	 * pg_authid because only superusers can view it.
+	 */
+	res = executeQueryOrDie(conn_template1,
+			"SELECT datname AS objname, 'database' AS objtype "
+			"FROM pg_catalog.pg_database UNION ALL "
+			"SELECT rolname AS objname, 'role' AS objtype "
+			"FROM pg_catalog.pg_roles UNION ALL "
+			"SELECT spcname AS objname, 'tablespace' AS objtype "
+			"FROM pg_catalog.pg_tablespace ORDER BY 2 ");
+
+	ntups = PQntuples(res);
+	for (i = 0; i < ntups; i++)
+	{
+		char	*objname = PQgetvalue(res, i, 0);
+		char	*objtype = PQgetvalue(res, i, 1);
+
+		/* If name has \n or \r, then report it. */
+		if (strpbrk(objname, "\n\r"))
+		{
+			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+				pg_fatal("could not open file \"%s\": %m", output_path);
+
+			fprintf(script, "%d : %s name = \"%s\"\n", ++count, objtype, objname);
+		}
+	}
+
+	PQclear(res);
+	PQfinish(conn_template1);
+
+	if (script)
+	{
+		fclose(script);
+		pg_log(PG_REPORT, "fatal");
+		pg_fatal("All the database, role and tablespace names should have only valid characters. A newline or \n"
+				"carriage return character is not allowed in these object names.  To fix this, please \n"
+				"rename these names with valid names. \n"
+				"To see all %d invalid object names, refer db_role_tablespace_invalid_names.txt file. \n"
+				"    %s", count, output_path);
+	}
+	else
+		check_ok();
+}

src/bin/scripts/t/020_createdb.pl

@@ -241,6 +241,18 @@ $node->command_fails(
 	],
 	'fails for invalid locale provider');
 
+$node->command_fails_like(
+    [ 'createdb', "invalid \n dbname" ],
+    qr(contains a newline or carriage return character),
+    'fails if database name contains a newline character in name'
+);
+
+$node->command_fails_like(
+    [ 'createdb', "invalid \r dbname" ],
+    qr(contains a newline or carriage return character),
+    'fails if database name contains a carriage return character in name'
+);
+
 # Check use of templates with shared dependencies copied from the template.
 my ($ret, $stdout, $stderr) = $node->psql(
 	'foobar2',

src/fe_utils/string_utils.c

@@ -568,12 +568,6 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
  * Append the given string to the shell command being built in the buffer,
  * with shell-style quoting as needed to create exactly one argument.
  *
- * 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.
- *
  * appendShellString() simply prints an error and dies if LF or CR appears.
  * appendShellStringNoError() omits those characters from the result, and
  * returns false if there were any.

src/test/modules/unsafe_tests/expected/alter_system_table.out

@@ -64,6 +64,11 @@ ERROR:  permission denied: "pg_description" is a system catalog
 CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
 ERROR:  unacceptable tablespace name "pg_foo"
 DETAIL:  The prefix "pg_" is reserved for system tablespaces.
+-- contains \n\r tablespace name
+CREATE TABLESPACE "invalid
+name" LOCATION '/no/such/location';
+ERROR:  tablespace name "invalid
+name" contains a newline or carriage return character
 -- triggers
 CREATE FUNCTION tf1() RETURNS trigger
 LANGUAGE plpgsql

src/test/modules/unsafe_tests/expected/rolenames.out

@@ -102,6 +102,10 @@ DETAIL:  Role names starting with "pg_" are reserved.
 CREATE ROLE "pg_abcdef"; -- error
 ERROR:  role name "pg_abcdef" is reserved
 DETAIL:  Role names starting with "pg_" are reserved.
+CREATE ROLE "invalid
+rolename"; -- error
+ERROR:  role name "invalid
+rolename" contains a newline or carriage return character
 CREATE ROLE regress_testrol0 SUPERUSER LOGIN;
 CREATE ROLE regress_testrolx SUPERUSER LOGIN;
 CREATE ROLE regress_testrol2 SUPERUSER;

src/test/modules/unsafe_tests/sql/alter_system_table.sql

@@ -64,6 +64,10 @@ ALTER TABLE pg_description SET SCHEMA public;
 -- reserved tablespace name
 CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
 
+-- contains \n\r tablespace name
+CREATE TABLESPACE "invalid
+name" LOCATION '/no/such/location';
+
 -- triggers
 CREATE FUNCTION tf1() RETURNS trigger
 LANGUAGE plpgsql

src/test/modules/unsafe_tests/sql/rolenames.sql

@@ -75,6 +75,8 @@ CREATE ROLE pg_abc; -- error
 CREATE ROLE "pg_abc"; -- error
 CREATE ROLE pg_abcdef; -- error
 CREATE ROLE "pg_abcdef"; -- error
+CREATE ROLE "invalid
+rolename"; -- error
 
 CREATE ROLE regress_testrol0 SUPERUSER LOGIN;
 CREATE ROLE regress_testrolx SUPERUSER LOGIN;

src/test/regress/expected/tablespace.out

@@ -958,6 +958,11 @@ ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default
 NOTICE:  no matching relations in tablespace "regress_tblspace_renamed" found
 ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
 NOTICE:  no matching relations in tablespace "regress_tblspace_renamed" found
+-- Should fail, contains \n in name
+ALTER TABLESPACE regress_tblspace_renamed RENAME TO "invalid
+name";
+ERROR:  tablespace name "invalid
+name" contains a newline or carriage return character
 -- Should succeed
 DROP TABLESPACE regress_tblspace_renamed;
 DROP SCHEMA testschema CASCADE;

src/test/regress/sql/tablespace.sql

@@ -430,6 +430,10 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPAC
 ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
 ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
 
+-- Should fail, contains \n in name
+ALTER TABLESPACE regress_tblspace_renamed RENAME TO "invalid
+name";
+
 -- Should succeed
 DROP TABLESPACE regress_tblspace_renamed;