Thread: Wrong security context for deferred triggers?
Create a table and a deferrable constraint trigger:
CREATE TABLE tab (i integer);
CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();
Create a role and allow it INSERT on the table:
CREATE ROLE duff;
GRANT INSERT ON tab TO duff;
Now become that role and try some inserts:
SET ROLE duff;
BEGIN;
INSERT INTO tab VALUES (1);
NOTICE: current_user = duff
That looks ok; the current user is "duff".
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO tab VALUES (2);
Become a superuser again and commit:
RESET ROLE;
COMMIT;
NOTICE: current_user = postgres
So a deferred constraint trigger does not run with the same security context
as an immediate trigger. This is somewhat nasty in combination with
SECURITY DEFINER functions: if that function performs an operation, and that
operation triggers a deferred trigger, that trigger will run in the wrong
security context.
This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.
Yours,
Laurenz Albe
On Mon, 2023-11-06 at 14:23 +0100, Laurenz Albe wrote: > CREATE FUNCTION trig() RETURNS trigger > LANGUAGE plpgsql AS > $$BEGIN > RAISE NOTICE 'current_user = %', current_user; > RETURN NEW; > END;$$; > > CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab > DEFERRABLE INITIALLY IMMEDIATE > FOR EACH ROW EXECUTE FUNCTION trig(); > > SET ROLE duff; > > BEGIN; > > INSERT INTO tab VALUES (1); > NOTICE: current_user = duff > > That looks ok; the current user is "duff". > > SET CONSTRAINTS ALL DEFERRED; > > INSERT INTO tab VALUES (2); > > Become a superuser again and commit: > > RESET ROLE; > > COMMIT; > NOTICE: current_user = postgres > > > So a deferred constraint trigger does not run with the same security context > as an immediate trigger. This is somewhat nasty in combination with > SECURITY DEFINER functions: if that function performs an operation, and that > operation triggers a deferred trigger, that trigger will run in the wrong > security context. > > This behavior looks buggy to me. What do you think? > I cannot imagine that it is a security problem, though. I just realized one problem with running a deferred constraint trigger as the triggering role: that role might have been dropped by the time the trigger executes. But then we could still error out. Yours, Laurenz Albe
On Mon, 6 Nov 2023 at 11:58, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Become a superuser again and commit:
>
> RESET ROLE;
>
> COMMIT;
> NOTICE: current_user = postgres
>
>
> So a deferred constraint trigger does not run with the same security context
> as an immediate trigger. This is somewhat nasty in combination with
> SECURITY DEFINER functions: if that function performs an operation, and that
> operation triggers a deferred trigger, that trigger will run in the wrong
> security context.
>
> This behavior looks buggy to me. What do you think?
> I cannot imagine that it is a security problem, though.
This looks to me like another reason that triggers should run as the trigger owner. Which role owns the trigger won’t change as a result of constraints being deferred or not, or any role setting done during the transaction, including that relating to security definer functions.
Right now triggers can’t do anything that those who can INSERT/UPDATE/DELETE (i.e., cause the trigger to fire) can’t do, which in particular breaks the almost canonical example of using triggers to log changes — I can’t do it without also allowing users to make spurious log entries.
Also if I cause a trigger to fire, I’ve just given the trigger owner the opportunity to run arbitrary code as me.
I just realized one problem with running a deferred constraint trigger as
the triggering role: that role might have been dropped by the time the trigger
executes. But then we could still error out.
This problem is also fixed by running triggers as their owners: there should be a dependency between an object and its owner. So the trigger-executing role can’t be dropped without dropping the trigger.
On 11/6/23 14:23, Laurenz Albe wrote: > ... > > This behavior looks buggy to me. What do you think? > I cannot imagine that it is a security problem, though. > How could code getting executed under the wrong role not be a security issue? Also, does this affect just the role, or are there some other settings that may unexpectedly change (e.g. search_path)? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 2023-11-06 at 18:29 +0100, Tomas Vondra wrote: > On 11/6/23 14:23, Laurenz Albe wrote: > > This behavior looks buggy to me. What do you think? > > I cannot imagine that it is a security problem, though. > > How could code getting executed under the wrong role not be a security > issue? Also, does this affect just the role, or are there some other > settings that may unexpectedly change (e.g. search_path)? Perhaps it is a security issue, and I am just lacking imagination. Yes, changes to "search_path" should also have an effect. Yours, Laurenz Albe
On Mon, 2023-11-06 at 18:29 +0100, Tomas Vondra wrote: > On 11/6/23 14:23, Laurenz Albe wrote: > > This behavior looks buggy to me. What do you think? > > I cannot imagine that it is a security problem, though. > > How could code getting executed under the wrong role not be a security > issue? Also, does this affect just the role, or are there some other > settings that may unexpectedly change (e.g. search_path)? Here is a patch that fixes this problem by keeping track of the current role in the AfterTriggerSharedData. I have thought some more about the security aspects: 1. With the new code, you could call a SECURITY DEFINER function that modifies data on a table with a deferred trigger, then modify the trigger function before you commit and have your code run with elevated privileges. But I think that we need not worry about that. If a superuser performs DML on a table that an untrusted user controls, all bets are off anyway. The attacker might as well put the bad code into the trigger *before* calling the SECURITY DEFINER function. 2. The more serious concern is that the old code constitutes a narrow security hazard: a superuser could temporarily assume an unprivileged role to avoid risks while performing DML on a table controlled by an untrusted user, but for some reason resume being a superuser *before* COMMIT. Then a deferred trigger would inadvertently run with superuser privileges. That seems like a very unlikely scenario (who would RESET ROLE before committing in that situation?). Moreover, it seems like the current buggy behavior has been in place for decades without anybody noticing. I am not against backpatching this (the fix is simple enough), but I am not convinced that it is necessary. Yours, Laurenz Albe
Attachment
Hi,
I see that this patch is marked as ready for review, so I thought I
would attempt to review it. This is my first review, so please take it
with a grain of salt.
> So a deferred constraint trigger does not run with the same security context
> as an immediate trigger.
It sounds like the crux of your argument is that the current behavior
is that triggers are executed with the role and security context of the
session at the time of execution. Instead, the trigger should be
executed with the role and security context of the session at the time
time of queuing (i.e. the same context as the action that triggered the
trigger). While I understand that the current behavior can be
surprising in some scenarios, it's not clear to me why this behavior is
wrong. It seems that the whole point of deferring a trigger to commit
time is that the context that the trigger is executed in is different
than the context that it was triggered in. Tables may have changed,
permissions may have changed, session configuration variables may have
changed, roles may have changed, etc. So why should the executing role
be treated differently and restored to the value at the time of
triggering. Perhaps you can expand on why you feel that the current
behavior is wrong?
> This is somewhat nasty in combination with
> SECURITY DEFINER functions: if that function performs an operation, and that
> operation triggers a deferred trigger, that trigger will run in the wrong
> security context.
...
> The more serious concern is that the old code constitutes
> a narrow security hazard: a superuser could temporarily
> assume an unprivileged role to avoid risks while performing
> DML on a table controlled by an untrusted user, but for
> some reason resume being a superuser *before* COMMIT.
> Then a deferred trigger would inadvertently run with
> superuser privileges.
I find these examples to be surprising, but not necessarily wrong
(as per my comment above). If someone wants the triggers to be executed
as the triggering role, then they can run `SET CONSTRAINTS ALL
IMMEDIATE`. If deferring a trigger to commit time and executing it as
the triggering role is desirable, then maybe we should add a modifier
to triggers that can control this behavior. Something like
`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.
> This looks to me like another reason that triggers should run as the
> trigger owner. Which role owns the trigger won’t change as a result of
> constraints being deferred or not, or any role setting done during the
> transaction, including that relating to security definer functions.
>
> Right now triggers can’t do anything that those who can
> INSERT/UPDATE/DELETE (i.e., cause the trigger to fire) can’t do, which in
>particular breaks the almost canonical example of using triggers to log
> changes — I can’t do it without also allowing users to make spurious log
> entries.
>
> Also if I cause a trigger to fire, I’ve just given the trigger owner the
> opportunity to run arbitrary code as me.
>
>> I just realized one problem with running a deferred constraint trigger as
>> the triggering role: that role might have been dropped by the time the
>> trigger
>> executes. But then we could still error out.
>
> This problem is also fixed by running triggers as their owners: there
> should be a dependency between an object and its owner. So the
> trigger-executing role can’t be dropped without dropping the trigger.
+1, this approach would remove all of the surprising/wrong behavior and
in my opinion is more obvious. I'd like to add some more reasons why
this behavior makes sense:
- The documentation [0] indicates that to create a trigger, the
creating role must have the `EXECUTE` privilege on the trigger
function. In fact this check is skipped for the role that triggers
trigger.
-- Create trig_owner role and function. Grant execute on function
-- to role.
test=# CREATE ROLE trig_owner;
CREATE ROLE
test=# GRANT CREATE ON SCHEMA public TO trig_owner;
GRANT
test=# CREATE OR REPLACE FUNCTION f() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# REVOKE EXECUTE ON FUNCTION f FROM PUBLIC;
REVOKE
test=# GRANT EXECUTE ON FUNCTION f TO trig_owner;
GRANT
-- Create the trigger as trig_owner.
test=# SET ROLE trig_owner;
SET
test=> CREATE TABLE t (a INT);
CREATE TABLE
test=> CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON t
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION f();
CREATE TRIGGER
-- Trigger the trigger with a role that doesn't have execute
-- privileges on the trigger function and also call the function
-- directly. The trigger succeeds but the function call fails.
test=> RESET ROLE;
RESET
test=# CREATE ROLE r1;
CREATE ROLE
test=# GRANT INSERT ON t TO r1;
GRANT
test=# SET ROLE r1;
SET
test=> INSERT INTO t VALUES (1);
NOTICE: current_user = r1
INSERT 0 1
test=> SELECT f();
ERROR: permission denied for function f
So the execute privilege check on the trigger function is done using
the trigger owner role, why shouldn't all privilege checks use the
trigger owner?
- The set of triggers that will execute as a result of any statement
is not obvious. Therefore it is non-trivial to figure out if you're
role will have the privileges necessary to execute a given statement.
Switching to the trigger owner role makes this check trivial.
- This is consistent with how view privileges work. When querying a
view, the privileges of the view owner is checked against the view
definition. Similarly when executing a trigger, the trigger owner's
privileges should be checked against the trigger definition.
However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work. Another draw back is that if
the trigger owner loses the required privileges, then no one can modify
to the table.
> Here is a patch that fixes this problem by keeping track of the
current role in the AfterTriggerSharedData.
I skimmed the code and haven't looked at in depth. Whichever direction
we go, I think it's worth updating the documentation to make the
behavior around triggers and roles clear.
Additionally, I applied your patch to master and re-ran the example and
didn't notice any behavior change.
test=# CREATE TABLE tab (i integer);
CREATE TABLE
test=# CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();
CREATE TRIGGER
test=# CREATE ROLE duff;
CREATE ROLE
test=# GRANT INSERT ON tab TO duff;
GRANT
test=# SET ROLE duff;
SET
test=> BEGIN;
BEGIN
test=*> INSERT INTO tab VALUES (1);
NOTICE: current_user = duff
INSERT 0 1
test=*> SET CONSTRAINTS ALL DEFERRED;
SET CONSTRAINTS
test=*> INSERT INTO tab VALUES (2);
INSERT 0 1
test=*> RESET ROLE;
RESET
test=*# COMMIT;
NOTICE: current_user = joe
COMMIT
Though maybe I'm just doing something wrong.
Thanks,
Joe Koshakow
P.S. Since this is my first review, feel free to give me meta-comments
critiquing the review.
[0] https://www.postgresql.org/docs/16/sql-createtrigger.html
I see that this patch is marked as ready for review, so I thought I
would attempt to review it. This is my first review, so please take it
with a grain of salt.
> So a deferred constraint trigger does not run with the same security context
> as an immediate trigger.
It sounds like the crux of your argument is that the current behavior
is that triggers are executed with the role and security context of the
session at the time of execution. Instead, the trigger should be
executed with the role and security context of the session at the time
time of queuing (i.e. the same context as the action that triggered the
trigger). While I understand that the current behavior can be
surprising in some scenarios, it's not clear to me why this behavior is
wrong. It seems that the whole point of deferring a trigger to commit
time is that the context that the trigger is executed in is different
than the context that it was triggered in. Tables may have changed,
permissions may have changed, session configuration variables may have
changed, roles may have changed, etc. So why should the executing role
be treated differently and restored to the value at the time of
triggering. Perhaps you can expand on why you feel that the current
behavior is wrong?
> This is somewhat nasty in combination with
> SECURITY DEFINER functions: if that function performs an operation, and that
> operation triggers a deferred trigger, that trigger will run in the wrong
> security context.
...
> The more serious concern is that the old code constitutes
> a narrow security hazard: a superuser could temporarily
> assume an unprivileged role to avoid risks while performing
> DML on a table controlled by an untrusted user, but for
> some reason resume being a superuser *before* COMMIT.
> Then a deferred trigger would inadvertently run with
> superuser privileges.
I find these examples to be surprising, but not necessarily wrong
(as per my comment above). If someone wants the triggers to be executed
as the triggering role, then they can run `SET CONSTRAINTS ALL
IMMEDIATE`. If deferring a trigger to commit time and executing it as
the triggering role is desirable, then maybe we should add a modifier
to triggers that can control this behavior. Something like
`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.
> This looks to me like another reason that triggers should run as the
> trigger owner. Which role owns the trigger won’t change as a result of
> constraints being deferred or not, or any role setting done during the
> transaction, including that relating to security definer functions.
>
> Right now triggers can’t do anything that those who can
> INSERT/UPDATE/DELETE (i.e., cause the trigger to fire) can’t do, which in
>particular breaks the almost canonical example of using triggers to log
> changes — I can’t do it without also allowing users to make spurious log
> entries.
>
> Also if I cause a trigger to fire, I’ve just given the trigger owner the
> opportunity to run arbitrary code as me.
>
>> I just realized one problem with running a deferred constraint trigger as
>> the triggering role: that role might have been dropped by the time the
>> trigger
>> executes. But then we could still error out.
>
> This problem is also fixed by running triggers as their owners: there
> should be a dependency between an object and its owner. So the
> trigger-executing role can’t be dropped without dropping the trigger.
+1, this approach would remove all of the surprising/wrong behavior and
in my opinion is more obvious. I'd like to add some more reasons why
this behavior makes sense:
- The documentation [0] indicates that to create a trigger, the
creating role must have the `EXECUTE` privilege on the trigger
function. In fact this check is skipped for the role that triggers
trigger.
-- Create trig_owner role and function. Grant execute on function
-- to role.
test=# CREATE ROLE trig_owner;
CREATE ROLE
test=# GRANT CREATE ON SCHEMA public TO trig_owner;
GRANT
test=# CREATE OR REPLACE FUNCTION f() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# REVOKE EXECUTE ON FUNCTION f FROM PUBLIC;
REVOKE
test=# GRANT EXECUTE ON FUNCTION f TO trig_owner;
GRANT
-- Create the trigger as trig_owner.
test=# SET ROLE trig_owner;
SET
test=> CREATE TABLE t (a INT);
CREATE TABLE
test=> CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON t
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION f();
CREATE TRIGGER
-- Trigger the trigger with a role that doesn't have execute
-- privileges on the trigger function and also call the function
-- directly. The trigger succeeds but the function call fails.
test=> RESET ROLE;
RESET
test=# CREATE ROLE r1;
CREATE ROLE
test=# GRANT INSERT ON t TO r1;
GRANT
test=# SET ROLE r1;
SET
test=> INSERT INTO t VALUES (1);
NOTICE: current_user = r1
INSERT 0 1
test=> SELECT f();
ERROR: permission denied for function f
So the execute privilege check on the trigger function is done using
the trigger owner role, why shouldn't all privilege checks use the
trigger owner?
- The set of triggers that will execute as a result of any statement
is not obvious. Therefore it is non-trivial to figure out if you're
role will have the privileges necessary to execute a given statement.
Switching to the trigger owner role makes this check trivial.
- This is consistent with how view privileges work. When querying a
view, the privileges of the view owner is checked against the view
definition. Similarly when executing a trigger, the trigger owner's
privileges should be checked against the trigger definition.
However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work. Another draw back is that if
the trigger owner loses the required privileges, then no one can modify
to the table.
> Here is a patch that fixes this problem by keeping track of the
current role in the AfterTriggerSharedData.
I skimmed the code and haven't looked at in depth. Whichever direction
we go, I think it's worth updating the documentation to make the
behavior around triggers and roles clear.
Additionally, I applied your patch to master and re-ran the example and
didn't notice any behavior change.
test=# CREATE TABLE tab (i integer);
CREATE TABLE
test=# CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();
CREATE TRIGGER
test=# CREATE ROLE duff;
CREATE ROLE
test=# GRANT INSERT ON tab TO duff;
GRANT
test=# SET ROLE duff;
SET
test=> BEGIN;
BEGIN
test=*> INSERT INTO tab VALUES (1);
NOTICE: current_user = duff
INSERT 0 1
test=*> SET CONSTRAINTS ALL DEFERRED;
SET CONSTRAINTS
test=*> INSERT INTO tab VALUES (2);
INSERT 0 1
test=*> RESET ROLE;
RESET
test=*# COMMIT;
NOTICE: current_user = joe
COMMIT
Though maybe I'm just doing something wrong.
Thanks,
Joe Koshakow
P.S. Since this is my first review, feel free to give me meta-comments
critiquing the review.
[0] https://www.postgresql.org/docs/16/sql-createtrigger.html
On Sat, Jun 8, 2024 at 5:36 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> Additionally, I applied your patch to master and re-ran the example and
> didn't notice any behavior change.
>
> test=# CREATE TABLE tab (i integer);
> CREATE TABLE
> test=# CREATE FUNCTION trig() RETURNS trigger
> LANGUAGE plpgsql AS
> $$BEGIN
> RAISE NOTICE 'current_user = %', current_user;
> RETURN NEW;
> END;$$;
> CREATE FUNCTION
> test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
> DEFERRABLE INITIALLY IMMEDIATE
> FOR EACH ROW EXECUTE FUNCTION trig();
> CREATE TRIGGER
> test=# CREATE ROLE duff;
> CREATE ROLE
> test=# GRANT INSERT ON tab TO duff;
> GRANT
> test=# SET ROLE duff;
> SET
> test=> BEGIN;
> BEGIN
> test=*> INSERT INTO tab VALUES (1);
> NOTICE: current_user = duff
> INSERT 0 1
> test=*> SET CONSTRAINTS ALL DEFERRED;
> SET CONSTRAINTS
> test=*> INSERT INTO tab VALUES (2);
> INSERT 0 1
> test=*> RESET ROLE;
> RESET
> test=*# COMMIT;
> NOTICE: current_user = joe
> COMMIT
>
> Though maybe I'm just doing something wrong.
Sorry, there's definitely something wrong with my environment. You can
> Additionally, I applied your patch to master and re-ran the example and
> didn't notice any behavior change.
>
> test=# CREATE TABLE tab (i integer);
> CREATE TABLE
> test=# CREATE FUNCTION trig() RETURNS trigger
> LANGUAGE plpgsql AS
> $$BEGIN
> RAISE NOTICE 'current_user = %', current_user;
> RETURN NEW;
> END;$$;
> CREATE FUNCTION
> test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
> DEFERRABLE INITIALLY IMMEDIATE
> FOR EACH ROW EXECUTE FUNCTION trig();
> CREATE TRIGGER
> test=# CREATE ROLE duff;
> CREATE ROLE
> test=# GRANT INSERT ON tab TO duff;
> GRANT
> test=# SET ROLE duff;
> SET
> test=> BEGIN;
> BEGIN
> test=*> INSERT INTO tab VALUES (1);
> NOTICE: current_user = duff
> INSERT 0 1
> test=*> SET CONSTRAINTS ALL DEFERRED;
> SET CONSTRAINTS
> test=*> INSERT INTO tab VALUES (2);
> INSERT 0 1
> test=*> RESET ROLE;
> RESET
> test=*# COMMIT;
> NOTICE: current_user = joe
> COMMIT
>
> Though maybe I'm just doing something wrong.
Sorry, there's definitely something wrong with my environment. You can
ignore this.
Thanks,
Joe Koshakow
On Sat, 8 Jun 2024 at 17:37, Joseph Koshakow <koshy44@gmail.com> wrote:
However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work. Another draw back is that if
the trigger owner loses the required privileges, then no one can modify
to the table.
I worry about breaking changes too. The more I think about it, though, the more I think the existing behaviour doesn’t make sense.
Speaking as a table owner, when I set a trigger on it, I expect that when the specified actions occur my trigger will fire and will do what I specify, without regard to the execution environment of the caller (search_path in particular); and my trigger should be able to do anything that I can do. For the canonical case of a logging table the trigger has to be able to do stuff the caller can't do. I don't expect to be able to do stuff that the caller can do.
Speaking as someone making an update on a table, I don't expect to have it fail because my execution environment (search_path in particular) is wrong for the trigger implementation, and I consider it a security violation if the table owner is able to do stuff as me as a result, especially if I am an administrator making an update as superuser.
In effect, I want the action which fires the trigger to be like a system call, and the trigger, plus the underlying action, to be like what the OS does in response to the system call.
I'm not sure how to evaluate what problems with existing implementations would be caused by changing what role executes triggers, but I think it's pretty clear the existing behaviour is the wrong choice in every other way than backward compatibility. I welcome examples to the contrary, where the existing behaviour is not just OK but actually wanted.
On Sat, Jun 8, 2024 at 10:13 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> Speaking as a table owner, when I set a trigger on it, I expect that when the specified actions occur my trigger will fire and will do what I specify, without regard to the execution environment of the caller (search_path in particular); and my trigger should be able to do anything that I can do. For the canonical case of a logging table the trigger has to be able to do stuff the caller can't do. I don't expect to be able to do stuff that the caller can do.
>
> Speaking as someone making an update on a table, I don't expect to have it fail because my execution environment (search_path in particular) is wrong for the trigger implementation, and I consider it a security violation if the table owner is able to do stuff as me as a result, especially if I am an administrator making an update as superuser.
Can you expand on this a bit? When a trigger executes should the
execution environment match:
- The execution environment of the trigger owner at the time of
trigger creation?
- The execution environment of the function owner at the time of
function creation?
- An execution environment built from the trigger owner's default
configuration parameters?
- Something else?
While I am convinced that privileges should be checked using the
trigger owner's role, I'm less convinced of other configuration
parameters. For the search_path example, that can be resolved by
either fully qualifying object names or setting the search_path in the
function itself. Similar approaches can be taken with other
configuration parameters.
I also worry that it would be a source of confusion that the execution
environment of triggers come from the trigger/function owner, but the
execution environment of function calls come from the caller.
> I think it's pretty clear the existing behaviour is the wrong choice in every other way than backward compatibility. I welcome examples to the contrary, where the existing behaviour is not just OK but actually wanted.
This is perhaps a contrived example, but here's one. Suppose I create a
trigger that raises a notice that includes the current timestamp. I
would probably want to use the timezone of the caller, not the
trigger owner.
Thanks,
Joe Koshakow
On Sat, 2024-06-08 at 17:36 -0400, Joseph Koshakow wrote: > I see that this patch is marked as ready for review, so I thought I > would attempt to review it. This is my first review, so please take it > with a grain of salt. Thank you. Your review is valuable and much to the point. > > It sounds like the crux of your argument is that the current behavior > is that triggers are executed with the role and security context of the > session at the time of execution. Instead, the trigger should be > executed with the role and security context of the session at the time > time of queuing (i.e. the same context as the action that triggered the > trigger). While I understand that the current behavior can be > surprising in some scenarios, it's not clear to me why this behavior is > wrong. Since neither the documentation nor any source comment cover this case, it is to some extent a matter of taste if the current behavior is correct ot not. An alternative to my patch would be to document the current behavior rather than change it. 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. On the other hand, changing current behavior carries the risk of backward compatibility problems. I don't know how much of a problem that would be, but I have the impression that few people use deferred triggers in combination with SET ROLE or SECURITY DEFINER functions, which makes the problem rather exotic, so hopefully the pain caused by the compatibility break will be moderate. 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. > It seems that the whole point of deferring a trigger to commit > time is that the context that the trigger is executed in is different > than the context that it was triggered in. Tables may have changed, > permissions may have changed, session configuration variables may have > changed, roles may have changed, etc. So why should the executing role > be treated differently and restored to the value at the time of > triggering. Perhaps you can expand on why you feel that the current > behavior is wrong? True, somebody could change permissions or parameters between the DML statement and COMMIT, but that feels like external influences to me. Those changes would be explicit. 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 find these examples to be surprising, but not necessarily wrong > (as per my comment above). If someone wants the triggers to be executed > as the triggering role, then they can run `SET CONSTRAINTS ALL > IMMEDIATE`. If deferring a trigger to commit time and executing it as > the triggering role is desirable, then maybe we should add a modifier > to triggers that can control this behavior. Something like > `SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in > `CREATE FUNCTION`) that control which role is used. SECURITY INVOKER and SECURITY TRIGGERER seem too confusing. I would say that the triggerer is the one who invokes the trigger... It would have to be a switch like EXECUTE DEFERRED TRIGGER AS INVOKER|COMMITTER or so, but I think that special SQL syntax for this exotic corner case is a little too much. And then: is there anybody who feels that the current behavior is desirable? > Isaac Morland wrote: > > This looks to me like another reason that triggers should run as the > > trigger owner. Which role owns the trigger won’t change as a result of > > constraints being deferred or not, or any role setting done during the > > transaction, including that relating to security definer functions. > > +1, this approach would remove all of the surprising/wrong behavior and > in my opinion is more obvious. I'd like to add some more reasons why > this behavior makes sense: [...] > > However, I do worry that this is too much of a breaking change and > fundamentally changes how triggers work. Yes. This might be the right thing to do if we designed triggers as a new feature, but changing the behavior like that would certainly break a lot of cases. People who want that behavior use a SECURITY DEFINER trigger function. > > I skimmed the code and haven't looked at in depth. Whichever direction > we go, I think it's worth updating the documentation to make the > behavior around triggers and roles clear. I agree: adding a sentence somewhere won't hurt. I'll do that once the feedback has given me the feeling that I am on the right track. Yours, Laurenz Albe
On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe <laurenz.albe@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
> 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
Attachment
On Sat, Jun 8, 2024 at 2:37 PM Joseph Koshakow <koshy44@gmail.com> wrote:
Something like
`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.
I'm inclined toward this option (except invoker and triggerer are the same entity, we need owner|definer). I'm having trouble accepting changing the existing behavior here but agree that having a mode whereby the owner of the trigger/table executes the trigger function in an initially clean environment (server/database defaults; the owner role isn't considered as having logged in so their personalized configurations do not take effect) (maybe add a SET clause to create trigger too). Security invoker would be the default, retaining current behavior for upgrade/dump+restore.
Security definer on the function would take precedence as would its set clause.
David J.
On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
> except invoker and triggerer are the same entity
Maybe "executor" would have been a better term than 'invoker". In this
specific example they are not the same entity. The trigger is
triggered and queued by one role and executed by a different role,
hence the confusion. Though I agree with Laurenz, special SQL syntax
for this exotic corner case is a little too much.
> Security definer on the function would take precedence as would its set clause.
These trigger options seem a bit redundant with the equivalent options
on the function that is executed by the trigger. What would be the
advantages or differences of setting these options on the trigger
> except invoker and triggerer are the same entity
Maybe "executor" would have been a better term than 'invoker". In this
specific example they are not the same entity. The trigger is
triggered and queued by one role and executed by a different role,
hence the confusion. Though I agree with Laurenz, special SQL syntax
for this exotic corner case is a little too much.
> Security definer on the function would take precedence as would its set clause.
These trigger options seem a bit redundant with the equivalent options
on the function that is executed by the trigger. What would be the
advantages or differences of setting these options on the trigger
versus the function?
Thanks,
Joe Koshakow
On Sat, Jun 22, 2024 at 7:21 PM Joseph Koshakow <koshy44@gmail.com> wrote:
On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
> except invoker and triggerer are the same entity
Maybe "executor" would have been a better term than 'invoker". In this
specific example they are not the same entity. The trigger is
triggered and queued by one role and executed by a different role,
hence the confusion.
No matter what we label the keyword it would be represent the default and existing behavior whereby the environment at trigger resolution time, not trigger enqueue time, is used.
I suppose there is an argument for capturing and reapplying the trigger enqueue time environment and giving that a keyword as well. But fewer options has value and security definer seems like the strictly superior option.
Though I agree with Laurenz, special SQL syntax
for this exotic corner case is a little too much.
It doesn't seem like a corner case if we want to institute a new recommended practice that all triggers should be created with security definer. We seem to be discussing that without giving the dba a choice in the matter - but IMO we do want to provide the choice and leave the default.
> Security definer on the function would take precedence as would its set clause.
These trigger options seem a bit redundant with the equivalent options
on the function that is executed by the trigger. What would be the
advantages or differences of setting these options on the triggerversus the function?
At least security definer needs to take precedence as the function owner is fully expecting their role to be the one executing the function, not whomever the trigger owner might be.
If putting a set clause on the trigger is a thing then the same thing goes - the function author, if they also did that, expects their settings to be in place. Whether it really makes sense to have trigger owner set configuration when they attach the function is arguable but also the most flexible option.
David J.
On Sat, 2024-06-22 at 20:13 -0700, David G. Johnston wrote:
> [bikeshedding discussion about SQL syntax]
Sure, something like CREATE TRIGGER ... USING {INVOKER|CURRENT} ROLE
orsimilar would work, but think that this discussion is premature
at this point. If we have syntax to specify the behavior
of deferred triggers, that needs a new column in "pg_trigger", support
in pg_get_triggerdef(), pg_dump, pg_upgrade etc.
All that is possible, but keep in mind that we are talking about corner
case behavior. To the best of my knowledge, nobody has even noticed the
difference in behavior up to now.
I think that we should have some consensus about the following before
we discuss syntax:
- Does anybody depend on the current behavior and would be hurt if
my current patch went in as it is?
- Is this worth changing at all or had we better document the current
behavior and leave it as it is?
Concerning the latter, I am hoping for a detailed description of our
customer's use case some time soon.
Yours,
Laurenz Albe
I think that we should have some consensus about the following before
we discuss syntax:
- Does anybody depend on the current behavior and would be hurt if
my current patch went in as it is?
- Is this worth changing at all or had we better document the current
behavior and leave it as it is?
Concerning the latter, I am hoping for a detailed description of our
customer's use case some time soon.
We have a few choices then:
1. Status quo + documentation backpatch
2. Change v18 narrowly + documentation backpatch
3. Backpatch narrowly (one infers the new behavior after reading the existing documentation)
4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow change to fix the POLA violation
I've been presenting option 4.
Pondering further, I see now that having the owner-execution mode be the only way to avoid the POLA violation in deferred triggers isn't great since many triggers benefit from the implied security of being able to run in the invoker's execution context - especially if the trigger doesn't do anything that PUBLIC cannot already do.
So, I'm on board with option 2 at this point.
David J.
On Wed, 2024-06-26 at 07:38 -0700, David G. Johnston wrote: > We have a few choices then: > 1. Status quo + documentation backpatch > 2. Change v18 narrowly + documentation backpatch > 3. Backpatch narrowly (one infers the new behavior after reading the existing documentation) > 4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow change to fix the POLA violation > > I've been presenting option 4. > > Pondering further, I see now that having the owner-execution mode be the only way to avoid > the POLA violation in deferred triggers isn't great since many triggers benefit from the > implied security of being able to run in the invoker's execution context - especially if > the trigger doesn't do anything that PUBLIC cannot already do. > > So, I'm on board with option 2 at this point. Nice. I think we can safely rule out option 3. Even if it is a bug, it is not one that has bothered anybody so far that a backpatch is indicated. Yours, Laurenz Albe