Skip site navigation (1) Skip section navigation (2)

Re: SECURITY DEFINER not being propagated...

From: Sean Chittenden <sean(at)chittenden(dot)org>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: SECURITY DEFINER not being propagated...
Date: 2004-04-29 23:00:42
Message-ID: 0A26A236-9A31-11D8-B5D6-000A95C705DC@chittenden.org (view raw or flat)
Thread:
Lists: 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


Attachment: patch.txt
Description: text/plain (2.3 KB)

In response to

pgsql-patches by date

Next:From: Bruce MomjianDate: 2004-04-30 00:02:06
Subject: Re: subtransactions -- storage manager
Previous:From: Simon RiggsDate: 2004-04-29 22:38:52
Subject: Re: subtransactions -- storage manager

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group