Re: [PATCHES] SECURITY DEFINER not being propagated...

2004-04-29 Thread Sean Chittenden
Without any rationale as to why CREATE TEMP TABLEs checks the session
user in the archives, could we open this up for discussion again?
Well, let me put it this way: if you want to change the behavior you're
going to have to work much harder than just reverting the prior patch.
IIRC the fundamental reason the code works that way is that
InitTempTableNamespace is done only once per session.  If it checks
against current_user rather than session_user then (a) the results will
be inconsistent, and (b) you create a different sort of security hole,
which is that if a setuid function is the first to try to create a temp
table in a particular session, then not-so-privileged functions will
still be able to create temp tables later in the session.
Hrm, I didn't realize that: points taken.  Since temp schemas are 
always owned by the superuser, why aren't the ACL checks done when the 
temp relation is created as opposed to when the schema is created?  I 
see what you're saying about things currently needing to use 
GetSessionuserId() instead of GetUserId(), but if a check for istemp is 
pushed down into DefineRelation(), then (from what I can tell) 
GetUserId() can be used in InitTempTableNamespace().  Object owners can 
only delete their objects, the temp schema can't be deleted as is 
because its owner is the superuser.

I think the attached patch addresses both of your concerns.  Things are 
consistent in that SECURITY DEFINER/TEMP TABLEs will now work as 
expected.  The security hole isn't an issue because security checks are 
applied both in InitTempTableNamespace() and DefineRelation().

At the moment, this behavior cripples the usefulness
of having a TEMP table be used as a trusted cache for data.
What exactly do you think makes a temp table suitable as a trusted
cache?  Or more suitable than non-temp tables?
Revoke all privs on temp tables except from the DBA, then setup 
functions to use the temp table as a way of maintaining state 
information across transactions (within the same session).  It's a hack 
to get around the lack of server side variables.  In many ways, 
actually, it works out better because I can wrap functions or 
PostgreSQL's permissions around the temp relations and get exactly the 
access that I need... far more fine grained than anything I could do 
with a GUC or some other server side MIB/variable implementation.

I don't really believe in the notion of restricting temp table creation
to setuid functions.  AFAICS the only reason for forbidding temp table
creation is to prevent a session from using any on-disk resources, and
that hardly works if it can still do so via calling setuid functions.
It can't populate or read data out of the temp relation though, which 
is ideal for my situation.

-scIndex: catalog/namespace.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/catalog/namespace.c,v
retrieving revision 1.63
diff -u -r1.63 namespace.c
--- catalog/namespace.c 13 Feb 2004 01:08:20 -  1.63
+++ catalog/namespace.c 29 Apr 2004 07:33:49 -
@@ -1639,12 +1639,8 @@
 * First, do permission check to see if we are authorized to make temp
 * tables.  We use a nonstandard error message here since
 * databasename: permission denied might be a tad cryptic.
-*
-* Note we apply the check to the session user, not the currently active
-* userid, since we are not going to change our minds about temp table
-* availability during the session.
 */
-   if (pg_database_aclcheck(MyDatabaseId, GetSessionUserId(),
+   if (pg_database_aclcheck(MyDatabaseId, GetUserId(),
 ACL_CREATE_TEMP) != 
ACLCHECK_OK)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
Index: commands/tablecmds.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/commands/tablecmds.c,v
retrieving revision 1.102
diff -u -r1.102 tablecmds.c
--- commands/tablecmds.c1 Apr 2004 21:28:44 -   1.102
+++ commands/tablecmds.c29 Apr 2004 07:33:49 -
@@ -165,8 +165,12 @@
{
AclResult   aclresult;
 
-   aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(),
- 
ACL_CREATE);
+   if (stmt-relation-istemp)
+ aclresult = pg_namespace_aclcheck(MyDatabaseId, GetUserId(),
+   ACL_CREATE_TEMP);
+   else
+ aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(),
+   ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,

Re: [PATCHES] SECURITY DEFINER not being propagated...

2004-04-29 Thread Tom Lane
Sean Chittenden [EMAIL PROTECTED] writes:
 I think the attached patch addresses both of your concerns.

Perhaps something like this will work, but the patch as given can't
possibly be right (or have been tested with any care):

 +   aclresult = pg_namespace_aclcheck(MyDatabaseId, GetUserId(),
 + ACL_CREATE_TEMP);

Surely that should be pg_database_aclcheck() ... and the error reporting
code just below won't be very appropriate for this case, either.

Also a comment or five would be appropriate.

A larger problem is that the reason that control makes it through that
path at the moment is this check in pg_namespace_aclcheck:

/*
 * If we have been assigned this namespace as a temp namespace, assume
 * we have all grantable privileges on it.
 */
if (isTempNamespace(nsp_oid))
return ACLCHECK_OK;

(Since the temp namespace is created as owned by the superuser, ordinary
users would always fail to create temp tables without this escape hatch.)
I am not at all convinced that this check could be removed, but I also
wonder whether its presence doesn't create some issues that are security
holes if we adopt your definition of how temp table creation ought to
behave.

 At the moment, this behavior cripples the usefulness
 of having a TEMP table be used as a trusted cache for data.
 
 What exactly do you think makes a temp table suitable as a trusted
 cache?  Or more suitable than non-temp tables?

 Revoke all privs on temp tables except from the DBA, then setup 
 functions to use the temp table as a way of maintaining state 
 information across transactions (within the same session).

This is pretty much a non-argument, as there is no part of it that says
that you have to revoke the right to create temp tables from Joe User.
What is necessary and sufficient is that the particular temp table you
want to keep your info in has to be owned by, and only accessible to,
the more-privileged account.  You need not muck with the temp namespace
behavior before you can do that.

regards, tom lane

---(end of broadcast)---
TIP 9: the planner will ignore your desire to choose an index scan if your
  joining column's datatypes do not match


Re: [PATCHES] SECURITY DEFINER not being propagated...

2004-04-28 Thread Tom Lane
Sean Chittenden [EMAIL PROTECTED] writes:
 Without any rationale as to why CREATE TEMP TABLEs checks the session 
 user in the archives, could we open this up for discussion again?

Well, let me put it this way: if you want to change the behavior you're
going to have to work much harder than just reverting the prior patch.

IIRC the fundamental reason the code works that way is that
InitTempTableNamespace is done only once per session.  If it checks
against current_user rather than session_user then (a) the results will
be inconsistent, and (b) you create a different sort of security hole,
which is that if a setuid function is the first to try to create a temp
table in a particular session, then not-so-privileged functions will
still be able to create temp tables later in the session.

 At the moment, this behavior cripples the usefulness 
 of having a TEMP table be used as a trusted cache for data.

What exactly do you think makes a temp table suitable as a trusted
cache?  Or more suitable than non-temp tables?

I don't really believe in the notion of restricting temp table creation
to setuid functions.  AFAICS the only reason for forbidding temp table
creation is to prevent a session from using any on-disk resources, and
that hardly works if it can still do so via calling setuid functions.

regards, tom lane

---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faqs/FAQ.html