From 6225780bd7e4282ec47e153c19ef59fc10648c17 Mon Sep 17 00:00:00 2001 From: Mark Dilger Date: Sun, 26 Sep 2021 18:21:26 -0700 Subject: [PATCH v2 1/3] Handle non-superuser subscription owners sensibly Stop pretending that subscriptions are always owned by superusers and instead fix security problems that arise as a consequence of them not being superuser. Specifically, disallow a non-superuser changing the connection, the list of publications, or the options for their subscription. The prior behavior violated the principle of least surprise, allowing a non-superuser in possession of a subscription to change all aspects of the subscription, connecting it to a different server and subscribing a different set of publications, effectively amounting to a non-superuser creating a new subscription. The new behavior restricts the non-superuser owner to enabling, disabling, refreshing, renaming, and further assigning ownership of the subscription. --- doc/src/sgml/ref/alter_subscription.sgml | 11 ++++- src/backend/commands/subscriptioncmds.c | 20 +++++++++ src/test/regress/expected/subscription.out | 51 ++++++++++++++++++++++ src/test/regress/sql/subscription.sql | 35 +++++++++++++++ 4 files changed, 115 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index 0b027cc346..b1fd73f776 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -47,8 +47,6 @@ ALTER SUBSCRIPTION name RENAME TO < You must own the subscription to use ALTER SUBSCRIPTION. To alter the owner, you must also be a direct or indirect member of the new owning role. The new owner has to be a superuser. - (Currently, all subscription owners must be superusers, so the owner checks - will be bypassed in practice. But this might change in the future.) @@ -99,6 +97,9 @@ ALTER SUBSCRIPTION name RENAME TO < . See there for more information. + + Only superusers may change the connection property. + @@ -139,6 +140,9 @@ ALTER SUBSCRIPTION name RENAME TO < REFRESH PUBLICATION may be specified, to control the implicit refresh operation. + + Only superusers may change the list of subscribed publications. + @@ -204,6 +208,9 @@ ALTER SUBSCRIPTION name RENAME TO < binary, and streaming. + + Only superusers may alter subscription parameters. + diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index c47ba26369..6a5d192128 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -868,6 +868,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, { case ALTER_SUBSCRIPTION_OPTIONS: { + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to alter the options for a subscription"))); + supported_opts = (SUBOPT_SLOT_NAME | SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY | SUBOPT_STREAMING); @@ -946,6 +951,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, } case ALTER_SUBSCRIPTION_CONNECTION: + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to alter the connection for a subscription"))); + /* Load the library providing us libpq calls. */ load_file("libpqwalreceiver", false); /* Check the connection info string. */ @@ -959,6 +969,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, case ALTER_SUBSCRIPTION_SET_PUBLICATION: { + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to alter publications for a subscription"))); + supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH; parse_subscription_options(pstate, stmt->options, supported_opts, &opts); @@ -1006,6 +1021,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, List *publist; bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION; + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to alter publications for a subscription"))); + supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA; parse_subscription_options(pstate, stmt->options, supported_opts, &opts); diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 15a1ac6398..a05e6f1795 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -144,6 +144,57 @@ HINT: The owner of a subscription must be a superuser. ALTER ROLE regress_subscription_user2 SUPERUSER; -- now it works ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2; +-- revoke superuser from new owner +ALTER ROLE regress_subscription_user2 NOSUPERUSER; +SET SESSION AUTHORIZATION regress_subscription_user2; +-- fail - non-superuser owner cannot change connection parameter +ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=somethingelse'; +ERROR: must be superuser to alter the connection for a subscription +-- fail - non-superuser owner cannot alter the publications list +ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION somepub; +ERROR: must be superuser to alter publications for a subscription +ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION otherpub; +ERROR: must be superuser to alter publications for a subscription +ALTER SUBSCRIPTION regress_testsub SET PUBLICATION somepub, otherpub; +ERROR: must be superuser to alter publications for a subscription +-- fail - non-superuser owner cannot change subscription parameters +ALTER SUBSCRIPTION regress_testsub SET (copy_data = true); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (copy_data = false); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (create_slot = true); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (create_slot = false); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'somethingelse'); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = on); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = off); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = local); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_write); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_apply); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (binary = on); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (binary = off); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (connect = on); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (connect = off); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (streaming = on); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (streaming = off); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (two_phase = on); +ERROR: must be superuser to alter the options for a subscription +ALTER SUBSCRIPTION regress_testsub SET (two_phase = off); +ERROR: must be superuser to alter the options for a subscription +SET SESSION AUTHORIZATION 'regress_subscription_user'; -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name BEGIN; DROP SUBSCRIPTION regress_testsub; diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 7faa935a2a..8f66709441 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -105,6 +105,41 @@ ALTER ROLE regress_subscription_user2 SUPERUSER; -- now it works ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2; +-- revoke superuser from new owner +ALTER ROLE regress_subscription_user2 NOSUPERUSER; + +SET SESSION AUTHORIZATION regress_subscription_user2; + +-- fail - non-superuser owner cannot change connection parameter +ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=somethingelse'; + +-- fail - non-superuser owner cannot alter the publications list +ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION somepub; +ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION otherpub; +ALTER SUBSCRIPTION regress_testsub SET PUBLICATION somepub, otherpub; + +-- fail - non-superuser owner cannot change subscription parameters +ALTER SUBSCRIPTION regress_testsub SET (copy_data = true); +ALTER SUBSCRIPTION regress_testsub SET (copy_data = false); +ALTER SUBSCRIPTION regress_testsub SET (create_slot = true); +ALTER SUBSCRIPTION regress_testsub SET (create_slot = false); +ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'somethingelse'); +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = on); +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = off); +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = local); +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_write); +ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_apply); +ALTER SUBSCRIPTION regress_testsub SET (binary = on); +ALTER SUBSCRIPTION regress_testsub SET (binary = off); +ALTER SUBSCRIPTION regress_testsub SET (connect = on); +ALTER SUBSCRIPTION regress_testsub SET (connect = off); +ALTER SUBSCRIPTION regress_testsub SET (streaming = on); +ALTER SUBSCRIPTION regress_testsub SET (streaming = off); +ALTER SUBSCRIPTION regress_testsub SET (two_phase = on); +ALTER SUBSCRIPTION regress_testsub SET (two_phase = off); + +SET SESSION AUTHORIZATION 'regress_subscription_user'; + -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name BEGIN; DROP SUBSCRIPTION regress_testsub; -- 2.21.1 (Apple Git-122.3)