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 <[email protected]>
Reviewed-by: Ayush Tiwari <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 14
Branch
------
REL_16_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/26d6c19d0f44355988176e3b9cb4342db0df87da
Modified Files
--------------
src/backend/commands/subscriptioncmds.c | 30 +++++++-
.../libpqwalreceiver/libpqwalreceiver.c | 83 ++++++++++++----------
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, 156 insertions(+), 71 deletions(-)
