Re: Wrong security context for deferred triggers?

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