From 49c811ccb70805ce0c92707e770682e1ff76e934 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 9 Jun 2026 18:07:46 -0400 Subject: [PATCH v1] 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 Backpatch-through: 14 --- src/backend/commands/subscriptioncmds.c | 30 ++++++- .../libpqwalreceiver/libpqwalreceiver.c | 88 +++++++++++-------- 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 | 57 ++++++++---- src/bin/pg_basebackup/streamutil.h | 11 ++- 7 files changed, 159 insertions(+), 73 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index fd026b304c2..4f466d85199 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -513,6 +513,32 @@ parse_subscription_options(ParseState *pstate, List *stmt_options, } } +/* + * 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, '\'') + /* * Check that the specified publications are present on the publisher. */ @@ -2513,7 +2539,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 ebfd64bdf05..5376519fea5 100644 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c @@ -116,7 +116,7 @@ static WalReceiverFunctionsType PQWalReceiverFunctions = { }; /* Prototypes for private functions */ -static char *stringlist_to_identifierstr(PGconn *conn, List *strings); +static char *stringlist_to_identifierstr(List *strings); /* * Module initialization function @@ -522,6 +522,32 @@ libpqrcv_get_option_from_conninfo(const char *connInfo, const char *keyword) return option; } +/* + * 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. * @@ -547,8 +573,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"); @@ -563,7 +591,6 @@ libpqrcv_startstreaming(WalReceiverConn *conn, { char *pubnames_str; List *pubnames; - char *pubnames_literal; appendStringInfoString(&cmd, " ("); @@ -571,8 +598,10 @@ libpqrcv_startstreaming(WalReceiverConn *conn, options->proto.logical.proto_version); if (options->proto.logical.streaming_str) - appendStringInfo(&cmd, ", streaming '%s'", - options->proto.logical.streaming_str); + { + appendStringInfoString(&cmd, ", streaming "); + appendQuotedLiteral(&cmd, options->proto.logical.streaming_str); + } if (options->proto.logical.twophase && PQserverVersion(conn->streamConn) >= 150000) @@ -580,25 +609,15 @@ libpqrcv_startstreaming(WalReceiverConn *conn, if (options->proto.logical.origin && PQserverVersion(conn->streamConn) >= 160000) - appendStringInfo(&cmd, ", origin '%s'", - options->proto.logical.origin); + { + appendStringInfoString(&cmd, ", origin "); + appendQuotedLiteral(&cmd, options->proto.logical.origin); + } 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 && @@ -899,7 +918,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"); @@ -1005,8 +1025,9 @@ libpqrcv_alter_slot(WalReceiverConn *conn, const char *slotname, PGresult *res; initStringInfo(&cmd); - appendStringInfo(&cmd, "ALTER_REPLICATION_SLOT %s ( ", - quote_identifier(slotname)); + appendStringInfoString(&cmd, "ALTER_REPLICATION_SLOT "); + appendQuotedIdentifier(&cmd, slotname); + appendStringInfoString(&cmd, " ( "); if (failover) appendStringInfo(&cmd, "FAILOVER %s", @@ -1203,10 +1224,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; @@ -1217,21 +1238,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 fcdeca04bf9..70b137acedc 100644 --- a/src/backend/replication/repl_scanner.l +++ b/src/backend/replication/repl_scanner.l @@ -197,6 +197,10 @@ UPLOAD_MANIFEST { return K_UPLOAD_MANIFEST; } return IDENT; } +{xddouble} { + addlitchar('"', yyscanner); + } + {xdinside} { addlit(yytext, yyleng, yyscanner); } diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 2fdf64bcadb..0f7d7fe9429 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -248,8 +248,9 @@ StreamLogicalLog(void) /* Initiate the replication stream at specified location */ query = createPQExpBuffer(); - appendPQExpBuffer(query, "START_REPLICATION SLOT \"%s\" LOGICAL %X/%08X", - replication_slot, LSN_FORMAT_ARGS(startpos)); + appendPQExpBufferStr(query, "START_REPLICATION SLOT "); + AppendQuotedIdentifier(query, replication_slot); + appendPQExpBuffer(query, " LOGICAL %X/%08X", LSN_FORMAT_ARGS(startpos)); /* print options if there are any */ if (noptions) @@ -262,11 +263,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 5ce8f2ba287..77894b721e1 100644 --- a/src/bin/pg_basebackup/receivelog.c +++ b/src/bin/pg_basebackup/receivelog.c @@ -452,8 +452,7 @@ CheckServerVersionForStreaming(PGconn *conn) bool ReceiveXlogStream(PGconn *conn, StreamCtl *stream) { - char query[128]; - char slotcmd[128]; + PQExpBuffer query; PGresult *res; XLogRecPtr stoppos; @@ -478,7 +477,6 @@ ReceiveXlogStream(PGconn *conn, StreamCtl *stream) if (stream->replication_slot != NULL) { reportFlushPosition = true; - sprintf(slotcmd, "SLOT \"%s\" ", stream->replication_slot); } else { @@ -486,7 +484,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/%08X 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/%08X 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 76abdfa2ae6..694326964e3 100644 --- a/src/bin/pg_basebackup/streamutil.c +++ b/src/bin/pg_basebackup/streamutil.c @@ -501,7 +501,8 @@ GetSlotInformation(PGconn *conn, const char *slot_name, *restart_tli = tli_loc; query = createPQExpBuffer(); - appendPQExpBuffer(query, "READ_REPLICATION_SLOT %s", slot_name); + appendPQExpBufferStr(query, "READ_REPLICATION_SLOT "); + AppendQuotedIdentifier(query, slot_name); res = PQexec(conn, query->data); destroyPQExpBuffer(query); @@ -598,13 +599,17 @@ CreateReplicationSlot(PGconn *conn, const char *slot_name, const char *plugin, Assert(slot_name != NULL); /* Build base portion of 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) appendPQExpBufferStr(query, " PHYSICAL"); else - appendPQExpBuffer(query, " LOGICAL \"%s\"", plugin); + { + appendPQExpBufferStr(query, " LOGICAL "); + AppendQuotedIdentifier(query, plugin); + } /* Add any requested options */ if (use_new_option_syntax) @@ -704,8 +709,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) { @@ -733,6 +738,29 @@ DropReplicationSlot(PGconn *conn, const char *slot_name) return true; } +/* + * 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); +} + /* * Append a "plain" option - one with no value - to a server command that * is being constructed. @@ -741,10 +769,13 @@ DropReplicationSlot(PGconn *conn, const char *slot_name) * write things like SOME_COMMAND OPTION1 OPTION2 'opt2value' OPTION3 42. The * new syntax uses a comma-separated list surrounded by parentheses, so the * equivalent is SOME_COMMAND (OPTION1, OPTION2 'optvalue', OPTION3 42). + * + * Note: we assume option names do not require quotes. Do not use this + * with option names coming from outside sources. */ void AppendPlainCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name) + const char *option_name) { if (buf->len > 0 && buf->data[buf->len - 1] != '(') { @@ -765,30 +796,26 @@ AppendPlainCommandOption(PQExpBuffer buf, bool use_new_option_syntax, */ void AppendStringCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name, char *option_value) + const char *option_name, const char *option_value) { AppendPlainCommandOption(buf, use_new_option_syntax, option_name); if (option_value != NULL) { - size_t length = strlen(option_value); - char *escaped_value = palloc(1 + 2 * length); - - PQescapeStringConn(conn, escaped_value, option_value, length, NULL); - appendPQExpBuffer(buf, " '%s'", escaped_value); - pfree(escaped_value); + appendPQExpBufferChar(buf, ' '); + AppendQuotedLiteral(buf, option_value); } } /* - * Append an option with an associated integer value to a server command + * Append an option with an associated integer value to a server command that * is being constructed. * * See comments for AppendPlainCommandOption, above. */ void AppendIntegerCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name, int32 option_value) + const char *option_name, int32 option_value) { AppendPlainCommandOption(buf, use_new_option_syntax, option_name); diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h index 15afef3a9c8..115703e9c6c 100644 --- a/src/bin/pg_basebackup/streamutil.h +++ b/src/bin/pg_basebackup/streamutil.h @@ -43,15 +43,20 @@ extern bool RunIdentifySystem(PGconn *conn, char **sysid, 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 void AppendPlainCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name); + const char *option_name); extern void AppendStringCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name, char *option_value); + const char *option_name, + const char *option_value); extern void AppendIntegerCommandOption(PQExpBuffer buf, bool use_new_option_syntax, - char *option_name, int32 option_value); + const char *option_name, + int32 option_value); extern bool GetSlotInformation(PGconn *conn, const char *slot_name, XLogRecPtr *restart_lsn, -- 2.52.0