Re: Wrong security context for deferred triggers?

[Laurenz Albe]

On 7/8/24 04:14, Joseph Koshakow wrote:

Given the above and the fact that the patch is a breaking change, my
vote would still be to keep the current behavior and update the
documentation. Though I'd be happy to be overruled by someone with more
knowledge of triggers.


Thanks for that feedback.
Based on that, the patch should be rejected.

Since there were a couple of other opinions early in the thread, I'll let
it sit like that for now, and judgement can be passed at the end of the
commitfest.  Perhaps somebody else wants to chime in.

Yours,
Laurenz Albe

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

On Mon, Jul 1, 2024 at 11:45 AM Laurenz Albe 
wrote:

> I asked them for a statement, and they were nice enough to write up
> https://postgr.es/m/e89e8dd9-7143-4db8-ac19-b2951cb0c0da%40gmail.com

> They have a workaround, so the patch is not absolutely necessary for them.

It sounds like the issue is that there is there is a constraint
trigger to check a table constraint that must be executed at commit
time, and we'd like to guarantee that if the triggering action was
successful, then the constraint check is also successful. This is an
even bigger issue for transactions that have multiple of these
constraint checks where there may be no single role that has the
privileges required to execute all checks.

Your patch would fix the issue in a majority of cases, but not all.
Since INSERT, UPDATE, DELETE privileges don't necessarily imply SELECT
privileges, the role that modifies a table doesn't necessarily have the
privileges required to check the constraints. It sounds like creating
the constraint check triggers as a security definer function, with a
role that has SELECT privileges, is the more complete solution rather
than a workaround.

Given the above and the fact that the patch is a breaking change, my
vote would still be to keep the current behavior and update the
documentation. Though I'd be happy to be overruled by someone with more
knowledge of triggers.

Thanks,
Joe Koshakow

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

On Sat, 2024-06-22 at 17:50 -0400, Joseph Koshakow wrote:
> On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe  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 definitely see your point, although GUC settings and the current
security context are something different.

It would definitely not be viable to put all GUC values in the trigger
state.

So if you say "all or nothing", it would be nothing, and the patch should
be rejected.

> > 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.

I asked them for a statement, and they were nice enough to write up
https://postgr.es/m/e89e8dd9-7143-4db8-ac19-b2951cb0c0da%40gmail.com

They have a workaround, so the patch is not absolutely necessary for them.

> 
> 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?

If that is a concern (and I can envision it to be), I can improve that.
One option is to copy the guts of GetUserNameFromId(), and another
is to factor out the common parts into a new function.

I'd wait until we have a decision whether we want the patch or not
before I make the effort, if that's ok.

> Everything else looked good, and the code does what it says it will.

Thanks for the review!

Yours,
Laurenz Albe

Re: Wrong security context for deferred triggers?

[Bennie Swart]

Hi,

Allow me to provide some background on how we came across this.

(This is my first time posting to a pgsql list so hopefully I've got 
everything set up correctly.)


We have a db with a big legacy section that we're in the process of 
modernizing. To compensate for some of the shortcomings we have designed 
a layer of writable views to better represent the underlying data and 
make working with it more convenient. The following should illustrate 
what we're doing:


    -- this is the schema containing the view layer.
    create schema api;
    -- and this user is granted access to the api, but not the rest of 
the legacy db.

    create role apiuser;
    grant usage on schema api to apiuser;

    -- some dummy objects in the legacy db - poorly laid out and poorly 
named.

    create schema legacy;
    create table legacy.stock_base (
    id serial primary key
      , lbl text not null unique
      , num int not null
      -- etc
    );
    create table legacy.stock_extra (
    id int not null unique references legacy.stock_base (id)
      , man text
      -- etc
    );

    -- create the stock view which better names and logically groups 
the data.

    create view api.stock as
      select sb.id
       , sb.lbl as description
       , sb.num as quantity
       , se.man as manufacturer
    from legacy.stock_base sb
      left join legacy.stock_extra se using (id);
    -- make it writable so it is easier to work with. use security 
definer to allow access to legacy sections.
    create function api.stock_cud() returns trigger language plpgsql 
security definer as $$

    begin
    -- insert/update legacy.stock_base and legacy.stock_extra 
depending on trigger action, modified fields, etc.

    assert tg_op = 'INSERT'; -- assume insert for example's sake.
    insert into legacy.stock_base (lbl, num) values 
(new.description, new.quantity) returning id into new.id;
    insert into legacy.stock_extra (id, man) values (new.id, 
new.manufacturer);

    return new;
    end;
    $$;
    create trigger stock_cud
    instead of insert or update or delete on api.stock
    for each row execute function api.stock_cud();

    -- grant the apiuser permission to work with the view.
    grant insert, update, delete on api.stock to apiuser;

    -- insert as superuser - works as expected.
    insert into api.stock (description, quantity, manufacturer) values 
('item1', 10, 'manufacturer1');

    -- insert as apiuser - works as expected.
    set role apiuser;
    insert into api.stock (description, quantity, manufacturer) values 
('item2', 10, 'manufacturer2');


In some cases there are constraint triggers on the underlying tables to 
validate certain states. It is, however, possible for a state to be 
temporarily invalid between statements, so long as it is valid at tx 
commit. For this reason the triggers are deferred by default. Consider 
the following example:


    reset role;
    create function legacy.stock_check_state() returns trigger language 
plpgsql as $$

    begin
    -- do some queries to check the state of stock based on 
modified rows and error if invalid.

    raise notice 'current_user %', current_user;
    -- dummy validation code.
    perform * from legacy.stock_base sb left join 
legacy.stock_extra se using (id) where sb.id = new.id;

    return new;
    end;
    $$;
    create constraint trigger stock_check_state
    after insert or update or delete on legacy.stock_base
    deferrable initially deferred
    for each row execute function legacy.stock_check_state();

Then repeat the inserts:

    -- insert as superuser - works as expected.
    reset role;
    insert into api.stock (description, quantity, manufacturer) values 
('item3', 10, 'manufacturer3'); -- NOTICE:  current_user postgres

    -- insert as apiuser - fails.
    set role apiuser;
    insert into api.stock (description, quantity, manufacturer) values 
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user apiuser


    -- insert as apiuser, not deferred - works as expected.
    begin;
    set constraints all immediate;
    insert into api.stock (description, quantity, manufacturer) values 
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user postgres

    commit;

The insert as apiuser fails when the constraint trigger is deferred, but 
works as expected when it is immediate.


Hopefully this helps to better paint the picture. Our workaround 
currently is to just make `legacy.stock_check_state()` security definer 
as well. As I told Laurenz, we're not looking to advocate for any 
specific outcome here. We noticed this strange behaviour and thought it 
to be a bug that should be fixed - whatever "fixed" ends up meaning.


Regards,

Bennie Swart


On 2024/06/26 17:53, Laurenz Albe wrote:

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 + 

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

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

Re: Wrong security context for deferred triggers?

[David G. Johnston]

On Wed, Jun 26, 2024 at 2:02 AM Laurenz Albe 
wrote:

>
> 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.

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

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

Re: Wrong security context for deferred triggers?

[David G. Johnston]

On Sat, Jun 22, 2024 at 7:21 PM Joseph Koshakow  wrote:

> On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <
> david.g.johns...@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 trigger
> versus 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.

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <
david.g.johns...@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
versus the function?

Thanks,
Joe Koshakow

Re: Wrong security context for deferred triggers?

[David G. Johnston]

On Sat, Jun 8, 2024 at 2:37 PM Joseph Koshakow  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.

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe 
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 
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
--- 

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

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 

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

On Sat, Jun 8, 2024 at 10:13 PM Isaac Morland 
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

Re: Wrong security context for deferred triggers?

[Isaac Morland]

On Sat, 8 Jun 2024 at 17:37, Joseph Koshakow  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.

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

On Sat, Jun 8, 2024 at 5:36 PM Joseph Koshakow  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
ignore this.

Thanks,
Joe Koshakow

Re: Wrong security context for deferred triggers?

[Joseph Koshakow]

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;
 

Re: Wrong security context for deferred triggers?

[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
From 71530f35ab003846d73f9444737e40557598d0f2 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Wed, 6 Mar 2024 14:09:43 +0100
Subject: [PATCH v1] 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 c344ff0944..68c89f1958 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3640,6 +3640,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 */
@@ -4121,6 +4122,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)
@@ -4295,6 +4297,8 @@ AfterTriggerExecute(EState *estate,
 	int			tgindx;
 	bool		should_free_trig = false;
 	bool		should_free_new = false;
+	Oid			save_rolid;
+	int			

Re: Wrong security context for deferred triggers?

[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)?

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

Re: Wrong security context for deferred triggers?

[Tomas Vondra]

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

Re: Wrong security context for deferred triggers?

[Isaac Morland]

On Mon, 6 Nov 2023 at 11:58, Laurenz Albe  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.

Re: Wrong security context for deferred triggers?

[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

Wrong security context for deferred triggers?

[Laurenz Albe]

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