Re: Insufficient attention to security in contrib (mostly) - Mailing list pgsql-hackers

Tom,

> dblink_get_pkey() will tell you what its primary key is.  This is not
> a big deal seeing that you can get the info by looking into the system
> catalogs, but someone who was trying to lock down the system catalogs
> wouldn't be happy.  I think this should require SELECT privilege on
> the table.

+1

> btreefuncs.c is a security hole a mile wide: it will happily dump the
> entire data content of an index for you.  It's a good thing this hasn't
> shipped in any release yet.  While we could possibly make it look up
> the index's parent table and check if you have SELECT privilege on
> that, it'd be easier just to make the functions demand superuser
> privilege, which is what the rest of the functions in this contrib
> module require.  Comments?

Superuser only should be fine.  Heck, I don't know anyone who actually uses 
this function ...

> pgrowlocks tells you about row lock states, which maybe is not that
> interesting for security, but still it's information that one wouldn't
> expect to be exposed to someone who isn't allowed to read the table.
> I suppose knowing the number of live tuples might in itself be
> sensitive information.

Here I think the advantage of being able to run this as a non-superuser 
(and thus not have the superuser password on the client machine) outweighs 
any data which can be reverse-engineered from the lock information.

> If you do think that's sensitive information, you probably won't be
> happy that pgstattuple and pgstatindex likewise have no permission
> checks.  The same goes for the various size-calculation functions in
> the backend's adt/dbsize.c file.  The latter should probably require
> superuser, particularly for the functions that are willing to tell
> you about whole databases you can't get into.

Hmmm, we can't really require anything greater than SELECT permission for 
dbsize.  Remember that every commonly-used function/utility which requires 
superuser or object owner status is an invitation to put the super or 
owner password on the client machine (or, worse, to run the whole 
application as the DB owner), or to create SECURITY DEFINER functions, 
both of which then become potential security holes much larger than the 
issue we were trying to fix.  

One of the things which makes PostgreSQL secure is the ability to run an 
entire application with extremely restricted permissions on the client 
side.  This is something which MySQL and SQL Server completely fail to do, 
and why we are much more secure than they are.  Don't break it!

So I think dbsize needs to run with the least reasonable security privs, 
namely SELECT.  I'd agree that it needs SELECT, though; you don't want 
someone who doesn't have rights to see a table getting an approximate 
rowcount.

> currtid_byrelname() and currtid_byreloid() have no permission checks
> either.  I'm not sure that TID relationships can be used for anything
> interesting, but ...

Hmmm.  I'm not even sure how to use these.  Will poke around.

> Lastly, int4notin() and oidnotin() have no permission checking, which
> means you can find out whether specific values are or are not present
> in an int4 or oid column you shouldn't read.  This code is so old,
> crufty, and undocumented that I'm strongly inclined to remove it
> instead of fix it --- it really has no excuse to live when we support
> IN (sub-SELECT) constructs.

+1 for removal.

-- 
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco