On Fri, 2024-10-18 at 11:32 +0200, Pavel Stehule wrote: > pá 18. 10. 2024 v 10:20 odesílatel Laurenz Albe <laurenz.a...@cybertec.at> > napsal: > > On Fri, 2024-10-18 at 07:47 +0200, Pavel Stehule wrote: > > > Without deeper checks I don't like using GetUserNameFromId for checking > > > the validity of a role. > > > > > > Maybe it is better to use own read of syscache or wrap > > > SetUserIdAndSecContext to do this check. > > > > I agree; it was just the simplest way I could make it happen. It is ugly > > to allocate and > > return the user name, since we don't really need it. > > > > I could write a dedicated function to check the existence of a user. > > +1
The check is so simple that I didn't write a dedicated function. Instead, I put the catalog search into the code directly. > > > > The trigger queue exists only in memory, and PostgreSQL tracks dependencies > > only between persisted objects. Do you think that I should add a sentence > > like that to the comment? > > yes, please. I think so it is not too intuitive. Inside a context of this > patch > it is ok, but without knowledge of this context, can be strange, why some > role used > for trigger can be invalid, although the transaction is not fully finished > yet. I tried to improve the patch along these lines. Attached is a new version. Thanks for the review! Yours, Laurenz Albe
From 05fdfd7f49696810f09caca72bb6f0f35014df56 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <laurenz.a...@cybertec.at> Date: Fri, 18 Oct 2024 15:19:28 +0200 Subject: [PATCH v2] Make AFTER triggers run with the correct user With deferred triggers, it is possible that the current role changes between the time when the trigger is queued and the time it is executed (for example, the triggering data modification could have been executed in a SECURITY DEFINER function). Up to now, deferred trigger functions would run with the current role set to whatever was active at commit time. That does not matter for regular constraints, whose correctness doesn't depend on the current role. But for user-written contraint triggers, the current role certainly matters. Security considerations: - The trigger function could be modified between the time the trigger is queued and the time it runs. If the trigger was executed by a privileged user, the new behavior could be used for privilege escalation. But if a privileged user executes DML on a table owned by an untrusted user, all bets are off anyway --- the malicious code could as well be in the trigger function from the beginning. So we don't consider this a security hazard. - The previous behavior could lead to code inadvertently running with elevated privileges if a privileged user temporarily assumes lower privileges while executing DML on an untrusted table, but the deferred trigger runs with the user's original privileges. Also, that only applies if the privileged user commits *after* resuming the original role. This should be seen as deliberate self-damage rather than a security problem. Author: Laurenz Albe Discussion: https://postgr.es/m/77ee784cf248e842f74588418f55c2931e47bd78.camel%40cybertec.at --- src/backend/commands/trigger.c | 36 ++++++++++++ src/test/regress/expected/triggers.out | 81 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 75 ++++++++++++++++++++++++ 3 files changed, 192 insertions(+) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 09356e46d16..f762e484e28 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3634,6 +3634,7 @@ typedef struct AfterTriggerSharedData TriggerEvent ats_event; /* event type indicator, see trigger.h */ Oid ats_tgoid; /* the trigger's ID */ Oid ats_relid; /* the relation it's on */ + Oid ats_rolid; /* role to execute the trigger */ CommandId ats_firing_id; /* ID for firing cycle */ struct AfterTriggersTableData *ats_table; /* transition table access */ Bitmapset *ats_modifiedcols; /* modified columns */ @@ -4118,6 +4119,7 @@ afterTriggerAddEvent(AfterTriggerEventList *events, { if (newshared->ats_tgoid == evtshared->ats_tgoid && newshared->ats_relid == evtshared->ats_relid && + newshared->ats_rolid == evtshared->ats_rolid && newshared->ats_event == evtshared->ats_event && newshared->ats_table == evtshared->ats_table && newshared->ats_firing_id == 0) @@ -4292,6 +4294,8 @@ AfterTriggerExecute(EState *estate, int tgindx; bool should_free_trig = false; bool should_free_new = false; + Oid save_rolid; + int save_sec_context; /* * Locate trigger in trigdesc. It might not be present, and in fact the @@ -4491,6 +4495,33 @@ AfterTriggerExecute(EState *estate, MemoryContextReset(per_tuple_context); + /* + * If the current role was different when the trigger got queued, + * temporarily change the current role. + */ + GetUserIdAndSecContext(&save_rolid, &save_sec_context); + if (save_rolid != evtshared->ats_rolid) + { + HeapTuple tuple; + + /* + * We have to check if the role still exists. Since the trigger queue + * is in memory only, no dependencies to database objects are tracked, + * and the role could have been dropped after the trigger was queued. + */ + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(evtshared->ats_rolid)); + + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("invalid role OID: %u", evtshared->ats_rolid))); + + ReleaseSysCache(tuple); + + SetUserIdAndSecContext(evtshared->ats_rolid, + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); + } + /* * Call the trigger and throw away any possibly returned updated tuple. * (Don't let ExecCallTriggerFunc measure EXPLAIN time.) @@ -4505,6 +4536,10 @@ AfterTriggerExecute(EState *estate, rettuple != LocTriggerData.tg_newtuple) heap_freetuple(rettuple); + /* reset the current role */ + if (save_rolid != evtshared->ats_rolid) + SetUserIdAndSecContext(save_rolid, save_sec_context); + /* * Release resources */ @@ -6430,6 +6465,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, (trigger->tginitdeferred ? AFTER_TRIGGER_INITDEFERRED : 0); new_shared.ats_tgoid = trigger->tgoid; new_shared.ats_relid = RelationGetRelid(rel); + new_shared.ats_rolid = GetUserId(); new_shared.ats_firing_id = 0; if ((trigger->tgoldtable || trigger->tgnewtable) && transition_capture != NULL) diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index a044d6afe27..ae166e40317 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -3728,3 +3728,84 @@ Inherits: parent drop table parent, child; drop function f(); +-- test who runs deferred trigger functions +-- setup +create role groot; +create role outis; +create function whoami() returns trigger language plpgsql +as $$ +begin + raise warning 'I am %', current_user; + return null; +end; +$$; +alter function whoami() owner to outis; +create table defer_trig (id integer); +grant insert on defer_trig to public; +create constraint trigger whoami after insert on defer_trig + deferrable initially deferred + for each row + execute function whoami(); +-- deferred triggers must run as the user that queued the trigger +begin; +set role groot; +insert into defer_trig values (1); +reset role; +set role outis; +insert into defer_trig values (1); +reset role; +commit; +WARNING: I am groot +WARNING: I am outis +-- make sure that the user still exists at commit time +begin; +set role groot; +insert into defer_trig values (1); +reset role; +drop role groot; +do $$ +begin + -- catch the execption because it contains the role OID + set constraints all immediate; +exception when undefined_object then + raise warning 'user does not exist'; +end; +$$; +WARNING: user does not exist +rollback; +-- security definer functions override the user who queued the trigger +alter function whoami() security definer; +begin; +set role groot; +insert into defer_trig values (2); +reset role; +commit; +WARNING: I am outis +alter function whoami() security invoker; +-- make sure the current user is reset on error +create or replace function whoami() returns trigger language plpgsql +as $$ +begin + perform 1 / 0; + return null; +end; +$$; +begin; +set role groot; +insert into defer_trig values (2); +reset role; +commit; -- error expected +ERROR: division by zero +CONTEXT: SQL statement "SELECT 1 / 0" +PL/pgSQL function whoami() line 3 at PERFORM +select current_user = session_user; + ?column? +---------- + t +(1 row) + +-- clean up +drop table defer_trig; +drop function whoami(); +drop role outis; +drop role groot; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 51610788b21..83ce83fc475 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -2820,3 +2820,78 @@ alter trigger parenttrig on parent rename to anothertrig; drop table parent, child; drop function f(); + +-- test who runs deferred trigger functions +-- setup +create role groot; +create role outis; +create function whoami() returns trigger language plpgsql +as $$ +begin + raise warning 'I am %', current_user; + return null; +end; +$$; +alter function whoami() owner to outis; +create table defer_trig (id integer); +grant insert on defer_trig to public; +create constraint trigger whoami after insert on defer_trig + deferrable initially deferred + for each row + execute function whoami(); + +-- deferred triggers must run as the user that queued the trigger +begin; +set role groot; +insert into defer_trig values (1); +reset role; +set role outis; +insert into defer_trig values (1); +reset role; +commit; + +-- make sure that the user still exists at commit time +begin; +set role groot; +insert into defer_trig values (1); +reset role; +drop role groot; +do $$ +begin + -- catch the execption because it contains the role OID + set constraints all immediate; +exception when undefined_object then + raise warning 'user does not exist'; +end; +$$; +rollback; + +-- security definer functions override the user who queued the trigger +alter function whoami() security definer; +begin; +set role groot; +insert into defer_trig values (2); +reset role; +commit; +alter function whoami() security invoker; + +-- make sure the current user is reset on error +create or replace function whoami() returns trigger language plpgsql +as $$ +begin + perform 1 / 0; + return null; +end; +$$; +begin; +set role groot; +insert into defer_trig values (2); +reset role; +commit; -- error expected +select current_user = session_user; + +-- clean up +drop table defer_trig; +drop function whoami(); +drop role outis; +drop role groot; -- 2.47.0
