Re: Insufficient attention to security in contrib (mostly)

I noticed a number of functions, most but not all in contrib, which
allow an unprivileged user to obtain varying amounts of information
about a relation he has no permissions to access:

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.

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?

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.

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.

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

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.

Comments?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

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

Wow, those are strange beasts.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

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

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> 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.

I have no objection to knocking this down to demanding only SELECT privs
on the table. It's hard to think that it is OK to be totally unsecured.

> Hmmm, we can't really require anything greater than SELECT permission for
> dbsize.

That's OK for individual tables, but we have no equivalent concept for
whole databases or tablespaces. What do you propose for them?

regards, tom lane

Tom,

> That's OK for individual tables, but we have no equivalent concept for
> whole databases or tablespaces. What do you propose for them?

CONNECT, of course. There are no permissions for tablespaces except
CREATE, so presumably if the user can connect to the db, they ought to be
able to see the size of the tablespaces.

Although it seems like SCHEMA USAGE ought to fit in there somewhere, I just
don't know how.

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Tom,
>> That's OK for individual tables, but we have no equivalent concept for
>> whole databases or tablespaces. What do you propose for them?

> CONNECT, of course. There are no permissions for tablespaces except
> CREATE, so presumably if the user can connect to the db, they ought to be
> able to see the size of the tablespaces.

Uh, if you agree that a user shouldn't be able to find out the size of a
table he can't read, why is it OK to find out the size of the database
or tablespace containing it? In typical cases where there's just one or
a few large tables, the latter number would go far towards giving you
the former. For instance, if you are allowed to access all but one
table in a DB, subtraction will give you its size exactly. In the same
way at the next level up, access to tablespace stats would tell a user a
lot about the size of databases he isn't supposed to have access to.

[ thinks for awhile... ]

It does strike me that there's a good reason for not restricting the
table-size functions: anyone who can look at pg_class.relpages can get
a maybe-not-up-to-date version of the same number. There are probably
some corner cases where exact knowledge is more interesting than
approximate knowledge, but it's hard to justify a blanket prohibition.
So maybe we should forget the restriction on the table size functions.

However, the pg_class argument doesn't extend to letting someone find
out the size of a database he's not supposed to be able to get into.

One possibility would be to make the tablespace size function silently
ignore any per-database subdirectories for which the current user
doesn't have CONNECT privilege. I'm not exactly thrilled with that,
though --- functions that silently lie to you are a great foot-gun.

