Thread: SECURITY DEFINER not being propagated...

This one's simple enough to reproduce (see SQL script below), but,
there are some comments in src/backend/catalog/namespace.c that seem
questionable and incorrect:


## BEGIN ##
         /*
          * 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.
          */
## END ##


Suppose the following example:

*) PUBLIC has TEMP privs revoked from the test database.
*) DBAs have TEMP privs on the test database.
*) A script is written by the DBA with SECURITY DEFINER that CREATEs a
TEMP TABLE, populates it, and REVOKEs all privs but SELECT from the
session user.

With the current logic, it's impossible to achieve this without
granting temp table privs to all users.  The attached patch changes
things from GetSessionUserID() to GetUserId().  Comments?  I think the
reasoning and rationale in the comments hinders a DBAs ability to
secure a database.


-- BEGIN EXAMPLE SQL SCRIPT
REVOKE ALL PRIVILEGES ON DATABASE test FROM PUBLIC CASCADE;
GRANT CREATE,TEMPORARY ON DATABASE test TO dba;
GRANT USAGE ON SCHEMA public TO PUBLIC;

CREATE OR REPLACE FUNCTION create_tmptbl()
         RETURNS BOOL
         AS 'BEGIN
         PERFORM TRUE FROM pg_catalog.pg_class c WHERE c.relkind =
''r''::CHAR(1) AND c.relname = ''tmptbl''::TEXT;
         IF NOT FOUND THEN
                 EXECUTE ''CREATE LOCAL TEMP TABLE tmptbl (
                         i INT
                 ) WITHOUT OIDS ON COMMIT DELETE ROWS;'';
         END IF;

         RETURN TRUE;
END;' LANGUAGE 'plpgsql';
REVOKE ALL PRIVILEGES ON FUNCTION create_tmptbl() FROM PUBLIC CASCADE;

CREATE OR REPLACE FUNCTION setuid_wrapper()
         RETURNS BOOL
         AS 'BEGIN
         RETURN create_tmptbl();
END;' LANGUAGE 'plpgsql'
         SECURITY DEFINER;
REVOKE ALL PRIVILEGES ON FUNCTION setuid_wrapper() FROM PUBLIC CASCADE;
GRANT EXECUTE ON FUNCTION setuid_wrapper() TO PUBLIC;
-- END SCRIPT

test=# \c test usr
You are now connected to database "test" as user "usr".
test=> SELECT setuid_wrapper();
ERROR:  permission denied to create temporary tables in database "test"
CONTEXT:  SQL query "CREATE LOCAL TEMP TABLE tmptbl (
                         i INT
                 ) WITHOUT OIDS ON COMMIT DELETE ROWS;"
PL/pgSQL function "create_tmptbl" line 4 at execute statement
PL/pgSQL function "setuid_wrapper" line 2 at return




--
Sean Chittenden

Sean Chittenden <sean@chittenden.org> writes:
> This one's simple enough to reproduce (see SQL script below), but,
> there are some comments in src/backend/catalog/namespace.c that seem
> questionable and incorrect:

The proposed patch reverts a change deliberately applied in namespace.c
rev 1.15 (4/29/02).  I think you need to go back and consult the schema
privilege discussions that occurred just before that; I'm much too tired
to do so myself right at the moment ...

            regards, tom lane

>> This one's simple enough to reproduce (see SQL script below), but,
>> there are some comments in src/backend/catalog/namespace.c that seem
>> questionable and incorrect:
>
> The proposed patch reverts a change deliberately applied in namespace.c
> rev 1.15 (4/29/02).  I think you need to go back and consult the schema
> privilege discussions that occurred just before that; I'm much too
> tired
> to do so myself right at the moment ...

I can see that it was done in rev 1.15, but I haven't seen any
discussion that suggests that it was deliberate beyond what's in the
comment... but that's lacking rationale, IMHO.  The thread that I think
you're referring to begins here:

http://archives.postgresql.org/pgsql-hackers/2002-04/msg01035.php

But here's pretty much the only relevant thread:

http://archives.postgresql.org/pgsql-hackers/2002-04/msg01191.php

But it doesn't have a conclusion, synopsis, or any agreement that comes
close to, "here's why we check the perms for the session user and not
the current user."  Having the permissions for CREATE TEMP TABLE check
on the session user defeats the purpose of having functions run as
SECURITY DEFINER.

Without any rationale as to why CREATE TEMP TABLEs checks the session
user in the archives, could we open this up for discussion again?  To
me, it seems to fly directly in the face of a function running as
SECURITY DEFINER.  At the moment, this behavior cripples the usefulness
of having a TEMP table be used as a trusted cache for data.

-sc

--
Sean Chittenden

Sean Chittenden <sean@chittenden.org> 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

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

-sc


--
Sean Chittenden

Sean Chittenden <sean@chittenden.org> 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

>> 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;
         }
# END

Which 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



--
Sean Chittenden