From 2a00840e8c9772b5b22accd652a495d7c654693e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 15 Jun 2026 15:35:37 -0400 Subject: [PATCH] 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 Reviewed-by: Ayush Tiwari Discussion: https://postgr.es/m/1648659.1781287310@sss.pgh.pa.us Backpatch-through: 14 --- src/backend/commands/subscriptioncmds.c | 30 +++++++- .../libpqwalreceiver/libpqwalreceiver.c | 71 ++++++++++--------- src/backend/replication/repl_scanner.l | 4 ++ src/bin/pg_basebackup/pg_recvlogical.c | 14 ++-- src/bin/pg_basebackup/receivelog.c | 28 +++++--- src/bin/pg_basebackup/streamutil.c | 33 +++++++-- src/bin/pg_basebackup/streamutil.h | 6 ++ 7 files changed, 133 insertions(+), 53 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index bbfd860c401..a0d310c9355 100644 --- 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 99c5f9f8784..98a786348b4 100644 --- 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 8a075e2d926..ee6cb7b237a 100644 --- 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; } +{xddouble} { + addlitchar('"'); + } + {xdinside} { addlit(yytext, yyleng); } diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 4eec0d8eee6..24529ae36a9 100644 --- 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 73620e0daf3..e89903c7900 100644 --- 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 f8764a853b6..4f7d89704f1 100644 --- 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 10f87ad0c14..a5ae1b91d64 100644 --- 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,