On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe <laurenz.a...@cybertec.at>
wrote:
> Like you, I was surprised by the current behavior. There is a design
> principle that PostgreSQL tries to follow, called the "Principle of
> least astonishment". Things should behave like a moderately skilled
> user would expect them to. In my opinion, the current behavior
violates
> that principle. Tomas seems to agree with that point of view.
I worry that both approaches violate this principle in different ways.
For example consider the following sequence of events:
SET ROLE r1;
BEGIN;
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO ...;
SET ROLE r2;
SET search_path = '...';
COMMIT;
I think that it would be reasonable to expect that the triggers execute
with r2 and not r1, since the triggers were explicitly deferred and the
role was explicitly set. It would likely be surprising that the search
path was updated for the trigger but not the role. With your proposed
approach it would be impossible for someone to trigger a trigger with
one role and execute it with another, if that's a desirable feature.
> I didn't find this strange behavior myself: it was one of our customers
> who uses security definer functions for data modifications and has
> problems with the current behavior, and I am trying to improve the
> situation on their behalf.
Would it be possible to share more details about this use case? For
example, What are their current problems? Are they not able to set
constraints to immediate? Or can they update the trigger function
itself be a security definer function? That might help illuminate why
the current behavior is wrong.
> But I feel that the database user that runs the trigger should be the
> same user that ran the triggering SQL statement. Even though I cannot
> put my hand on a case where changing this user would constitute a real
> security problem, it feels wrong.
>
> I am aware that that is rather squishy argumentation, but I have no
> better one. Both my and Thomas' gut reaction seems to have been "the
> current behavior is wrong".
I understand the gut reaction, and I even have the same gut reaction,
but since we would be treating roles exceptionally compared to the rest
of the execution context, I would feel better if we had a more concrete
reason.
I also took a look at the code. It doesn't apply cleanly to master, so
I took the liberty of rebasing and attaching it.
> + /*
> + * The role could have been dropped since the trigger was queued.
> + * In that case, give up and error out.
> + */
> + pfree(GetUserNameFromId(evtshared->ats_rolid, false));
It feels a bit wasteful to allocate and copy the role name when we
never actually use it. Is it possible to check that the role exists
without copying the name?
Everything else looked good, and the code does what it says it will.
Thanks,
Joe Koshakow
From f5de4ea29d0f78549618c23db5951120218af203 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.a...@cybertec.at>
Date: Wed, 6 Mar 2024 14:09:43 +0100
Subject: [PATCH] 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. However, that only
applies if the privileged user commits *after* resuming the original
role. Should this be backpatched as a security bug?
Author: Laurenz Albe
Discussion: https://postgr.es/m/77ee784cf248e842f74588418f55c2931e47bd78.camel%40cybertec.at
---
src/backend/commands/trigger.c | 23 ++++++++
src/test/regress/expected/triggers.out | 81 ++++++++++++++++++++++++++
src/test/regress/sql/triggers.sql | 75 ++++++++++++++++++++++++
3 files changed, 179 insertions(+)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 58b7fc5bbd..69d583751a 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3632,6 +3632,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 */
@@ -4113,6 +4114,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)
@@ -4287,6 +4289,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
@@ -4486,6 +4490,20 @@ AfterTriggerExecute(EState *estate,
MemoryContextReset(per_tuple_context);
+ /* set the appropriate role if necessary */
+ GetUserIdAndSecContext(&save_rolid, &save_sec_context);
+ if (save_rolid != evtshared->ats_rolid)
+ {
+ /*
+ * The role could have been dropped since the trigger was queued.
+ * In that case, give up and error out.
+ */
+ pfree(GetUserNameFromId(evtshared->ats_rolid, false));
+
+ 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.)
@@ -4500,6 +4518,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
*/
@@ -6425,6 +6447,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 a044d6afe2..ae166e4031 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 51610788b2..83ce83fc47 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.34.1
