Forbid use of LF and CR characters in database and role names

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index c1c0223..5746958 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);
@@ -456,6 +457,9 @@ createdb(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
@@ -745,6 +749,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 use LF character")));
+	if (strchr(dbname, '\r') != NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("database name cannot use CR character")));
+}
+
 /* Error cleanup callback for createdb */
 static void
 createdb_failure_callback(int code, Datum arg)
@@ -949,6 +969,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 b6ea950..8954e16 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 LF character")));
+	if (strchr(rolname, '\r') != NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("role name cannot use CR character")));
+}
+
+
 /* Check if current user has createrole privileges */
 static bool
 have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(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)
 	{
@@ -1137,6 +1155,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/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index cf33746..d73dc3a 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -260,6 +260,13 @@ CREATE DATABASE <replaceable class="PARAMETER">name</replaceable>
    connection <quote>slot</> remains for the database, it is possible that
    both will fail.  Also, the limit is not enforced against superusers.
   </para>
+
+  <para>
+    Database names cannot include <literal>LF</> or <literal>CR</> 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.
+   </para>
  </refsect1>
 
  <refsect1>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..27259f2 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -404,6 +404,13 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
    <command>\password</command> that can be used to safely change the
    password later.
   </para>
+
+  <para>
+   Role names cannot include <literal>LF</> or <literal>CR</> 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.
+  <para>
  </refsect1>
 
  <refsect1>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index c1c0223..5746958 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);
@@ -456,6 +457,9 @@ createdb(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
@@ -745,6 +749,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 use LF character")));
+	if (strchr(dbname, '\r') != NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("database name cannot use CR character")));
+}
+
 /* Error cleanup callback for createdb */
 static void
 createdb_failure_callback(int code, Datum arg)
@@ -949,6 +969,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 b6ea950..8954e16 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 LF character")));
+	if (strchr(rolname, '\r') != NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("role name cannot use CR character")));
+}
+
+
 /* Check if current user has createrole privileges */
 static bool
 have_createrole_privilege(void)
@@ -111,6 +126,9 @@ CreateRole(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)
 	{
@@ -1137,6 +1155,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);
 

From 016a8168b39b67a6468ee1a752ee9ec82cf3fecb Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 7 Sep 2016 16:39:57 +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/fe_utils/string_utils.c           |  4 +---
 5 files changed, 59 insertions(+), 3 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 <replaceable class="PARAMETER">name</replaceable>
    connection <quote>slot</> remains for the database, it is possible that
    both will fail.  Also, the limit is not enforced against superusers.
   </para>
+
+  <para>
+    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.
+   </para>
  </refsect1>
 
  <refsect1>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..9d6926e 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -404,6 +404,13 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
    <command>\password</command> that can be used to safely change the
    password later.
   </para>
+
+  <para>
+   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.
+  <para>
  </refsect1>
 
  <refsect1>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index ef48659..22712ec 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 4027c89..fd737d8 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/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.0

From 8fbf60619e12f5cd10ba90cba59496f7c962f8c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 7 Sep 2016 16:55:15 +0900
Subject: [PATCH 2/2] Ensure clean up of data directory even with restricted
 path applied

Newline and carriage return characters are forbidden by appendShellCommand,
call used when generating the command recommended to the user to launch a
server. However in the case where the data directory contained such characters
initdb did not perform any cleanup of the existing data.

Check that the data path is safe to use at a more upstream place, a point
where nothing has been created yet.
---
 doc/src/sgml/ref/initdb.sgml        |  7 +++++++
 src/bin/initdb/initdb.c             |  2 ++
 src/fe_utils/string_utils.c         | 25 +++++++++++++++++++++++++
 src/include/fe_utils/string_utils.h |  1 +
 4 files changed, 35 insertions(+)

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 4e339ec..dc01444 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -442,6 +442,13 @@ PostgreSQL documentation
    <command>initdb</command> can also be invoked via
    <command>pg_ctl initdb</command>.
   </para>
+
+  <para>
+   The data directory path 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.
+  </para>
  </refsect1>
 
  <refsect1>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3350e13..a2aec1b 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3499,6 +3499,8 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	checkShellString(pg_data);
+
 	/* If we only need to fsync, just do it and exit */
 	if (sync_only)
 	{
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 1c3d9b3..b8c96a9 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -417,6 +417,31 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
 
 
 /*
+ * Check whether the given string is suited to be used within a shell command.
+ *
+ * The same restrictions as for appendShellString apply.
+ */
+void
+checkShellString(const char *str)
+{
+	if (strchr(str, '\n') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a newline character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+	if (strchr(str, '\r') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a carriage return character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+}
+
+
+/*
  * Append the given string to the shell command being built in the buffer,
  * with shell-style quoting as needed to create exactly one argument.
  *
diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h
index 452ffc0..3af601f 100644
--- a/src/include/fe_utils/string_utils.h
+++ b/src/include/fe_utils/string_utils.h
@@ -43,6 +43,7 @@ extern void appendByteaLiteral(PQExpBuffer buf,
 				   const unsigned char *str, size_t length,
 				   bool std_strings);
 
+extern void checkShellString(const char *str);
 extern void appendShellString(PQExpBuffer buf, const char *str);
 extern void appendConnStrVal(PQExpBuffer buf, const char *str);
 extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
-- 
2.10.0

From 1572145ce8a5846df823d861b662a48c1ac1bb90 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 22 Nov 2016 22:25:17 +0900
Subject: [PATCH 2/2] Ensure clean up of data directory even with restricted
 path applied

Newline and carriage return characters are forbidden by appendShellCommand,
call used when generating the command recommended to the user to launch a
server. However in the case where the data directory contained such characters
initdb did not perform any cleanup of the existing data.

Check that the data path is safe to use at a more upstream place, a point
where nothing has been created yet.
---
 doc/src/sgml/ref/initdb.sgml        |  7 +++++++
 src/bin/initdb/initdb.c             |  2 ++
 src/bin/initdb/t/001_initdb.pl      |  4 +++-
 src/fe_utils/string_utils.c         | 25 +++++++++++++++++++++++++
 src/include/fe_utils/string_utils.h |  1 +
 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 31f081a..5fabfda 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -442,6 +442,13 @@ PostgreSQL documentation
    <command>initdb</command> can also be invoked via
    <command>pg_ctl initdb</command>.
   </para>
+
+  <para>
+   The data directory path 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.
+  </para>
  </refsect1>
 
  <refsect1>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index c8a8c52..6b54a46 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3243,6 +3243,8 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	checkShellString(pg_data);
+
 	/* If we only need to fsync, just do it and exit */
 	if (sync_only)
 	{
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 372865d..307c3a1 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -6,7 +6,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 15;
+use Test::More tests => 16;
 
 my $tempdir = TestLib::tempdir;
 my $xlogdir = "$tempdir/pgxlog";
@@ -39,3 +39,5 @@ command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
 
 command_ok([ 'initdb', '-S', $datadir ], 'sync only');
 command_fails([ 'initdb', $datadir ], 'existing data directory');
+command_fails([ 'initdb', $datadir . "foo\n\rbar" ],
+	'data directory with \n\r');
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 1c3d9b3..b8c96a9 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -417,6 +417,31 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
 
 
 /*
+ * Check whether the given string is suited to be used within a shell command.
+ *
+ * The same restrictions as for appendShellString apply.
+ */
+void
+checkShellString(const char *str)
+{
+	if (strchr(str, '\n') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a newline character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+	if (strchr(str, '\r') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a carriage return character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+}
+
+
+/*
  * Append the given string to the shell command being built in the buffer,
  * with shell-style quoting as needed to create exactly one argument.
  *
diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h
index 452ffc0..3af601f 100644
--- a/src/include/fe_utils/string_utils.h
+++ b/src/include/fe_utils/string_utils.h
@@ -43,6 +43,7 @@ extern void appendByteaLiteral(PQExpBuffer buf,
 				   const unsigned char *str, size_t length,
 				   bool std_strings);
 
+extern void checkShellString(const char *str);
 extern void appendShellString(PQExpBuffer buf, const char *str);
 extern void appendConnStrVal(PQExpBuffer buf, const char *str);
 extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
-- 
2.10.2

From 4b5fa316e9b7bc1b981537d9081874a17109497c Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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 <replaceable class="PARAMETER">name</replaceable>
    connection <quote>slot</> remains for the database, it is possible that
    both will fail.  Also, the limit is not enforced against superusers.
   </para>
+
+  <para>
+    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.
+   </para>
  </refsect1>
 
  <refsect1>
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 <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
    <command>\password</command> that can be used to safely change the
    password later.
   </para>
+
+  <para>
+   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.
+  </para>
  </refsect1>
 
  <refsect1>
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

From 217d934d0c252e312610a21cd75fb01082bb9bad Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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 <replaceable class="PARAMETER">name</replaceable>
    connection <quote>slot</> remains for the database, it is possible that
    both will fail.  Also, the limit is not enforced against superusers.
   </para>
+
+  <para>
+    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.
+   </para>
  </refsect1>
 
  <refsect1>
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 <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
    <command>\password</command> that can be used to safely change the
    password later.
   </para>
+
+  <para>
+   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.
+  </para>
  </refsect1>
 
  <refsect1>
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

From 8268accf95236fd55e02b9168a64b4b472cc9ce7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 22 Nov 2016 22:25:17 +0900
Subject: [PATCH 2/2] Ensure clean up of data directory even with restricted
 path applied

Newline and carriage return characters are forbidden by appendShellCommand,
call used when generating the command recommended to the user to launch a
server. However in the case where the data directory contained such characters
initdb did not perform any cleanup of the existing data.

Check that the data path is safe to use at a more upstream place, a point
where nothing has been created yet.
---
 doc/src/sgml/ref/initdb.sgml        |  7 +++++++
 src/bin/initdb/initdb.c             |  2 ++
 src/bin/initdb/t/001_initdb.pl      |  4 +++-
 src/fe_utils/string_utils.c         | 25 +++++++++++++++++++++++++
 src/include/fe_utils/string_utils.h |  1 +
 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 31f081ae7a..5fabfda23f 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -442,6 +442,13 @@ PostgreSQL documentation
    <command>initdb</command> can also be invoked via
    <command>pg_ctl initdb</command>.
   </para>
+
+  <para>
+   The data directory path 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.
+  </para>
  </refsect1>
 
  <refsect1>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 443c2ee468..7cc430f686 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3103,6 +3103,8 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	checkShellString(pg_data);
+
 	/* If we only need to fsync, just do it and exit */
 	if (sync_only)
 	{
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 372865d3f7..307c3a1b64 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -6,7 +6,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 15;
+use Test::More tests => 16;
 
 my $tempdir = TestLib::tempdir;
 my $xlogdir = "$tempdir/pgxlog";
@@ -39,3 +39,5 @@ command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
 
 command_ok([ 'initdb', '-S', $datadir ], 'sync only');
 command_fails([ 'initdb', $datadir ], 'existing data directory');
+command_fails([ 'initdb', $datadir . "foo\n\rbar" ],
+	'data directory with \n\r');
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 876c877373..0265033f59 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -417,6 +417,31 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
 
 
 /*
+ * Check whether the given string is suited to be used within a shell command.
+ *
+ * The same restrictions as for appendShellString apply.
+ */
+void
+checkShellString(const char *str)
+{
+	if (strchr(str, '\n') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a newline character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+	if (strchr(str, '\r') != NULL)
+	{
+		fprintf(stderr,
+				_("string contains a carriage return character: \"%s\"\n"),
+				str);
+		exit(EXIT_FAILURE);
+	}
+}
+
+
+/*
  * Append the given string to the shell command being built in the buffer,
  * with shell-style quoting as needed to create exactly one argument.
  *
diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h
index 8f96a0b991..6b1ee52eb6 100644
--- a/src/include/fe_utils/string_utils.h
+++ b/src/include/fe_utils/string_utils.h
@@ -43,6 +43,7 @@ extern void appendByteaLiteral(PQExpBuffer buf,
 				   const unsigned char *str, size_t length,
 				   bool std_strings);
 
+extern void checkShellString(const char *str);
 extern void appendShellString(PQExpBuffer buf, const char *str);
 extern void appendConnStrVal(PQExpBuffer buf, const char *str);
 extern void appendPsqlMetaConnect(PQExpBuffer buf, const char *dbname);
-- 
2.11.0