diff options
-rw-r--r-- | doc/src/sgml/logical-replication.sgml | 25 | ||||
-rw-r--r-- | src/backend/commands/tablecmds.c | 20 | ||||
-rw-r--r-- | src/backend/replication/logical/worker.c | 22 | ||||
-rw-r--r-- | src/backend/utils/init/Makefile | 3 | ||||
-rw-r--r-- | src/backend/utils/init/meson.build | 3 | ||||
-rw-r--r-- | src/backend/utils/init/usercontext.c | 92 | ||||
-rw-r--r-- | src/include/commands/tablecmds.h | 3 | ||||
-rw-r--r-- | src/include/utils/usercontext.h | 26 | ||||
-rw-r--r-- | src/test/subscription/t/027_nosuperuser.pl | 165 |
9 files changed, 250 insertions, 109 deletions
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index 10ada41d803..90f39676632 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -1730,19 +1730,6 @@ CONTEXT: processing remote data for replication origin "pg_16395" during "INSER <title>Security</title> <para> - A user able to modify the schema of subscriber-side tables can execute - arbitrary code as the role which owns any subscription which modifies those tables. Limit ownership - and <literal>TRIGGER</literal> privilege on such tables to trusted roles. - Moreover, if untrusted users can create tables, use only - publications that list tables explicitly. That is to say, create a - subscription - <link linkend="sql-createpublication-for-all-tables"><literal>FOR ALL TABLES</literal></link> - or <link linkend="sql-createpublication-for-tables-in-schema"><literal>FOR TABLES IN SCHEMA</literal></link> - only when superusers trust every user permitted to create a non-temp table - on the publisher or the subscriber. - </para> - - <para> The role used for the replication connection must have the <literal>REPLICATION</literal> attribute (or be a superuser). If the role lacks <literal>SUPERUSER</literal> and <literal>BYPASSRLS</literal>, @@ -1784,12 +1771,18 @@ CONTEXT: processing remote data for replication origin "pg_16395" during "INSER </para> <para> - To create a subscription, the user must be a superuser. + To create a subscription, the user must have the privileges of the + the <literal>pg_create_subscription</literal> role, as well as + <literal>CREATE</literal> privileges on the database. </para> <para> - The subscription apply process will run in the local database with the - privileges of the subscription owner. + The subscription apply process will, at a session level, run with the + privileges of the subscription owner. However, when performing an insert, + update, delete, or truncate operation on a particular table, it will switch + roles to the table owner and perform the operation with the table owner's + privileges. This means that the subscription owner needs to be able to + <literal>SET ROLE</literal> to each role that owns a replicated table. </para> <para> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7808241d3fd..d9bbeafd82c 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 @@ -1762,7 +1763,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) @@ -1790,7 +1791,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; @@ -1929,7 +1931,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++; } @@ -2134,7 +2143,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 ef2a6beb361..61009fa8cda 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) */ @@ -2395,6 +2396,7 @@ apply_handle_insert(StringInfo s) LogicalRepRelMapEntry *rel; LogicalRepTupleData newtup; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; @@ -2423,6 +2425,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; @@ -2452,6 +2457,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(); @@ -2530,6 +2537,7 @@ apply_handle_update(StringInfo s) { LogicalRepRelMapEntry *rel; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; LogicalRepTupleData oldtup; @@ -2569,6 +2577,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; @@ -2619,6 +2630,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(); @@ -2702,6 +2715,7 @@ apply_handle_delete(StringInfo s) LogicalRepRelMapEntry *rel; LogicalRepTupleData oldtup; LogicalRepRelId relid; + UserContext ucxt; ApplyExecutionData *edata; EState *estate; TupleTableSlot *remoteslot; @@ -2736,6 +2750,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; @@ -2761,6 +2778,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(); @@ -3211,7 +3230,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 362569393b5..18c947464f4 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 b5ab154055e..186be1381d8 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 00000000000..88a7e55478e --- /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 e7c2b91a583..17b94049371 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 00000000000..a8195c194de --- /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 e770e0615c9..8a7e79cacac 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(); |