Obstruct shell, SQL, and conninfo injection via database and role names.

Due to simplistic quoting and confusion of database names with conninfo
strings, roles with the CREATEDB or CREATEROLE option could escalate to
superuser privileges when a superuser next ran certain maintenance
commands.  The new coding rule for PQconnectdbParams() calls, documented
at conninfo_array_parse(), is to pass expand_dbname=true and wrap
literal database names in a trivial connection string.  Escape
zero-length values in appendConnStrVal().  Back-patch to 9.1 (all
supported versions).

Nathan Bossart, Michael Paquier, and Noah Misch.  Reviewed by Peter
Eisentraut.  Reported by Nathan Bossart.

Security: CVE-2016-5424

1 files changed, 32 insertions, 19 deletions

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 02b736dbd0b..830335f5019 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -9,6 +9,7 @@
 
 #include "postgres_fe.h"
 
+#include "fe_utils/string_utils.h"
 #include "pg_upgrade.h"
 
 
@@ -51,18 +52,25 @@ connectToServer(ClusterInfo *cluster, const char *db_name)
 static PGconn *
 get_db_conn(ClusterInfo *cluster, const char *db_name)
 {
-	char		conn_opts[2 * NAMEDATALEN + MAXPGPATH + 100];
+	PQExpBufferData conn_opts;
+	PGconn	   *conn;
 
+	/* Build connection string with proper quoting */
+	initPQExpBuffer(&conn_opts);
+	appendPQExpBufferStr(&conn_opts, "dbname=");
+	appendConnStrVal(&conn_opts, db_name);
+	appendPQExpBufferStr(&conn_opts, " user=");
+	appendConnStrVal(&conn_opts, os_info.user);
+	appendPQExpBuffer(&conn_opts, " port=%d", cluster->port);
 	if (cluster->sockdir)
-		snprintf(conn_opts, sizeof(conn_opts),
-				 "dbname = '%s' user = '%s' host = '%s' port = %d",
-				 db_name, os_info.user, cluster->sockdir, cluster->port);
-	else
-		snprintf(conn_opts, sizeof(conn_opts),
-				 "dbname = '%s' user = '%s' port = %d",
-				 db_name, os_info.user, cluster->port);
+	{
+		appendPQExpBufferStr(&conn_opts, " host=");
+		appendConnStrVal(&conn_opts, cluster->sockdir);
+	}
 
-	return PQconnectdb(conn_opts);
+	conn = PQconnectdb(conn_opts.data);
+	termPQExpBuffer(&conn_opts);
+	return conn;
 }
 
 
@@ -74,23 +82,28 @@ get_db_conn(ClusterInfo *cluster, const char *db_name)
  * sets, but the utilities we need aren't very consistent about the treatment
  * of database name options, so we leave that out.
  *
- * Note result is in static storage, so use it right away.
+ * Result is valid until the next call to this function.
  */
 char *
 cluster_conn_opts(ClusterInfo *cluster)
 {
-	static char conn_opts[MAXPGPATH + NAMEDATALEN + 100];
+	static PQExpBuffer buf;
 
-	if (cluster->sockdir)
-		snprintf(conn_opts, sizeof(conn_opts),
-				 "--host \"%s\" --port %d --username \"%s\"",
-				 cluster->sockdir, cluster->port, os_info.user);
+	if (buf == NULL)
+		buf = createPQExpBuffer();
 	else
-		snprintf(conn_opts, sizeof(conn_opts),
-				 "--port %d --username \"%s\"",
-				 cluster->port, os_info.user);
+		resetPQExpBuffer(buf);
+
+	if (cluster->sockdir)
+	{
+		appendPQExpBufferStr(buf, "--host ");
+		appendShellString(buf, cluster->sockdir);
+		appendPQExpBufferChar(buf, ' ');
+	}
+	appendPQExpBuffer(buf, "--port %d --username ", cluster->port);
+	appendShellString(buf, os_info.user);
 
-	return conn_opts;
+	return buf->data;
 }