From cad995bc66aef479d7a075fe9ff1af195de04a3c Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 20 Jan 2023 09:52:58 -0500 Subject: [PATCH v1] Add new predefined role pg_create_subscriptions. This role can be granted to non-superusers to allow them to issue CREATE SUBSCRIPTION. If a connection string passed to CREATE SUBSCRIPTION or ALTER SUBSCRIPTION would trigger access to local files (e.g. because it specifies host=/someplace or sslcert=/some/file), then pg_read_server_files is also required. This commit also allows non-superusers to use ALTER SUBSCRIPTION .. SKIP. It is good enough to be the owner of the subscription. XXX. Documentation changes not included. --- src/backend/commands/subscriptioncmds.c | 33 ++++++++----- .../libpqwalreceiver/libpqwalreceiver.c | 46 +++++++++++++++++-- src/include/catalog/pg_authid.dat | 5 ++ src/include/replication/walreceiver.h | 2 +- src/test/regress/expected/subscription.out | 4 +- 5 files changed, 72 insertions(+), 18 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index baff00dd74..258414dc5c 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -23,6 +23,7 @@ #include "catalog/namespace.h" #include "catalog/objectaccess.h" #include "catalog/objectaddress.h" +#include "catalog/pg_authid_d.h" #include "catalog/pg_subscription.h" #include "catalog/pg_subscription_rel.h" #include "catalog/pg_type.h" @@ -572,10 +573,15 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, if (opts.create_slot) PreventInTransactionBlock(isTopLevel, "CREATE SUBSCRIPTION ... WITH (create_slot = true)"); - if (!superuser()) + /* + * We don't want to allow unprivileged users to be able to trigger attempts + * to access arbitrary network destinations, so require the user to have + * been specifically authorized to perform this operation. + */ + if (!has_privs_of_role(owner, ROLE_PG_CREATE_SUBSCRIPTION)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser to create subscriptions"))); + errmsg("must have privileges of pg_create_subscription to create subscriptions"))); /* * If built with appropriate switch, whine when regression-testing @@ -614,7 +620,11 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, load_file("libpqwalreceiver", false); /* Check the connection info string. */ - walrcv_check_conninfo(conninfo); + if (walrcv_check_conninfo(conninfo) && + !has_privs_of_role(owner, ROLE_PG_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must have privileges of pg_read_server_files to specify this connection string"))); /* Everything ok, form a new tuple. */ memset(values, 0, sizeof(values)); @@ -1148,7 +1158,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, /* Load the library providing us libpq calls. */ load_file("libpqwalreceiver", false); /* Check the connection info string. */ - walrcv_check_conninfo(stmt->conninfo); + if (walrcv_check_conninfo(stmt->conninfo) && + !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must have privileges of pg_read_server_files to specify this connection string"))); values[Anum_pg_subscription_subconninfo - 1] = CStringGetTextDatum(stmt->conninfo); @@ -1305,11 +1319,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, /* ALTER SUBSCRIPTION ... SKIP supports only LSN option */ Assert(IsSet(opts.specified_opts, SUBOPT_LSN)); - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser to skip transaction"))); - /* * If the user sets subskiplsn, we do a sanity check to make * sure that the specified LSN is a probable value. @@ -1717,13 +1726,13 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, NameStr(form->subname)); - /* New owner must be a superuser */ - if (!superuser_arg(newOwnerId)) + /* New owner must have pg_create_subscription */ + if (!has_privs_of_role(newOwnerId, ROLE_PG_CREATE_SUBSCRIPTION)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied to change owner of subscription \"%s\"", NameStr(form->subname)), - errhint("The owner of a subscription must be a superuser."))); + errhint("The owner of a subscription must have the privileges of pg_create_subscription."))); form->subowner = newOwnerId; CatalogTupleUpdate(rel, &tup->t_self, tup); diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c index c40c6220db..d70bca87fb 100644 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c @@ -50,7 +50,7 @@ struct WalReceiverConn static WalReceiverConn *libpqrcv_connect(const char *conninfo, bool logical, const char *appname, char **err); -static void libpqrcv_check_conninfo(const char *conninfo); +static bool libpqrcv_check_conninfo(const char *conninfo); static char *libpqrcv_get_conninfo(WalReceiverConn *conn); static void libpqrcv_get_senderinfo(WalReceiverConn *conn, char **sender_host, int *sender_port); @@ -247,13 +247,20 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname, } /* - * Validate connection info string (just try to parse it) + * Validate connection info string, and determine whether it might cause + * local filesystem access to be attempted. + * + * If the connection string can't be parsed, this function will raise + * an error and will not return. If it can, it will return true if local + * filesystem access may be attempted and false otherwise. */ -static void +static bool libpqrcv_check_conninfo(const char *conninfo) { PQconninfoOption *opts = NULL; + PQconninfoOption *opt; char *err = NULL; + bool result = false; opts = PQconninfoParse(conninfo, &err); if (opts == NULL) @@ -267,7 +274,40 @@ libpqrcv_check_conninfo(const char *conninfo) errmsg("invalid connection string syntax: %s", errcopy))); } + for (opt = opts; opt->keyword != NULL; ++opt) + { + /* Ignore connection options that are not present. */ + if (opt->val == NULL) + continue; + + /* For all these parameters, the value is a local filename. */ + if (strcmp(opt->keyword, "passfile") == 0 || + strcmp(opt->keyword, "sslcert") == 0 || + strcmp(opt->keyword, "sslkey") == 0 || + strcmp(opt->keyword, "sslrootcert") == 0 || + strcmp(opt->keyword, "sslcrl") == 0 || + strcmp(opt->keyword, "sslcrldir") == 0 || + strcmp(opt->keyword, "service") == 0) + { + result = true; + break; + } + + /* + * For the host parameter, the value might be a local filename. + * It might also be a reference to the local host's abstract UNIX + * socket namespace, which we consider equivalent to a local pathname + * for security purporses. + */ + if (strcmp(opt->keyword, "host") == 0 && is_unixsock_path(opt->val)) + { + result = true; + break; + } + } + PQconninfoFree(opts); + return result; } /* diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index 2a2fee7d28..7daad12e96 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -89,5 +89,10 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '9535', oid_symbol => 'ROLE_PG_CREATE_SUBSCRIPTION', + rolname => 'pg_create_subscription', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h index decffe352d..1227650a85 100644 --- a/src/include/replication/walreceiver.h +++ b/src/include/replication/walreceiver.h @@ -247,7 +247,7 @@ typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo, * * Parse and validate the connection string given as of 'conninfo'. */ -typedef void (*walrcv_check_conninfo_fn) (const char *conninfo); +typedef bool (*walrcv_check_conninfo_fn) (const char *conninfo); /* * walrcv_get_conninfo_fn diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 4e5cb0d3a9..7843867695 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -78,7 +78,7 @@ ERROR: subscription "regress_testsub" already exists -- fail - must be superuser SET SESSION AUTHORIZATION 'regress_subscription_user2'; CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION foo WITH (connect = false); -ERROR: must be superuser to create subscriptions +ERROR: must have privileges of pg_create_subscription to create subscriptions SET SESSION AUTHORIZATION 'regress_subscription_user'; -- fail - invalid option combinations CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true); @@ -213,7 +213,7 @@ ALTER SUBSCRIPTION regress_testsub_foo RENAME TO regress_testsub; -- fail - new owner must be superuser ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2; ERROR: permission denied to change owner of subscription "regress_testsub" -HINT: The owner of a subscription must be a superuser. +HINT: The owner of a subscription must have the privileges of pg_create_subscription. ALTER ROLE regress_subscription_user2 SUPERUSER; -- now it works ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2; -- 2.37.1 (Apple Git-137.1)