Thread: Allow GRANT TRIGGER privilege to DROP TRIGGER (Re: Bug ##7716)
I'd reported a few years ago what seemed an inconsistency with the TRIGGER permission on tables where it allows a role to create a trigger but not to drop it. I don't have the original email to reply directly to the thread anymore, but I've moved on to actually trying to fix it myself, hence sending to this list instead.
Original thread - http://www.postgresql.org/message-id/E1TeaD4-0004le-Vd@wrigleys.postgresql.org
I found RemoveTriggerById() in backend/commands/trigger.c which seems to be the code that actually drops the trigger. And I see in CreateTrigger() above it how to use pg_class_aclcheck() to check for valid permissions, so I was going to see about adding that code to the Remove section. However, I see no code in Remove for checking for object ownership, so I'm not sure how that is being enforced currently which would also have to be adjusted. I see down in RangeVarCallbackForRenameTrigger() that it uses pg_class_ownercheck() to do so, but can't find where that is being done for dropping a trigger. Original thread - http://www.postgresql.org/message-id/E1TeaD4-0004le-Vd@wrigleys.postgresql.org
I've tried tracing the function calls in RemoveTriggerById() to see if one of them is doing the owner check, but I haven't been able to see it. The only other reference to RemoveTriggerById() I can find is in backend/catalog/dependency.c and I don't see the check there either.
Keith Fiske <keith@omniti.com> writes: > I found RemoveTriggerById() in backend/commands/trigger.c which seems to > be the code that actually drops the trigger. And I see in CreateTrigger() > above it how to use pg_class_aclcheck() to check for valid permissions, so > I was going to see about adding that code to the Remove section. However, I > see no code in Remove for checking for object ownership, so I'm not sure > how that is being enforced currently which would also have to be adjusted. An easy way to find the code involved in any error report is to set a breakpoint at errfinish() and then provoke the error. In this case I get #0 errfinish (dummy=0) at elog.c:410 #1 0x00000000004f407f in aclcheck_error (aclerr=<value optimized out>, objectkind=ACL_KIND_CLASS, objectname=0x7f39db129d68"bobstable") at aclchk.c:3374 #2 0x00000000004fc8e4 in check_object_ownership (roleid=207490, objtype=OBJECT_TRIGGER, address=..., objname=0x1e301e0, objargs=<value optimized out>, relation=0x7f39db129b50) at objectaddress.c:1160 #3 0x000000000055ba5d in RemoveObjects (stmt=0x1e30218) at dropcmds.c:123 #4 0x00000000006a2540 in ExecDropStmt (stmt=0x1e30218, isTopLevel=<value optimized out>) at utility.c:1370 #5 0x00000000006a2d93 in ProcessUtilitySlow (parsetree=0x1e30218, queryString=0x1e2f728 "drop trigger insert_oid on bobstable;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, dest=<value optimized out>, completionTag=0x7fff3ad9ed90"") at utility.c:1301 #6 0x00000000006a370a in standard_ProcessUtility (parsetree=0x1e30218, queryString=0x1e2f728 "drop trigger insert_oidon bobstable;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, dest=0x1e30670, completionTag=0x7fff3ad9ed90"") at utility.c:793 #7 0x00000000006a3837 in ProcessUtility (parsetree=<value optimized out>, queryString=<value optimized out>, context=<valueoptimized out>, params=<value optimized out>, dest=<value optimized out>, completionTag=<value optimizedout>) at utility.c:311 ... A look at check_object_ownership suggests that you could take the TRIGGER case out of the generic relation path and make it a special case that allows either ownership or TRIGGER permission. TBH, though, I'm not sure this is something to pursue. We discussed all this back in 2006. As I pointed out at the time, giving somebody TRIGGER permission is tantamount to giving them full control of your account: http://www.postgresql.org/message-id/21827.1166115978@sss.pgh.pa.us because they can install a trigger that will execute arbitrary code with *your* privileges the next time you modify that table. I think we should get rid of the separate TRIGGER privilege altogether, not make it an even bigger security hole. regards, tom lane
On Wed, Jul 16, 2014 at 07:45:56PM -0400, Tom Lane wrote: > A look at check_object_ownership suggests that you could take the TRIGGER > case out of the generic relation path and make it a special case that > allows either ownership or TRIGGER permission. > > TBH, though, I'm not sure this is something to pursue. We discussed all > this back in 2006. As I pointed out at the time, giving somebody TRIGGER > permission is tantamount to giving them full control of your account: > http://www.postgresql.org/message-id/21827.1166115978@sss.pgh.pa.us > because they can install a trigger that will execute arbitrary code with > *your* privileges the next time you modify that table. > > I think we should get rid of the separate TRIGGER privilege altogether, > not make it an even bigger security hole. Uh, how does removing a trigger cause a larger security hole? As long as users can create triggers, removal seems logical. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Wed, Jul 16, 2014 at 07:45:56PM -0400, Tom Lane wrote:
>> I think we should get rid of the separate TRIGGER privilege altogether,
>> not make it an even bigger security hole.
> Uh, how does removing a trigger cause a larger security hole? As long
> as users can create triggers, removal seems logical.
It's bigger in the sense that you can not only add arbitrary actions,
but remove actions that the table owner intended to have happen.
For example, the ability to temporarily suppress entries in a logging
table (by dropping the trigger that makes them, and then putting the
trigger back later to cover one's tracks) could be of considerable use
to a black hat.
regards, tom lane