Re: SECURITY DEFINER not being propagated... - Mailing list pgsql-patches

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