public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation 7+ messages / 5 participants [nested] [flat]
* [PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation @ 2020-07-14 00:15 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Alvaro Herrera @ 2020-07-14 00:15 UTC (permalink / raw) This means that ATExecCmd relies on AlteredRelationInfo->rel instead of keeping the relation as a local variable; this is useful if the subcommand needs to modify the relation internally. For example, a subcommand that internally commits its transaction needs this. --- src/backend/commands/tablecmds.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 993da56d43..a8528a3423 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -156,6 +156,8 @@ typedef struct AlteredTableInfo Oid relid; /* Relation to work on */ char relkind; /* Its relkind */ TupleDesc oldDesc; /* Pre-modification tuple descriptor */ + /* Transiently set during Phase 2, normally set to NULL */ + Relation rel; /* Information saved by Phase 1 for Phase 2: */ List *subcmds[AT_NUM_PASSES]; /* Lists of AlterTableCmd */ /* Information saved by Phases 1/2 for Phase 3: */ @@ -353,7 +355,7 @@ static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, AlterTableUtilityContext *context); static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, AlterTableUtilityContext *context); -static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, +static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, AlterTableCmd *cmd, LOCKMODE lockmode, int cur_pass, AlterTableUtilityContext *context); static AlterTableCmd *ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, @@ -4405,7 +4407,6 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, { AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); List *subcmds = tab->subcmds[pass]; - Relation rel; ListCell *lcmd; if (subcmds == NIL) @@ -4414,10 +4415,10 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, /* * Appropriate lock was obtained by phase 1, needn't get it again */ - rel = relation_open(tab->relid, NoLock); + tab->rel = relation_open(tab->relid, NoLock); foreach(lcmd, subcmds) - ATExecCmd(wqueue, tab, rel, + ATExecCmd(wqueue, tab, castNode(AlterTableCmd, lfirst(lcmd)), lockmode, pass, context); @@ -4429,7 +4430,11 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, if (pass == AT_PASS_ALTER_TYPE) ATPostAlterTypeCleanup(wqueue, tab, lockmode); - relation_close(rel, NoLock); + if (tab->rel) + { + relation_close(tab->rel, NoLock); + tab->rel = NULL; + } } } @@ -4455,11 +4460,12 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, * ATExecCmd: dispatch a subcommand to appropriate execution routine */ static void -ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, +ATExecCmd(List **wqueue, AlteredTableInfo *tab, AlterTableCmd *cmd, LOCKMODE lockmode, int cur_pass, AlterTableUtilityContext *context) { ObjectAddress address = InvalidObjectAddress; + Relation rel = tab->rel; switch (cmd->subtype) { @@ -5562,6 +5568,7 @@ ATGetQueueEntry(List **wqueue, Relation rel) */ tab = (AlteredTableInfo *) palloc0(sizeof(AlteredTableInfo)); tab->relid = relid; + tab->rel = NULL; /* set later */ tab->relkind = rel->rd_rel->relkind; tab->oldDesc = CreateTupleDescCopyConstr(RelationGetDescr(rel)); tab->newrelpersistence = RELPERSISTENCE_PERMANENT; -- 2.20.1 --/04w6evG8XlLl3ft Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-ALTER-TABLE-.-DETACH-CONCURRENTLY.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* running logical replication as the subscription owner @ 2023-03-03 16:02 Robert Haas <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Robert Haas @ 2023-03-03 16:02 UTC (permalink / raw) To: pgsql-hackers Hi, Here's a patch against master for $SUBJECT. It lacks documentation changes and might have bugs, so please review if you're concerned about this issue. To recap, under CVE-2020-14349, Noah documented that untrusted users shouldn't own tables into which the system is performing logical replication. Otherwise, the users could hook up triggers or default expressions or whatever to those tables and they would execute with the subscription owner's privileges, which would allow the table owners to escalate to superuser. However, I'm unsatisfied with just documenting the hazard, because I feel like almost everyone who uses logical replication wants to do the exact thing that this documentation says you shouldn't do. If you don't use logical replication or run everything as the superuser or just don't care about security, well then you don't have any problem here, but otherwise you probably do. The proposed fix is to perform logical replication actions (SELECT, INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table rather than as the owner of the subscription. The session still runs as the subscription owner, but the active user ID is switched to the table owner for the duration of each operation. To prevent table owners from doing tricky things to attack the subscription owner, we impose SECURITY_RESTRICTED_OPERATION while running as the table owner. To avoid inconveniencing users when this restriction adds no meaningful security, we refrain from imposing SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to the subscription owner anyway. Such a user need not use logical replication to break into the subscription owner's account: they have access to it anyway. There is also a possibility of an attack in the other direction. Maybe the subscription owner would like to obtain the table owner's permissions, or at the very least, use logical replication as a vehicle to perform operations they can't perform directly. A malicious subscription owner could hook up logical replication to a table into which the table owner doesn't want replication to occur. To block such attacks, the patch requires that the subscription owner have the ability to SET ROLE to each table owner. If the subscription owner is a superuser, which is usual, this will be automatic. Otherwise, the superuser will need to grant to the subscription owner the roles that own relevant tables. This can usefully serve as a kind of access control to make sure that the subscription doesn't touch any tables other than the ones it's supposed to be touching: just make those tables owned by a different user and don't grant them to the subscription owner. Previously, we provided no way at all of controlling the tables that replication can target. This fix interacts in an interesting way with Mark Dilger's work, committed by Jeff Davis, to make logical replication respect table permissions. I initially thought that with this change, that commit would end up just being reverted, with the permissions scheme described above replacing the existing one. However, I then realized that it's still good to perform those checks. Normally, a table owner can do any DML operation on a table they own, so those checks will never fail. However, if a table owner has revoked their ability to, say, INSERT into one of their own tables, then logical replication shouldn't bypass that and perform the INSERT anyway. So I now think that the checks added by that commit complement the ones added by this proposed patch, rather than competing with them. It is unclear to me whether we should try to back-port this. It's definitely a behavior change, and changing the behavior in the back branches is not a nice thing to do. On the other hand, at least in my opinion, the security consequences of the status quo are pretty dire. I tend to suspect that a really high percentage of people who are using logical replication at all are vulnerable to this, and lots of people having a way to escalate to superuser isn't good. Comments? -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v1-0001-Perform-logical-replication-actions-as-the-table-.patch (21.4K, ../../CA+TgmoaSCkg9ww9oppPqqs+9RVqCexYCE6Aq=UsYPfnOoDeFkw@mail.gmail.com/2-v1-0001-Perform-logical-replication-actions-as-the-table-.patch) download | inline diff: From 9168aa37141a59efccf3cea419ff4cacff7c1bb0 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Wed, 1 Mar 2023 12:59:02 -0500 Subject: [PATCH v1] Perform logical replication actions as the table owner. Up until now, logical replication actions have been performed as the subscription owner, who will generally be a superuser. Commit cec57b1a0fbcd3833086ba686897c5883e0a2afc documented hazards associated with that situation, namely, that any user who owns a table on the subscriber side could assume the privileges of the subscription owner by attaching a trigger, expression index, or some other kind of executable code to it. As a remedy, it suggested not creating configurations where users who are not fully trusted own tables on the subscriber. Although that will work, it basically precludes using logical replication in the way that people typically want to use it, namely, to replicate a database from one node to another without necessarily having any restrictions on which database users can own tables. So, instead, change logical replication to execute INSERT, UPDATE, DELETE, and TRUNCATE operations as the table owner when they are replicated. Since this involves switching the active user frequently within a session that is authenticated as the subscription user, also impose SECURITY_RESTRICTED_OPEATION restrictions on logical replication code. As an exception, if the table owner can SET ROLE to the subscription owner, these restrictions have no security value, so don't impose them in that case. Subscription owners are now required to have the ability to SET ROLE to every role that owns a table that the subscription is replicating. If they don't, replication will fail. Superusers, who normally own subscriptions, satisfy this property by default. Non-superusers users who own subscriptions will needed to be granted the roles that own relevant tables. --- src/backend/commands/tablecmds.c | 20 ++- src/backend/replication/logical/worker.c | 22 ++- src/backend/utils/init/Makefile | 3 +- src/backend/utils/init/meson.build | 3 +- src/backend/utils/init/usercontext.c | 92 ++++++++++++ src/include/commands/tablecmds.h | 3 +- src/include/utils/usercontext.h | 26 ++++ src/test/subscription/t/027_nosuperuser.pl | 165 ++++++++++----------- 8 files changed, 241 insertions(+), 93 deletions(-) create mode 100644 src/backend/utils/init/usercontext.c create mode 100644 src/include/utils/usercontext.h diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 62d9917ca3..a53ab38eb3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -103,6 +103,7 @@ #include "utils/syscache.h" #include "utils/timestamp.h" #include "utils/typcache.h" +#include "utils/usercontext.h" /* * ON COMMIT action list @@ -1761,7 +1762,7 @@ ExecuteTruncate(TruncateStmt *stmt) } ExecuteTruncateGuts(rels, relids, relids_logged, - stmt->behavior, stmt->restart_seqs); + stmt->behavior, stmt->restart_seqs, false); /* And close the rels */ foreach(cell, rels) @@ -1789,7 +1790,8 @@ void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, - DropBehavior behavior, bool restart_seqs) + DropBehavior behavior, bool restart_seqs, + bool run_as_table_owner) { List *rels; List *seq_relids = NIL; @@ -1928,7 +1930,14 @@ ExecuteTruncateGuts(List *explicit_rels, resultRelInfo = resultRelInfos; foreach(cell, rels) { + UserContext ucxt; + + if (run_as_table_owner) + SwitchToUntrustedUser(resultRelInfo->ri_RelationDesc->rd_rel->relowner, + &ucxt); ExecBSTruncateTriggers(estate, resultRelInfo); + if (run_as_table_owner) + RestoreUserContext(&ucxt); resultRelInfo++; } @@ -2133,7 +2142,14 @@ ExecuteTruncateGuts(List *explicit_rels, resultRelInfo = resultRelInfos; foreach(cell, rels) { + UserContext ucxt; + + if (run_as_table_owner) + SwitchToUntrustedUser(resultRelInfo->ri_RelationDesc->rd_rel->relowner, + &ucxt); ExecASTruncateTriggers(estate, resultRelInfo); + if (run_as_table_owner) + RestoreUserContext(&ucxt); resultRelInfo++; } diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index cfb2ab6248..9e07fbc6a8 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -207,6 +207,7 @@ #include "utils/rls.h" #include "utils/syscache.h" #include "utils/timeout.h" +#include "utils/usercontext.h" #define NAPTIME_PER_CYCLE 1000 /* max sleep time between cycles (1s) */ @@ -2408,6 +2409,7 @@ apply_handle_insert(StringInfo s) LogicalRepRelMapEntry *rel; LogicalRepTupleData newtup; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; @@ -2436,6 +2438,9 @@ apply_handle_insert(StringInfo s) return; } + /* Make sure that any user-supplied code runs as the table owner. */ + SwitchToUntrustedUser(rel->localrel->rd_rel->relowner, &ucxt); + /* Set relation for error callback */ apply_error_callback_arg.rel = rel; @@ -2465,6 +2470,8 @@ apply_handle_insert(StringInfo s) /* Reset relation for error callback */ apply_error_callback_arg.rel = NULL; + RestoreUserContext(&ucxt); + logicalrep_rel_close(rel, NoLock); end_replication_step(); @@ -2543,6 +2550,7 @@ apply_handle_update(StringInfo s) { LogicalRepRelMapEntry *rel; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; LogicalRepTupleData oldtup; @@ -2582,6 +2590,9 @@ apply_handle_update(StringInfo s) /* Check if we can do the update. */ check_relation_updatable(rel); + /* Make sure that any user-supplied code runs as the table owner. */ + SwitchToUntrustedUser(rel->localrel->rd_rel->relowner, &ucxt); + /* Initialize the executor state. */ edata = create_edata_for_relation(rel); estate = edata->estate; @@ -2632,6 +2643,8 @@ apply_handle_update(StringInfo s) /* Reset relation for error callback */ apply_error_callback_arg.rel = NULL; + RestoreUserContext(&ucxt); + logicalrep_rel_close(rel, NoLock); end_replication_step(); @@ -2713,6 +2726,7 @@ apply_handle_delete(StringInfo s) LogicalRepRelMapEntry *rel; LogicalRepTupleData oldtup; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; @@ -2747,6 +2761,9 @@ apply_handle_delete(StringInfo s) /* Check if we can do the delete. */ check_relation_updatable(rel); + /* Make sure that any user-supplied code runs as the table owner. */ + SwitchToUntrustedUser(rel->localrel->rd_rel->relowner, &ucxt); + /* Initialize the executor state. */ edata = create_edata_for_relation(rel); estate = edata->estate; @@ -2772,6 +2789,8 @@ apply_handle_delete(StringInfo s) /* Reset relation for error callback */ apply_error_callback_arg.rel = NULL; + RestoreUserContext(&ucxt); + logicalrep_rel_close(rel, NoLock); end_replication_step(); @@ -3219,7 +3238,8 @@ apply_handle_truncate(StringInfo s) relids, relids_logged, DROP_RESTRICT, - restart_seqs); + restart_seqs, + true); foreach(lc, remote_rels) { LogicalRepRelMapEntry *rel = lfirst(lc); diff --git a/src/backend/utils/init/Makefile b/src/backend/utils/init/Makefile index 362569393b..18c947464f 100644 --- a/src/backend/utils/init/Makefile +++ b/src/backend/utils/init/Makefile @@ -15,6 +15,7 @@ include $(top_builddir)/src/Makefile.global OBJS = \ globals.o \ miscinit.o \ - postinit.o + postinit.o \ + usercontext.o include $(top_srcdir)/src/backend/common.mk diff --git a/src/backend/utils/init/meson.build b/src/backend/utils/init/meson.build index b5ab154055..186be1381d 100644 --- a/src/backend/utils/init/meson.build +++ b/src/backend/utils/init/meson.build @@ -3,4 +3,5 @@ backend_sources += files( 'globals.c', 'miscinit.c', - 'postinit.c') + 'postinit.c', + 'usercontext.c') diff --git a/src/backend/utils/init/usercontext.c b/src/backend/utils/init/usercontext.c new file mode 100644 index 0000000000..88a7e55478 --- /dev/null +++ b/src/backend/utils/init/usercontext.c @@ -0,0 +1,92 @@ +/*------------------------------------------------------------------------- + * + * usercontext.c + * Convenience functions for running code as a different database user. + * + * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/backend/utils/init/usercontext.c + * + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "miscadmin.h" +#include "utils/acl.h" +#include "utils/guc.h" +#include "utils/usercontext.h" + +/* + * Temporarily switch to a new user ID. + * + * If the current user doesn't have permission to SET ROLE to the new user, + * an ERROR occurs. + * + * If the new user doesn't have permission to SET ROLE to the current user, + * SECURITY_RESTRICTED_OPERATION is imposed and a new GUC nest level is + * created so that any settings changes can be rolled back. + */ +void +SwitchToUntrustedUser(Oid userid, UserContext *context) +{ + /* Get the current user ID and security context. */ + GetUserIdAndSecContext(&context->save_userid, + &context->save_sec_context); + + /* Check that we have sufficient privileges to assume the target role. */ + if (!member_can_set_role(context->save_userid, userid)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("role \"%s\" cannot SET ROLE to \"%s\"", + GetUserNameFromId(context->save_userid, false), + GetUserNameFromId(userid, false)))); + + /* + * Try to prevent the user to which we're switching from assuming the + * privileges of the current user, unless they can SET ROLE to that user + * anyway. + */ + if (member_can_set_role(userid, context->save_userid)) + { + /* + * Each user can SET ROLE to the other, so there's no point in + * imposing any security restrictions. Just let the user do whatever + * they want. + */ + SetUserIdAndSecContext(userid, context->save_sec_context); + context->save_nestlevel = -1; + } + else + { + int sec_context = context->save_sec_context; + + /* + * This user can SET ROLE to the target user, but not the other way + * around, so protect ourselves against the target user by setting + * SECURITY_RESTRICTED_OPERATION to prevent certain changes to the + * session state. Also set up a new GUC nest level, so that we can roll + * back any GUC changes that may be made by code running as the target + * user, inasmuch as they could be malicious. + */ + sec_context |= SECURITY_RESTRICTED_OPERATION; + SetUserIdAndSecContext(userid, sec_context); + context->save_nestlevel = NewGUCNestLevel(); + } +} + +/* + * Switch back to the original user ID. + * + * If we created a new GUC nest level, also role back any changes that were + * made within it. + */ +void +RestoreUserContext(UserContext *context) +{ + if (context->save_nestlevel != -1) + AtEOXact_GUC(false, context->save_nestlevel); + SetUserIdAndSecContext(context->save_userid, context->save_sec_context); +} diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index e7c2b91a58..17b9404937 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -60,7 +60,8 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged, DropBehavior behavior, - bool restart_seqs); + bool restart_seqs, + bool run_as_table_owner); extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass); diff --git a/src/include/utils/usercontext.h b/src/include/utils/usercontext.h new file mode 100644 index 0000000000..a8195c194d --- /dev/null +++ b/src/include/utils/usercontext.h @@ -0,0 +1,26 @@ +/*------------------------------------------------------------------------- + * + * usercontext.h + * Convenience functions for running code as a different database user. + * + *------------------------------------------------------------------------- + */ +#ifndef USERCONTEXT_H +#define USERCONTEXT_H + +/* + * When temporarily changing to run as a different user, this structure + * holds the details needed to restore the original state. + */ +typedef struct UserContext +{ + Oid save_userid; + int save_sec_context; + int save_nestlevel; +} UserContext; + +/* Function prototypes. */ +extern void SwitchToUntrustedUser(Oid userid, UserContext *context); +extern void RestoreUserContext(UserContext *context); + +#endif /* USERCONTEXT_H */ diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl index 59192dbe2f..44d708877b 100644 --- a/src/test/subscription/t/027_nosuperuser.pl +++ b/src/test/subscription/t/027_nosuperuser.pl @@ -76,22 +76,6 @@ sub grant_superuser ALTER ROLE $role SUPERUSER)); } -sub revoke_bypassrls -{ - my ($role) = @_; - $node_subscriber->safe_psql( - 'postgres', qq( - ALTER ROLE $role NOBYPASSRLS)); -} - -sub grant_bypassrls -{ - my ($role) = @_; - $node_subscriber->safe_psql( - 'postgres', qq( - ALTER ROLE $role BYPASSRLS)); -} - # Create publisher and subscriber nodes with schemas owned and published by # "regress_alice" but subscribed and replicated by different role # "regress_admin". For partitioned tables, layout the partitions differently @@ -177,59 +161,32 @@ expect_failure( 2, 5, 7, - qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi, + qr/ERROR: ( [A-Z0-9]+:)? role "regress_admin" cannot SET ROLE to "regress_alice"/msi, "non-superuser admin fails to replicate update"); grant_superuser("regress_admin"); expect_replication("alice.unpartitioned", 2, 7, 9, "admin with restored superuser privilege replicates update"); -# Grant INSERT, UPDATE, DELETE privileges on the target tables to -# "regress_admin" so that superuser privileges are not necessary for -# replication. -# -# Note that UPDATE and DELETE also require SELECT privileges, which -# will be granted in subsequent test. -# +# Privileges on the target role suffice for non-superuser replication. $node_subscriber->safe_psql( 'postgres', qq( ALTER ROLE regress_admin NOSUPERUSER; -SET SESSION AUTHORIZATION regress_alice; -GRANT INSERT,UPDATE,DELETE ON - alice.unpartitioned, - alice.hashpart, alice.hashpart_a, alice.hashpart_b - TO regress_admin; -REVOKE SELECT ON alice.unpartitioned FROM regress_admin; +GRANT regress_alice TO regress_admin; )); publish_insert("alice.unpartitioned", 11); expect_replication("alice.unpartitioned", 3, 7, 11, - "nosuperuser admin with INSERT privileges can replicate into unpartitioned" + "nosuperuser admin with privileges on role can replicate INSERT into unpartitioned" ); publish_update("alice.unpartitioned", 7 => 13); -expect_failure( - "alice.unpartitioned", - 3, - 7, - 11, - qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi, - "non-superuser admin without SELECT privileges fails to replicate update" +expect_replication("alice.unpartitioned", 3, 9, 13, + "nosuperuser admin with privileges on role can replicate UPDATE into unpartitioned" ); -# Now grant SELECT -# -$node_subscriber->safe_psql( - 'postgres', qq( -SET SESSION AUTHORIZATION regress_alice; -GRANT SELECT ON - alice.unpartitioned, - alice.hashpart, alice.hashpart_a, alice.hashpart_b - TO regress_admin; -)); - publish_delete("alice.unpartitioned", 9); expect_replication("alice.unpartitioned", 2, 11, 13, - "nosuperuser admin with all table privileges can replicate into unpartitioned" + "nosuperuser admin with privileges on role can replicate DELETE into unpartitioned" ); # Test partitioning @@ -240,80 +197,114 @@ publish_insert("alice.hashpart", 103); publish_update("alice.hashpart", 102 => 120); publish_delete("alice.hashpart", 101); expect_replication("alice.hashpart", 2, 103, 120, - "nosuperuser admin with all table privileges can replicate into hashpart" + "nosuperuser admin with privileges on role can replicate into hashpart" ); - -# Enable RLS on the target table and check that "regress_admin" can -# only replicate into it when superuser or bypassrls. -# +# Force RLS on the target table and check that replication fails. $node_subscriber->safe_psql( 'postgres', qq( SET SESSION AUTHORIZATION regress_alice; ALTER TABLE alice.unpartitioned ENABLE ROW LEVEL SECURITY; +ALTER TABLE alice.unpartitioned FORCE ROW LEVEL SECURITY; )); -revoke_superuser("regress_admin"); publish_insert("alice.unpartitioned", 15); expect_failure( "alice.unpartitioned", 2, 11, 13, - qr/ERROR: ( [A-Z0-9]+:)? user "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi, - "non-superuser admin fails to replicate insert into rls enabled table"); -grant_superuser("regress_admin"); + qr/ERROR: ( [A-Z0-9]+:)? user "regress_alice" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi, + "replication of insert into table with forced rls fails"); + +# Since replication acts as the table owner, replication will succeed if we don't force it. +$node_subscriber->safe_psql( + 'postgres', qq( +ALTER TABLE alice.unpartitioned NO FORCE ROW LEVEL SECURITY; +)); expect_replication("alice.unpartitioned", 3, 11, 15, - "admin with restored superuser privilege replicates insert into rls enabled unpartitioned" + "non-superuser admin can replicate insert if rls is not forced" ); -revoke_superuser("regress_admin"); +$node_subscriber->safe_psql( + 'postgres', qq( +ALTER TABLE alice.unpartitioned FORCE ROW LEVEL SECURITY; +)); publish_update("alice.unpartitioned", 11 => 17); expect_failure( "alice.unpartitioned", 3, 11, 15, - qr/ERROR: ( [A-Z0-9]+:)? user "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi, - "non-superuser admin fails to replicate update into rls enabled unpartitioned" + qr/ERROR: ( [A-Z0-9]+:)? user "regress_alice" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi, + "replication of update into table with forced rls fails" ); - -grant_bypassrls("regress_admin"); +$node_subscriber->safe_psql( + 'postgres', qq( +ALTER TABLE alice.unpartitioned NO FORCE ROW LEVEL SECURITY; +)); expect_replication("alice.unpartitioned", 3, 13, 17, - "admin with bypassrls replicates update into rls enabled unpartitioned"); + "non-superuser admin can replicate update if rls is not forced"); -revoke_bypassrls("regress_admin"); -publish_delete("alice.unpartitioned", 13); +# Remove some of alice's privileges on her own table. Then replication should fail. +$node_subscriber->safe_psql( + 'postgres', qq( +REVOKE SELECT, INSERT ON alice.unpartitioned FROM regress_alice; +)); +publish_insert("alice.unpartitioned", 19); expect_failure( "alice.unpartitioned", 3, 13, 17, - qr/ERROR: ( [A-Z0-9]+:)? user "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi, - "non-superuser admin without bypassrls fails to replicate delete into rls enabled unpartitioned" + qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi, + "replication of insert fails if table owner lacks insert permission" ); -grant_bypassrls("regress_admin"); -expect_replication("alice.unpartitioned", 2, 15, 17, - "admin with bypassrls replicates delete into rls enabled unpartitioned"); -grant_superuser("regress_admin"); -# Alter the subscription owner to "regress_alice". She has neither superuser -# nor bypassrls, but as the table owner should be able to replicate. -# +# alice needs INSERT but not SELECT to replicate an INSERT. $node_subscriber->safe_psql( 'postgres', qq( -ALTER SUBSCRIPTION admin_sub DISABLE; -ALTER ROLE regress_alice SUPERUSER; -ALTER SUBSCRIPTION admin_sub OWNER TO regress_alice; -ALTER ROLE regress_alice NOSUPERUSER; -ALTER SUBSCRIPTION admin_sub ENABLE; +GRANT INSERT ON alice.unpartitioned TO regress_alice; )); +expect_replication("alice.unpartitioned", 4, 13, 19, + "restoring insert permission permits replication to continue"); -publish_insert("alice.unpartitioned", 23); -publish_update("alice.unpartitioned", 15 => 25); -publish_delete("alice.unpartitioned", 17); -expect_replication("alice.unpartitioned", 2, 23, 25, - "nosuperuser nobypassrls table owner can replicate delete into unpartitioned despite rls" +# Now let's try an UPDATE and a DELETE. +$node_subscriber->safe_psql( + 'postgres', qq( +REVOKE UPDATE, DELETE ON alice.unpartitioned FROM regress_alice; +)); +publish_update("alice.unpartitioned", 13 => 21); +publish_delete("alice.unpartitioned", 15); +expect_failure( + "alice.unpartitioned", + 4, + 13, + 19, + qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi, + "replication of update/delete fails if table owner lacks corresponding permission" ); +# Restoring UPDATE and DELETE is insufficient. +$node_subscriber->safe_psql( + 'postgres', qq( +GRANT UPDATE, DELETE ON alice.unpartitioned TO regress_alice; +)); +expect_failure( + "alice.unpartitioned", + 4, + 13, + 19, + qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi, + "replication of update/delete fails if table owner lacks SELECT permission" +); + +# alice needs INSERT but not SELECT to replicate an INSERT. +$node_subscriber->safe_psql( + 'postgres', qq( +GRANT SELECT ON alice.unpartitioned TO regress_alice; +)); +expect_replication("alice.unpartitioned", 3, 17, 21, + "restoring SELECT permission permits replication to continue"); + done_testing(); -- 2.37.1 (Apple Git-137.1) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: running logical replication as the subscription owner @ 2023-03-24 07:59 Jeff Davis <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Jeff Davis @ 2023-03-24 07:59 UTC (permalink / raw) To: Robert Haas <[email protected]>; pgsql-hackers On Fri, 2023-03-03 at 11:02 -0500, Robert Haas wrote: > Hi, > > Here's a patch against master for $SUBJECT. It lacks documentation > changes and might have bugs, so please review if you're concerned > about this issue. I think the subject has a typo, you mean "as the table owner", right? > However, I'm unsatisfied with just > documenting the hazard, because I feel like almost everyone who uses > logical replication wants to do the exact thing that this > documentation says you shouldn't do. +1 > The proposed fix is to perform logical replication actions (SELECT, > INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table > rather than as the owner of the subscription. The session still runs > as the subscription owner, but the active user ID is switched to the > table owner for the duration of each operation. To prevent table > owners from doing tricky things to attack the subscription owner, we > impose SECURITY_RESTRICTED_OPERATION while running as the table > owner. +1 > To avoid inconveniencing users when this restriction adds no > meaningful security, we refrain from imposing > SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to > the > subscription owner anyway. That's a little confusing, why not just always use the SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? > There is also a possibility of an attack in the other direction. > Maybe > the subscription owner would like to obtain the table owner's > permissions, or at the very least, use logical replication as a > vehicle to perform operations they can't perform directly. A > malicious > subscription owner could hook up logical replication to a table into > which the table owner doesn't want replication to occur. To block > such > attacks, the patch requires that the subscription owner have the > ability to SET ROLE to each table owner. It would be really nice if this could be done with some kind of ordinary privilege rather than requiring SET ROLE. A user might expect that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or pg_write_all_data. I can see theoretically that a table owner might write some dangerous code and attach it to their table. But I don't quite understand why they would do that. If the code was vulnerable to attack, would that mean that they couldn't even insert into their own table safely? Requiring SET ROLE seems like it makes the subscription role into something very close to a superuser. And that takes away some of the benefits of delegating to non-superusers. > If the subscription owner is > a superuser, which is usual, this will be automatic. Otherwise, the > superuser will need to grant to the subscription owner the roles that > own relevant tables. This can usefully serve as a kind of access > control to make sure that the subscription doesn't touch any tables > other than the ones it's supposed to be touching: just make those > tables owned by a different user and don't grant them to the > subscription owner. Previously, we provided no way at all of > controlling the tables that replication can target. We check for ordinary access privileges, which I think is your next point, so the above paragraph confuses me a bit. > This fix interacts in an interesting way with Mark Dilger's work, > committed by Jeff Davis, to make logical replication respect table > permissions. I initially thought that with this change, that commit > would end up just being reverted, with the permissions scheme > described above replacing the existing one. However, I then realized > that it's still good to perform those checks. Normally, a table owner > can do any DML operation on a table they own, so those checks will > never fail. However, if a table owner has revoked their ability to, > say, INSERT into one of their own tables, then logical replication > shouldn't bypass that and perform the INSERT anyway. So I now think > that the checks added by that commit complement the ones added by > this > proposed patch, rather than competing with them. That's an interesting case. > It is unclear to me whether we should try to back-port this. It's > definitely a behavior change, and changing the behavior in the back > branches is not a nice thing to do. On the other hand, at least in my > opinion, the security consequences of the status quo are pretty dire. > I tend to suspect that a really high percentage of people who are > using logical replication at all are vulnerable to this, and lots of > people having a way to escalate to superuser isn't good. It's worth considering given that most subscription owners are superusers anyway. What plausible cases might it break? Regards, Jeff Davis ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: running logical replication as the subscription owner @ 2023-03-24 14:00 Robert Haas <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Robert Haas @ 2023-03-24 14:00 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>; Noah Misch <[email protected]> On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <[email protected]> wrote: > That's a little confusing, why not just always use the > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? Some concern was expressed -- not sure exactly where the email is exactly, and it might've been on pgsql-security -- that doing that categorically might break things that are currently working. The people who were concerned included Andres and I forget who else. My gut reaction was the same as yours, just do it always and don't worry about it. But if people think that users are likely to run afoul of the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this is better, and the implementation complexity isn't high. We could even think of extending this kind of logic to other places where SECURITY_RESTRICTED_OPERATION is enforced, if desired. > It would be really nice if this could be done with some kind of > ordinary privilege rather than requiring SET ROLE. A user might expect > that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or > pg_write_all_data. > > I can see theoretically that a table owner might write some dangerous > code and attach it to their table. But I don't quite understand why > they would do that. If the code was vulnerable to attack, would that > mean that they couldn't even insert into their own table safely? > > Requiring SET ROLE seems like it makes the subscription role into > something very close to a superuser. And that takes away some of the > benefits of delegating to non-superusers. I am not thrilled with this either, but if you can arrange to run code as a certain user without the ability to SET ROLE to that user then there is, by definition, a potential privilege escalation. I don't think we can just dismiss that as a non-issue. If the ability of one user to potentially become some other user weren't a problem, we wouldn't need any patch at all. Imagine for example that the table owner has a trigger which doesn't sanitize search_path. The subscription owner can potentially leverage that to get the table owner's privileges. More generally, Stephen Frost has elsewhere argued that we should want the subscription owner to be a very low-privilege user, so that if their privileges get stolen, it's no big deal. I disagree with that. I think it's always a problem if one user can get unauthorized access to another user's account, regardless of exactly what those accounts can do. I think our goal should be to make it safe for the subscription owner to be a very high-privilege user, because you're going to need to be a very high-privilege user to set up replication. And if you do have that level of privilege, it's more convenient and simpler if you can just own the subscription yourself, rather than having to make a dummy account to own it. To put that another way, I think that what people are going to want to do in a lot of cases is have the superuser own the subscription, so I think we need to make that case safe, whatever it takes. In cases where the subscription owner isn't the superuser, I think the next most likely possibility is that the subscription owner is some kind of almost-super-user, like a CREATEROLE user or someone running with rds_superuser or similar on some PG fork. So that needs to be safe too, and I think this does that. Having the subscription owner be some random user that doesn't have a lot of privileges doesn't seem particularly useful to me. If it were unproblematic to allow that, sure, but considering how easy it would be for that low-privilege user to steal table owner privileges, I don't think it makes sense. > > It is unclear to me whether we should try to back-port this. It's > > definitely a behavior change, and changing the behavior in the back > > branches is not a nice thing to do. On the other hand, at least in my > > opinion, the security consequences of the status quo are pretty dire. > > I tend to suspect that a really high percentage of people who are > > using logical replication at all are vulnerable to this, and lots of > > people having a way to escalate to superuser isn't good. > > It's worth considering given that most subscription owners are > superusers anyway. What plausible cases might it break? AFAIU, the main concern is about the SECURITY_RESTRICTED_OPERATION flag interacting badly with things people are already doing. Other problems seem possible, e.g. if you're doing something that gets the current user name and does something with it, the answer's going to change, and you might like the new answer more or less than the old one. It's a little hard to predict who will be inconvenienced in what ways when you change behavior, but problems are certainly possible. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: running logical replication as the subscription owner @ 2023-03-24 16:58 Mark Dilger <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Mark Dilger @ 2023-03-24 16:58 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Noah Misch <[email protected]> > On Mar 24, 2023, at 7:00 AM, Robert Haas <[email protected]> wrote: > > More generally, Stephen Frost has elsewhere argued that we should want > the subscription owner to be a very low-privilege user, so that if > their privileges get stolen, it's no big deal. I disagree with that. I > think it's always a problem if one user can get unauthorized access to > another user's account, regardless of exactly what those accounts can > do. I think our goal should be to make it safe for the subscription > owner to be a very high-privilege user, because you're going to need > to be a very high-privilege user to set up replication. And if you do > have that level of privilege, it's more convenient and simpler if you > can just own the subscription yourself, rather than having to make a > dummy account to own it. To put that another way, I think that what > people are going to want to do in a lot of cases is have the superuser > own the subscription, so I think we need to make that case safe, > whatever it takes. I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting malicious content into the publication. I think you are focused on all the bad actors on the subscription-side database and what they can do to each other. That's also valid, but I get the impression that you're losing sight of the risk posed by malicious publishers. Or maybe you aren't, and can explain? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: running logical replication as the subscription owner @ 2023-03-24 18:35 Robert Haas <[email protected]> parent: Mark Dilger <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Robert Haas @ 2023-03-24 18:35 UTC (permalink / raw) To: Mark Dilger <[email protected]>; +Cc: Jeff Davis <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Noah Misch <[email protected]> On Fri, Mar 24, 2023 at 12:58 PM Mark Dilger <[email protected]> wrote: > I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting malicious content into the publication. I think you are focused on all the bad actors on the subscription-side database and what they can do to each other. That's also valid, but I get the impression that you're losing sight of the risk posed by malicious publishers. Or maybe you aren't, and can explain? You have a point. As things stand today, if you think somebody's going to send you changes to tables other than the ones you intend to replicate, you could handle that by making sure that the user that owns the subscription only has permission to write to the tables that are expected to receive replicated data. It's a bit awkward to set up because you have to initially make the subscription owner a superuser and then later remove the superuser bit, so I think this is another argument for the pg_create_subscription patch posted elsewhere, but if you're a superuser yourself, you can do it. However, with this patch, that wouldn't work any more, because the permissions checks don't happen until after we've switched to the target role. You could alternatively set up a user to own the subscription who has the ability to SET ROLE to some users and not others, but that only lets you restrict replication based on which user owns the tables, rather than which specific tables are getting data replicated into them. That actually wouldn't work today, and with the patch it would start working, so basically the effect of the patch on the problem that you mention would be to remove the ability to filter by specific table and add the ability to filter by owning role. I don't know how bad that sounds to you, and if it does sound bad, I don't immediately see how to mitigate it. As I said to Jeff, if you can replicate into a table that has a casually-written SECURITY INVOKER trigger on it, you can probably hack into the table owner's account. So I think that if we allow user A to replicate into user B's table with fewer privileges than A-can-set-role-to-B, we're building a privilege-escalation attack into the system. But if we do require A-can-set-role-to-B, then things change as described above. I suppose in theory we could check both A-can-set-role-to-B and A-can-modify-this-table-as-A, but that feels pretty unprincipled. I can't particularly see why A should need permission to perform an action that is actually going to be performed as B; what makes sense is to check that A can become B, and that B has permission to perform the action, which is the state that this patch would create. I guess there are other things we could do, too, like add replication restriction or filtering capabilities specifically to address this problem, or devise some other kind of new kind of permission system around this, but I don't have a specific idea what that would look like. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: running logical replication as the subscription owner @ 2023-03-25 09:24 Jelte Fennema <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Jelte Fennema @ 2023-03-25 09:24 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Mark Dilger <[email protected]>; Jeff Davis <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>; Noah Misch <[email protected]> > As things stand today, if you think somebody's going to send you > changes to tables other than the ones you intend to replicate, you > could handle that by making sure that the user that owns the > subscription only has permission to write to the tables that are > expected to receive replicated data. It's a bit awkward to set up > because you have to initially make the subscription owner a superuser > and then later remove the superuser bit, so I think this is another > argument for the pg_create_subscription patch posted elsewhere, but if > you're a superuser yourself, you can do it. However, with this patch, > that wouldn't work any more, because the permissions checks don't > happen until after we've switched to the target role. For my purposes I always trust the publisher, what I don't trust is the table owners. But I can indeed imagine scenarios where that's the other way around, and indeed you can protect against that currently, but not with your new patch. That seems fairly easily solvable though. + if (!member_can_set_role(context->save_userid, userid)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("role \"%s\" cannot SET ROLE to \"%s\"", + GetUserNameFromId(context->save_userid, false), + GetUserNameFromId(userid, false)))); If we don't throw an error here, but instead simply return, then the current behaviour is preserved and people can manually configure permissions to protect against an untrusted publisher. This would still mean that the table owners can escalate privileges to the subscription owner, but if that subscription owner actually has fewer privileges than the table owner then you don't have that issue. ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-03-25 09:24 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-07-14 00:15 [PATCH v5 1/2] Let ALTER TABLE exec routines deal with the relation Alvaro Herrera <[email protected]> 2023-03-03 16:02 running logical replication as the subscription owner Robert Haas <[email protected]> 2023-03-24 07:59 ` Re: running logical replication as the subscription owner Jeff Davis <[email protected]> 2023-03-24 14:00 ` Re: running logical replication as the subscription owner Robert Haas <[email protected]> 2023-03-24 16:58 ` Re: running logical replication as the subscription owner Mark Dilger <[email protected]> 2023-03-24 18:35 ` Re: running logical replication as the subscription owner Robert Haas <[email protected]> 2023-03-25 09:24 ` Re: running logical replication as the subscription owner Jelte Fennema <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox