On Wed, 2025-01-22 at 13:25 -0500, Tom Lane wrote: > Laurenz Albe <laurenz.a...@cybertec.at> writes: > > [ v4-0001-Make-AFTER-triggers-run-with-the-correct-user.patch ] > > I started to look at this and got distracted by wondering why > afterTriggerAddEvent's scanning loop wasn't checking ats_modifiedcols. > That led to ea68ea632, which means this now needs a minor rebase.
Thanks for your efforts! > I have a couple other thoughts: > > * It's kind of sad that we have to add yet another field to > AfterTriggerSharedData, especially since this one will cost 8 bytes > thanks to alignment considerations. I'm not sure if there's another > way, but it seems like ats_relid, ats_rolid, and ats_modifiedcols > are all going to be extremely redundant in typical scenarios. > Maybe it's worth rethinking that data structure a bit? Or maybe > I'm worried over nothing; perhaps normal situations have only a few > AfterTriggerSharedDatas anyway. It might be worth trying to gather > some statistics about that. Thanks for investigating that as well. That matches my intuition that you typically don't run hundreds of different AFTER triggers in a single transaction. What about partitioned tables with hundreds of partitions? Could that be a problem? But then 100 times 8 bytes is still not a lot these days. > * I don't buy the bit about "We have to check if the role still > exists"; I think that's just a waste of cycles. There is no check > that prevents dropping the role a session is currently running as, > so why do we need one here? Moreover, the role could still get > dropped immediately after you look. Or for that matter, save_rolid > could be invalid by the time you restore it. None of this bothers me, > because if a session is running as a dropped role it will still > function pretty normally. It will find that it has no privileges > beyond those granted to PUBLIC, and if it tries to inspect > current_user or related functions those will fail, but there's > no compromise to system integrity. So I'd just remove that test. That makes sense to me. I was worried that it may lead to cryptic, hard-to-debug errors later on. Version 5 of the patch is attached. Yours, Laurenz Albe
From 23dd8630c09120d82a11b5334f8eda147f122bea Mon Sep 17 00:00:00 2001 From: Laurenz Albe <laurenz.a...@cybertec.at> Date: Thu, 23 Jan 2025 11:19:36 +0100 Subject: [PATCH v5] 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 Reviewed-By: Pavel Stehule, Joseph Koshakow, Isaac Morland Discussion: https://postgr.es/m/77ee784cf248e842f74588418f55c2931e47bd78.camel%40cybertec.at --- doc/src/sgml/trigger.sgml | 4 ++ src/backend/commands/trigger.c | 20 +++++++ src/test/regress/expected/triggers.out | 81 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 75 ++++++++++++++++++++++++ 4 files changed, 180 insertions(+) diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml index a9abaab9056..c3c0faf7a1b 100644 --- a/doc/src/sgml/trigger.sgml +++ b/doc/src/sgml/trigger.sgml @@ -129,6 +129,10 @@ In all cases, a trigger is executed as part of the same transaction as the statement that triggered it, so if either the statement or the trigger causes an error, the effects of both will be rolled back. + Also, the trigger will always run in the security context of the role + that executed the statement that caused the trigger to fire, unless + the trigger function is defined as <literal>SECURITY DEFINER</literal>, + in which case it will run as the function owner. </para> <para> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 1fa63ab7d0f..8f9cdf14067 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3635,6 +3635,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 */ @@ -4116,6 +4117,7 @@ afterTriggerAddEvent(AfterTriggerEventList *events, newshared->ats_firing_id == 0 && newshared->ats_table == evtshared->ats_table && newshared->ats_relid == evtshared->ats_relid && + newshared->ats_rolid == evtshared->ats_rolid && bms_equal(newshared->ats_modifiedcols, evtshared->ats_modifiedcols)) break; @@ -4291,6 +4293,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 @@ -4490,6 +4494,17 @@ AfterTriggerExecute(EState *estate, MemoryContextReset(per_tuple_context); + /* + * If the current role was different when the trigger got queued, + * temporarily change the current role. Note that the role might have been + * dropped since the trigger was queued, but if that is a problem, we will + * get an error later. Checking here would still leave a race condition. + */ + GetUserIdAndSecContext(&save_rolid, &save_sec_context); + if (save_rolid != evtshared->ats_rolid) + 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.) @@ -4504,6 +4519,10 @@ AfterTriggerExecute(EState *estate, rettuple != LocTriggerData.tg_newtuple) heap_freetuple(rettuple); + /* reset the current role if necessary */ + if (save_rolid != evtshared->ats_rolid) + SetUserIdAndSecContext(save_rolid, save_sec_context); + /* * Release resources */ @@ -6429,6 +6448,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 0657da17577..6bc3ad1ca16 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -3748,3 +3748,84 @@ Inherits: parent drop table parent, child; drop function f(); +-- test who runs deferred trigger functions +-- setup +create role regress_groot; +create role regress_outis; +create function whoami() returns trigger language plpgsql +as $$ +begin + raise warning 'I am %', current_user; + return null; +end; +$$; +alter function whoami() owner to regress_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 regress_groot; +insert into defer_trig values (1); +reset role; +set role regress_outis; +insert into defer_trig values (1); +reset role; +commit; +WARNING: I am regress_groot +WARNING: I am regress_outis +-- make sure that the user still exists at commit time +begin; +set role regress_groot; +insert into defer_trig values (1); +reset role; +drop role regress_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 regress_groot; +insert into defer_trig values (2); +reset role; +commit; +WARNING: I am regress_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 regress_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 regress_outis; +drop role regress_groot; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 7e2f7597c10..3f526798a77 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -2839,3 +2839,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 regress_groot; +create role regress_outis; +create function whoami() returns trigger language plpgsql +as $$ +begin + raise warning 'I am %', current_user; + return null; +end; +$$; +alter function whoami() owner to regress_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 regress_groot; +insert into defer_trig values (1); +reset role; +set role regress_outis; +insert into defer_trig values (1); +reset role; +commit; + +-- make sure that the user still exists at commit time +begin; +set role regress_groot; +insert into defer_trig values (1); +reset role; +drop role regress_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 regress_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 regress_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 regress_outis; +drop role regress_groot; -- 2.48.1
