Clean up quoting of variable strings within replication commands.

Our handling of quoting within replication commands was pretty
sloppy, typically looking like
        appendStringInfo(&cmd, " SLOT \"%s\"", options->slotname);
This is fine as long as options->slotname doesn't contain a double
quote mark, but what if it does?  In principle this'd allow injection
of harmful options into replication commands, in the probably-unlikely
case that a slot name comes from untrustworthy input.  We ought to
clean that up.

Moreover, even the places that were trying to be more careful
generally got it wrong, because they used quoting subroutines
intended for SQL commands rather than something that will work
with the replication-command scanner repl_scanner.l.  For example,
several places naively use PQescapeLiteral() to quote option values
for replication commands.  If the string contains a backslash,
PQescapeLiteral() will produce E'...' literal syntax, which
repl_scanner.l doesn't recognize.  Another near miss was to use
quote_identifier() to quote identifiers.  That function won't quote
valid lowercase identifiers unless they match SQL keywords ... but in
this context, replication keywords are what matter.  Neither of these
errors seem to risk string injection, but they definitely can cause
syntax errors in replication commands that ought to be valid.

We can clean all this up by using simple quoting logic that just
doubles single or double quotes respectively.

Or at least, we could if repl_scanner.l handled doubled double quotes
in identifiers, but for some reason it doesn't!  So the first step in
this fix has to be to fix that.  (The fact that we'll later reject
slot names containing double quotes is very far short of justifying
this omission.)

Having done that, this patch runs around and applies correct
quoting in all places that generate replication commands containing
strings coming from outside the immediate context.  Probably some
of these places are safe because of restrictions elsewhere, but it
seems best to just quote all the time.

This was originally reported as a security bug, which it could be
if replication slot names or parameters were to originate from
untrustworthy sources.  But the security team concluded that that
was a very improbable situation, so we're just going to fix this
as a regular bug.

Reported-by: Team Dhiutsa
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/1648659.1781287310@sss.pgh.pa.us
Backpatch-through: 14

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bbfd860c4011421ae995236999c48eaf16b10f55..a0d310c9355b230055641b0c7f732867700eb7c7 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -320,6 +320,32 @@ publicationListToArray(List *publist)
        return PointerGetDatum(arr);
 }
 
+/*
+ * Append a suitably-quoted identifier or string literal to buf.
+ * "quote" should be either a double-quote or single-quote character.
+ *
+ * Caution: this quoting logic is sufficient for identifiers and literals
+ * in the replication grammar, but not always in regular SQL.  Specifically,
+ * it'd fail for a string literal if standard_conforming_strings is off.
+ */
+static void
+appendQuotedString(StringInfo buf, const char *str, char quote)
+{
+       appendStringInfoChar(buf, quote);
+       while (*str)
+       {
+               char            c = *str++;
+
+               if (c == quote)
+                       appendStringInfoChar(buf, c);
+               appendStringInfoChar(buf, c);
+       }
+       appendStringInfoChar(buf, quote);
+}
+
+#define appendQuotedIdentifier(b, s)   appendQuotedString(b, s, '"')
+#define appendQuotedLiteral(b, s)              appendQuotedString(b, s, '\'')
+
 /*
  * Create new subscription.
  */
@@ -1331,7 +1357,9 @@ ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname, bool missi
        load_file("libpqwalreceiver", false);
 
        initStringInfo(&cmd);