Another idea is to not hardwire any restriction into the C code, but
instead have initdb revoke the default public execute access on the
tablespace size function. It would still work for superusers, and
a particular DBA could choose to grant execute to trustworthy people.
The problem is that you'd have to repeat the grant over and over
(in particular, pg_dump wouldn't save its effects).

regards, tom lane

Tom,

> Another idea is to not hardwire any restriction into the C code, but
> instead have initdb revoke the default public execute access on the
> tablespace size function. It would still work for superusers, and
> a particular DBA could choose to grant execute to trustworthy people.
> The problem is that you'd have to repeat the grant over and over
> (in particular, pg_dump wouldn't save its effects).

Yeah, the big issue here is that Tablespaces do not have any kind of "read"
permission, so there's nothing for us to go by. There's a good reason for
them not to, since they're orthagonal to schema and databases, but it
leaves us without a "handle" for tablespace size.

On the other hand, how useful is the information that a tablespace is 35GB
in size and that includes 16GB of stuff you're not allowed to see? Are we
hypothesizing that some attacker would not have CONNECT on a DB, but would
know exactly which tables that DB stores on which tablespace? This seems
very corner-case.

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Yeah, the big issue here is that Tablespaces do not have any kind of "read"
> permission, so there's nothing for us to go by. There's a good reason for
> them not to, since they're orthagonal to schema and databases, but it
> leaves us without a "handle" for tablespace size.

Yeah --- we do have a "usage" permission for tablespaces but it's not
clear that that should extend to letting people deduce info about other
databases.

> On the other hand, how useful is the information that a tablespace is 35GB
> in size and that includes 16GB of stuff you're not allowed to see? Are we
> hypothesizing that some attacker would not have CONNECT on a DB, but would
> know exactly which tables that DB stores on which tablespace? This seems
> very corner-case.

No, it's exactly the sort of "side channel" that blackhats look for all
the time. For instance, many of openssl's recent security fixes have
been about closing ways to infer what the program is doing from
secondary information, like how long it takes to respond.

Now you can argue that approximate database size information simply
isn't that useful to an attacker, and maybe that's true. But are
we prepared to make a policy decision that we aren't going to try to
protect such information at all? It's analogous to saying that we don't
try to protect system catalog contents, which is a policy that we've
held to but it's a frequent source of complaints. Also, we do have one
answer for people who complain about that: give mutually untrusting
users separate databases. The functions at issue here give some
visibility into other databases, with *no* recourse for someone who
finds that unhappy-making, short of running multiple postmasters
(which is a pretty inefficient solution).

regards, tom lane

Tom,

> Now you can argue that approximate database size information simply
> isn't that useful to an attacker, and maybe that's true. But are
> we prepared to make a policy decision that we aren't going to try to
> protect such information at all?

But it's not making *no* attempt. This is a special case; it only applies
when a limited number of databases share the same tablespace. If the admin
is concerned about protecting private info about database size, then either
put the DBs in separate tablespaces, or make sure there's so many dbs in the
tablespace that no useful information can be derived.

Hmmm ... execept we're not requiring even permission on *one* DB in the
tablespace are we? That *is* an issue. How difficult would it be to require
that the requestor have CONNECT on at least one DB in the tablespace? Like
by requiring them to be connected to that DB, or to be the Superuser?

--
Josh Berkus
PostgreSQL @ Sun
San Francisco

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Hmmm ... execept we're not requiring even permission on *one* DB in the
> tablespace are we?

The status-quo-ante was that any user could get the number for any
database and/or any tablespace. I'm prepared to admit that what I
committed is too strong, but no restriction at all still seems too weak.

> How difficult would it be to require
> that the requestor have CONNECT on at least one DB in the tablespace?

... in particular, that restriction seems pretty content-free for most
practical layouts. And it's got interesting security behaviors:
DBA A, by more-or-less innocently allowing some tables in his database B
to be created in tablespace C, might be allowing his unrelated user D to
find out info about some other database E that shares use of C. I'd
like there to have to be some direct, intended connection of D to E
before D can measure E's size ...

regards, tom lane

Tom Lane wrote:
> 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?

Oh dear. Those functions were actually just moved from pgstattuple, and
has been there since 8.2. Better backpatch that to the pgstattuple
functions in 8.2. It didn't occur to me to check the permissions on the
existing functions while I added the new ones.

I doubt there's any tools out there using those functions, so
restricting them to superuser only is probably ok.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Tom,

> ... in particular, that restriction seems pretty content-free for most
> practical layouts. And it's got interesting security behaviors:
> DBA A, by more-or-less innocently allowing some tables in his database B
> to be created in tablespace C, might be allowing his unrelated user D to
> find out info about some other database E that shares use of C. I'd
> like there to have to be some direct, intended connection of D to E
> before D can measure E's size ...

Well, that puts us back in the position of requiring a "read" or "metadata"
permission for tablespaces, or requiring superuser access. The latter is
unpalatable because there are existing tools in the field which work without
superuser access; the former is troublesome because it wouldn't be used for
anything other than the dbsize function, at least not right now.

--
Josh Berkus
PostgreSQL @ Sun
San Francisco

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Well, that puts us back in the position of requiring a "read" or "metadata"
> permission for tablespaces, or requiring superuser access. The latter is
> unpalatable because there are existing tools in the field which work without
> superuser access; the former is troublesome because it wouldn't be used for
> anything other than the dbsize function, at least not right now.

Well, what about something like the following:

* no restriction on table-size function (on the grounds that you could
look into pg_class)

* no restriction on database-size function *when applied to the current
database* (again, you could look into pg_class); to apply to some other
database, you must have connect privileges. (Actually, on the
assumption that you must have connect privs to current DB, I guess we
could simplify that to connect privs on target DB, full stop.)

* tablespace-size function requires being owner of current DB.

There is nothing particularly principled about the last choice, but
it's not superuser and not wide open either.

Another option for tablespace-size is that you must have CREATE on the
target tablespace. This is also not real principled, since CREATE is
more of a "write" privilege than a "read" one, but it'd at least give
DBAs some tool to work with.

Also, I don't see anything very wrong with defining USAGE on a
tablespace to mean that you can use the tablespace-size function on it.
AFAIR the overhead for allowing an existing privilege keyword to apply
to another type of object is just about nil --- a couple macro changes.

With any of these three options, the tablespace-size function would
change from "allowed to anyone" to "not allowed by default", unless you
happened to be the DB owner. Not sure which would be preferable from
the point of view of those existing tools you mention.

regards, tom lane

Tom Lane wrote:
> * no restriction on database-size function *when applied to the current
> database* (again, you could look into pg_class); to apply to some other
> database, you must have connect privileges. (Actually, on the
> assumption that you must have connect privs to current DB, I guess we
> could simplify that to connect privs on target DB, full stop.)

The latter would be preferrable for pgAdmin which queries database-level
info from the maintenance DB (usually postgres).

> * tablespace-size function requires being owner of current DB.

I assume superusers will also be able to use it, not just the actual owner?

/D

Dave Page <dpage(at)postgresql(dot)org> writes:
> Tom Lane wrote:
>> * tablespace-size function requires being owner of current DB.

> I assume superusers will also be able to use it, not just the actual owner?

Right --- it'd be an "ownercheck" call which means that superusers and
anyone who's been granted membership in the owning role will succeed,
not just exact matches to the role OID.

However the privilege-bit alternatives might be easier to manage.

regards, tom lane

We seem to be down to arguing about what permissions are needed to
execute the tablespace-size functions. I wrote:

> * tablespace-size function requires being owner of current DB.
> There is nothing particularly principled about the last choice, but
> it's not superuser and not wide open either.

> Another option for tablespace-size is that you must have CREATE on the
> target tablespace. This is also not real principled, since CREATE is
> more of a "write" privilege than a "read" one, but it'd at least give
> DBAs some tool to work with.

> Also, I don't see anything very wrong with defining USAGE on a
> tablespace to mean that you can use the tablespace-size function on it.
> AFAIR the overhead for allowing an existing privilege keyword to apply
> to another type of object is just about nil --- a couple macro changes.

Bruce and I were just chatting about this, and we came up with a couple
of fairly good arguments against the first option (that DB owners can
use it):

* DB owners are supposed to have special permissions within their own
DB, but not necessarily system-wide; so being a DB owner shouldn't in
itself grant privileges to check tablespace sizes for tablespaces you
otherwise have no access to.

* Dave was concerned about being able to use tools that connect to
postgres or template1 by default. Well, those databases are almost
certainly going to be owned by postgres, so for such a tool to be able
to use a DB-owner-only function, it's gonna have to be superuser or
darn near.

So I think I don't like that idea at all. And the third one does seem
like overkill. Bruce likes the CREATE-privilege option on the grounds
that if you have the right to make tables in a tablespace, you should
have the right to find out how big it is. From a security point of
view, you can find it out indirectly if you have some idea how big a
disk partition it's mounted on: fill up a temp table until you run out
of disk space, and subtract. So trying to deny the knowledge seems a
bit pointless anyway.

Therefore, I propose the same choices as before for table-size (no
restriction) and database-size (must have CONNECT priv), and this
for tablespace-size: you must have the ability to create tables in
the target tablespace. This could be either an explicit CREATE
grant, or implicit because you are in a DB that has it as the default
tablespace.

Comments?

regards, tom lane

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> Tom Lane wrote:
>> 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?

> Oh dear. Those functions were actually just moved from pgstattuple, and
> has been there since 8.2. Better backpatch that to the pgstattuple
> functions in 8.2.

Done, thanks for the heads-up.

> I doubt there's any tools out there using those functions, so
> restricting them to superuser only is probably ok.

If anyone complains, we could probably knock them down to SELECT
privilege, but finding the parent table to apply the select priv check
to seems a bit of a pain for the index cases. So I won't bother unless
someone does complain.

regards, tom lane

On Tuesday 28 August 2007 15:38, Tom Lane wrote:
> Therefore, I propose the same choices as before for table-size (no
> restriction) and database-size (must have CONNECT priv), and this
> for tablespace-size: you must have the ability to create tables in
> the target tablespace.  This could be either an explicit CREATE
> grant, or implicit because you are in a DB that has it as the default
> tablespace.

+1

--
Josh Berkus
PostgreSQL @ Sun
San Francisco