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):
Not tested in the slightest, actually. The attached has been, however is commented and tested.
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;
Yup, just figured that out.
test=> SET search_path = pg_temp_2;
test=> \dt
List of relations
Schema | Name | Type | Owner
-----------+--------+-------+-------
pg_temp_2 | tmptbl | table | dba
(1 row)test=> CREATE sequence tmp_seq;
test=> \ds
List of relations
Schema | Name | Type | Owner
-----------+---------+----------+-------
pg_temp_2 | tmp_seq | sequence | nxad
(1 row):-/ Which leads to a different problem with error reporting. I've changed pg_namespace_aclcheck() to the following:
# BEGIN
if (isTempNamespace(nsp_oid)) {
if (pg_database_aclcheck(MyDatabaseId, GetUserId(),
ACL_CREATE_TEMP) == ACLCHECK_OK)
return ACLCHECK_OK;
else
return ACLCHECK_NO_PRIV;
}
# ENDWhich works alright, but I'm worried you'll think it will lead the user astray if they don't have TEMP privs on the database and set their search path to an already existing be pg_temp_%d (created by a user who does have TEMP privs). I think it's reasonably easy to justify the confusion in that most users will be smacked with the 'permission denied to create temporary tables in database "test"' message when they try CREATE TEMP TABLE foo(i INT); since most users won't be using a FUNCTION to create temp tables.
(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.
With the aclcheck now moved into pg_namespace_aclcheck() - which gets used everywhere already - I would think this would be as secure as what we've got right now. For a bit I was concerned about a user and superuser creating a temp table with the same name and thought about creating a temp schema for each backend and each user, but backed off from that because it would add a fair amount of complexity.
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.
Correct. I don't have to REVOKE TEMP privs in order to store info across transactions. I do need to REVOKE TEMP privs to reduce unauthorized users from creating temp tables and filling up my disk. I know I could simply, "not allow unauthorized users/clients from accessing your database," (I do that on 99/100 of my databases, but in this case I can't/don't want to) but I've got a device that runs PostgreSQL and is secured to the point that I've opened it up for public connections without fear of information loss to unauthorized parties. -sc
Post patch:
test=> SHOW search_path;
search_path
-------------------
pg_temp_2, public
(1 row)test=> SELECT public.setuid_wrapper(); -- Create's the temp table && schema
setuid_wrapper
----------------
t
(1 row)
test=> \dt
List of relations
Schema | Name | Type | Owner
-----------+--------+-------+-------
pg_temp_2 | tmptbl | table | sean
(1 row)test=> CREATE SEQUENCE tmp_seq;
CREATE SEQUENCE
test=> \ds
List of relations
Schema | Name | Type | Owner
--------+-----------+----------+-------
public | tmp_seq | sequence | nxad
(1 row)test=> CREATE SEQUENCE pg_temp_2.tmp2_seq;
ERROR: permission denied for schema pg_temp_2
test=> CREATE FUNCTION pg_temp_2.foo() RETURNS BOOL AS 'BEGIN RETURN TRUE; END;' LANGUAGE 'plpgsql';
ERROR: permission denied for schema pg_temp_2
My only gripe about what I've is with the wording in the following use case:
## BEGIN -- The schema pg_temp_2 doesn't exist right now. test=> SHOW search_path ; search_path ------------- pg_temp_2 (1 row)
test=> CREATE sequence foo_seq;
ERROR: no schema has been selected to create in
test=> SELECT public.setuid_wrapper(); -- This function creates a temp namespace && table
setuid_wrapper
----------------
t
(1 row)
test=> CREATE sequence foo_seq; ERROR: no schema has been selected to create in ## END
Ideally the wording should be different (ie, "can't create an object in a temp namespace without TEMP permissions"), but in most cases people won't use pg_temp_%d as the only namespace in their search_path so I don't think this is a big issue. Please feel free to editorialize the comments. I don't think anything is needed in namespace.c now that things work as expected, but there's something there. I also moved the check below the superuser check. Comments? I think this is ready to be committed. -sc
Index: src/backend/catalog/aclchk.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/catalog/aclchk.c,v
retrieving revision 1.97
diff -u -r1.97 aclchk.c
--- src/backend/catalog/aclchk.c 14 Jan 2004 03:44:53 -0000 1.97
+++ src/backend/catalog/aclchk.c 29 Apr 2004 22:57:43 -0000
@@ -1299,16 +1299,26 @@
bool isNull;
Acl *acl;
- /*
- * 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;
-
/* Superusers bypass all permission checking. */
if (superuser_arg(userid))
return ACLCHECK_OK;
+
+ /*
+ * If we have been assigned this namespace as a temp
+ * namespace, check to make sure we have CREATE permissions on
+ * the database.
+ *
+ * Instead of returning ACLCHECK_NO_PRIV, should we return via
+ * ereport() with a message about trying to create an object
+ * in a TEMP namespace when GetUserId() doesn't have perms?
+ */
+ if (isTempNamespace(nsp_oid)) {
+ if (pg_database_aclcheck(MyDatabaseId, GetUserId(),
+ ACL_CREATE_TEMP) == ACLCHECK_OK)
+ return ACLCHECK_OK;
+ else
+ return ACLCHECK_NO_PRIV;
+ }
/*
* Get the schema's ACL from pg_namespace
Index: src/backend/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
--- src/backend/catalog/namespace.c 13 Feb 2004 01:08:20 -0000 1.63
+++ src/backend/catalog/namespace.c 29 Apr 2004 22:57:44 -0000
@@ -1640,11 +1640,11 @@
* 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.
+ * ACL_CREATE_TEMP perms are also checked in
+ * pg_namespace_aclcheck() that way only users who have TEMP
+ * perms can create objects.
*/
- if (pg_database_aclcheck(MyDatabaseId, GetSessionUserId(),
+ if (pg_database_aclcheck(MyDatabaseId, GetUserId(),
ACL_CREATE_TEMP) !=
ACLCHECK_OK)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-- Sean Chittenden
---------------------------(end of broadcast)--------------------------- TIP 7: don't forget to increase your free space map settings