-       appendStringInfo(&cmd, "DROP_REPLICATION_SLOT %s WAIT", quote_identifier(slotname));
+       appendStringInfoString(&cmd, "DROP_REPLICATION_SLOT ");
+       appendQuotedIdentifier(&cmd, slotname);
+       appendStringInfoString(&cmd, " WAIT");
 
        PG_TRY();
        {

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 99c5f9f878453cbd2cdffb7fef9b04d0198658c1..98a786348b4cf363a3580c9a999ac2aaae3f3132 100644 (file)
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -103,7 +103,7 @@ static WalReceiverFunctionsType PQWalReceiverFunctions = {
 /* Prototypes for private functions */
 static PGresult *libpqrcv_PQexec(PGconn *streamConn, const char *query);
 static PGresult *libpqrcv_PQgetResult(PGconn *streamConn);
-static char *stringlist_to_identifierstr(PGconn *conn, List *strings);
+static char *stringlist_to_identifierstr(List *strings);
 
 /*
  * Module initialization function
@@ -392,6 +392,32 @@ libpqrcv_server_version(WalReceiverConn *conn)
        return PQserverVersion(conn->streamConn);
 }
 
+/*
+ * Append a suitably-quoted identifier or string literal to buf.
+ * "quote" should be either a double-quote or single-quote character.
+ *
+ * Caution: this quoting logic is sufficient for identifiers and literals
+ * in the replication grammar, but not always in regular SQL.  Specifically,
+ * it'd fail for a string literal if standard_conforming_strings is off.
+ */
+static void
+appendQuotedString(StringInfo buf, const char *str, char quote)
+{
+       appendStringInfoChar(buf, quote);
+       while (*str)
+       {
+               char            c = *str++;
+
+               if (c == quote)
+                       appendStringInfoChar(buf, c);
+               appendStringInfoChar(buf, c);
+       }
+       appendStringInfoChar(buf, quote);
+}
+
+#define appendQuotedIdentifier(b, s)   appendQuotedString(b, s, '"')
+#define appendQuotedLiteral(b, s)              appendQuotedString(b, s, '\'')
+
 /*
  * Start streaming WAL data from given streaming options.
  *
@@ -417,8 +443,10 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
        /* Build the command. */
        appendStringInfoString(&cmd, "START_REPLICATION");
        if (options->slotname != NULL)
-               appendStringInfo(&cmd, " SLOT \"%s\"",
-                                                options->slotname);
+       {
+               appendStringInfoString(&cmd, " SLOT ");
+               appendQuotedIdentifier(&cmd, options->slotname);
+       }
 
        if (options->logical)
                appendStringInfoString(&cmd, " LOGICAL");
@@ -433,7 +461,6 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
        {
                char       *pubnames_str;
                List       *pubnames;
-               char       *pubnames_literal;
 
                appendStringInfoString(&cmd, " (");
 
@@ -445,21 +472,9 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
                        appendStringInfoString(&cmd, ", streaming 'on'");
 
                pubnames = options->proto.logical.publication_names;
-               pubnames_str = stringlist_to_identifierstr(conn->streamConn, pubnames);
-               if (!pubnames_str)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_OUT_OF_MEMORY),        /* likely guess */
-                                        errmsg("could not start WAL streaming: %s",
-                                                       pchomp(PQerrorMessage(conn->streamConn)))));
-               pubnames_literal = PQescapeLiteral(conn->streamConn, pubnames_str,
-                                                                                  strlen(pubnames_str));
-               if (!pubnames_literal)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_OUT_OF_MEMORY),        /* likely guess */
-                                        errmsg("could not start WAL streaming: %s",
-                                                       pchomp(PQerrorMessage(conn->streamConn)))));
-               appendStringInfo(&cmd, ", publication_names %s", pubnames_literal);
-               PQfreemem(pubnames_literal);
+               pubnames_str = stringlist_to_identifierstr(pubnames);
+               appendStringInfoString(&cmd, ", publication_names ");
+               appendQuotedLiteral(&cmd, pubnames_str);
                pfree(pubnames_str);
 
                if (options->proto.logical.binary &&
@@ -865,7 +880,8 @@ libpqrcv_create_slot(WalReceiverConn *conn, const char *slotname,
 
        initStringInfo(&cmd);
 
-       appendStringInfo(&cmd, "CREATE_REPLICATION_SLOT \"%s\"", slotname);
+       appendStringInfoString(&cmd, "CREATE_REPLICATION_SLOT ");
+       appendQuotedIdentifier(&cmd, slotname);
 
        if (temporary)
                appendStringInfoString(&cmd, " TEMPORARY");
@@ -1082,10 +1098,10 @@ libpqrcv_exec(WalReceiverConn *conn, const char *query,
  *
  * This is essentially the reverse of SplitIdentifierString.
  *
- * The caller should free the result.
+ * The caller should pfree the result.
  */
 static char *
-stringlist_to_identifierstr(PGconn *conn, List *strings)
+stringlist_to_identifierstr(List *strings)
 {
        ListCell   *lc;
        StringInfoData res;
@@ -1096,21 +1112,12 @@ stringlist_to_identifierstr(PGconn *conn, List *strings)
        foreach(lc, strings)
        {
                char       *val = strVal(lfirst(lc));
-               char       *val_escaped;
 
                if (first)
                        first = false;
                else
                        appendStringInfoChar(&res, ',');
-
-               val_escaped = PQescapeIdentifier(conn, val, strlen(val));
-               if (!val_escaped)
-               {
-                       free(res.data);
-                       return NULL;
-               }
-               appendStringInfoString(&res, val_escaped);
-               PQfreemem(val_escaped);
+               appendQuotedIdentifier(&res, val);
        }
 
        return res.data;

diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 8a075e2d926099a89b4395dba10f06f4cb70e347..ee6cb7b237ab440d440ae19e3c9ec674cde62e47 100644 (file)
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -186,6 +186,10 @@ MANIFEST_CHECKSUMS { return K_MANIFEST_CHECKSUMS; }
                                        return IDENT;
                                }
 
+<xd>{xddouble} {
+                                       addlitchar('"');
+                               }
+
 <xd>{xdinside}  {
                                        addlit(yytext, yyleng);
                                }

diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 4eec0d8eee6ce8d44846d4e3e5fbd6a36106618b..24529ae36a9f0fba4fcb328867c44d80e5e4f259 100644 (file)
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -231,8 +231,9 @@ StreamLogicalLog(void)
 
        /* Initiate the replication stream at specified location */
        query = createPQExpBuffer();
-       appendPQExpBuffer(query, "START_REPLICATION SLOT \"%s\" LOGICAL %X/%X",
-                                         replication_slot, LSN_FORMAT_ARGS(startpos));
+       appendPQExpBufferStr(query, "START_REPLICATION SLOT ");
+       AppendQuotedIdentifier(query, replication_slot);
+       appendPQExpBuffer(query, " LOGICAL %X/%X", LSN_FORMAT_ARGS(startpos));
 
        /* print options if there are any */
        if (noptions)
@@ -245,11 +246,14 @@ StreamLogicalLog(void)
                        appendPQExpBufferStr(query, ", ");
 
                /* write option name */
-               appendPQExpBuffer(query, "\"%s\"", options[(i * 2)]);
+               AppendQuotedIdentifier(query, options[i * 2]);
 
                /* write option value if specified */
-               if (options[(i * 2) + 1] != NULL)
-                       appendPQExpBuffer(query, " '%s'", options[(i * 2) + 1]);
+               if (options[i * 2 + 1] != NULL)
+               {
+                       appendPQExpBufferChar(query, ' ');
+                       AppendQuotedLiteral(query, options[i * 2 + 1]);
+               }
        }
 
        if (noptions)

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 73620e0daf33c1b429ce308a8629978cd71b4748..e89903c790073534e855765c6b21945226c8758e 100644 (file)
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -448,8 +448,7 @@ CheckServerVersionForStreaming(PGconn *conn)
 bool
 ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
 {
-       char            query[128];
-       char            slotcmd[128];
+       PQExpBuffer query;
        PGresult   *res;
        XLogRecPtr      stoppos;
 
@@ -474,7 +473,6 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
        if (stream->replication_slot != NULL)
        {
                reportFlushPosition = true;
-               sprintf(slotcmd, "SLOT \"%s\" ", stream->replication_slot);
        }
        else
        {
@@ -482,7 +480,6 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
                        reportFlushPosition = true;
                else
                        reportFlushPosition = false;
-               slotcmd[0] = 0;
        }
 
        if (stream->sysidentifier != NULL)
@@ -535,8 +532,10 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
                 */
                if (!existsTimeLineHistoryFile(stream))
                {
-                       snprintf(query, sizeof(query), "TIMELINE_HISTORY %u", stream->timeline);
-                       res = PQexec(conn, query);
+                       query = createPQExpBuffer();
+                       appendPQExpBuffer(query, "TIMELINE_HISTORY %u", stream->timeline);
+                       res = PQexec(conn, query->data);
+                       destroyPQExpBuffer(query);
                        if (PQresultStatus(res) != PGRES_TUPLES_OK)
                        {
                                /* FIXME: we might send it ok, but get an error */
@@ -572,11 +571,18 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream)
                        return true;
 
                /* Initiate the replication stream at specified location */
-               snprintf(query, sizeof(query), "START_REPLICATION %s%X/%X TIMELINE %u",
-                                slotcmd,
-                                LSN_FORMAT_ARGS(stream->startpos),
-                                stream->timeline);
-               res = PQexec(conn, query);
+               query = createPQExpBuffer();
+               appendPQExpBufferStr(query, "START_REPLICATION");
+               if (stream->replication_slot != NULL)
+               {
+                       appendPQExpBufferStr(query, " SLOT ");
+                       AppendQuotedIdentifier(query, stream->replication_slot);
+               }
+               appendPQExpBuffer(query, " %X/%X TIMELINE %u",
+                                                 LSN_FORMAT_ARGS(stream->startpos),
+                                                 stream->timeline);
+               res = PQexec(conn, query->data);
+               destroyPQExpBuffer(query);
                if (PQresultStatus(res) != PGRES_COPY_BOTH)
                {
                        pg_log_error("could not send replication command \"%s\": %s",

diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index f8764a853b685462bb1f6ce78700b9eaf4a1cb80..4f7d89704f163c7175d8dd9dac0b8a59f5118812 100644 (file)
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -498,7 +498,8 @@ CreateReplicationSlot(PGconn *conn, const char *slot_name, const char *plugin,
        Assert(slot_name != NULL);
 
        /* Build query */
-       appendPQExpBuffer(query, "CREATE_REPLICATION_SLOT \"%s\"", slot_name);
+       appendPQExpBufferStr(query, "CREATE_REPLICATION_SLOT ");
+       AppendQuotedIdentifier(query, slot_name);
        if (is_temporary)
                appendPQExpBufferStr(query, " TEMPORARY");
        if (is_physical)
@@ -509,7 +510,8 @@ CreateReplicationSlot(PGconn *conn, const char *slot_name, const char *plugin,
        }
        else
        {
-               appendPQExpBuffer(query, " LOGICAL \"%s\"", plugin);
+               appendPQExpBufferStr(query, " LOGICAL ");
+               AppendQuotedIdentifier(query, plugin);
                if (PQserverVersion(conn) >= 100000)
                        /* pg_recvlogical doesn't use an exported snapshot, so suppress */
                        appendPQExpBufferStr(query, " NOEXPORT_SNAPSHOT");
@@ -570,8 +572,8 @@ DropReplicationSlot(PGconn *conn, const char *slot_name)
        query = createPQExpBuffer();
 
        /* Build query */
-       appendPQExpBuffer(query, "DROP_REPLICATION_SLOT \"%s\"",
-                                         slot_name);
+       appendPQExpBufferStr(query, "DROP_REPLICATION_SLOT ");
+       AppendQuotedIdentifier(query, slot_name);
        res = PQexec(conn, query->data);
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
        {
@@ -600,6 +602,29 @@ DropReplicationSlot(PGconn *conn, const char *slot_name)
 }
 
 
+/*
+ * Append a suitably-quoted identifier or string literal to buf.
+ * "quote" should be either a double-quote or single-quote character.
+ *
+ * Caution: this quoting logic is sufficient for identifiers and literals
+ * in the replication grammar, but not always in regular SQL.  Specifically,
+ * it'd fail for a string literal if standard_conforming_strings is off.
+ */
+void
+AppendQuotedString(PQExpBuffer buf, const char *str, char quote)
+{
+       appendPQExpBufferChar(buf, quote);
+       while (*str)
+       {
+               char            c = *str++;
+
+               if (c == quote)
+                       appendPQExpBufferChar(buf, c);
+               appendPQExpBufferChar(buf, c);
+       }
+       appendPQExpBufferChar(buf, quote);
+}
+
 /*
  * Frontend version of GetCurrentTimestamp(), since we are not linked with
  * backend code.

diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index 10f87ad0c14b31987176a20486e5ab1c29427b4a..a5ae1b91d64f6596ae9e3e6162c0f924c58438d8 100644 (file)
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -15,6 +15,7 @@
 #include "access/xlogdefs.h"
 #include "datatype/timestamp.h"
 #include "libpq-fe.h"
+#include "pqexpbuffer.h"
 
 extern const char *progname;
 extern char *connection_string;
@@ -40,6 +41,11 @@ extern bool RunIdentifySystem(PGconn *conn, char **sysid,
                                                          TimeLineID *starttli,
                                                          XLogRecPtr *startpos,
                                                          char **db_name);
+
+extern void AppendQuotedString(PQExpBuffer buf, const char *str, char quote);
+#define AppendQuotedIdentifier(b, s)   AppendQuotedString(b, s, '"')
+#define AppendQuotedLiteral(b, s)              AppendQuotedString(b, s, '\'')
+
 extern bool RetrieveWalSegSize(PGconn *conn);
 extern TimestampTz feGetCurrentTimestamp(void);
 extern void feTimestampDifference(TimestampTz start_time, TimestampTz stop_time,