Thread: Fw: Isn't pg_statistic a security hole - Solution Proposal
Hello all,
Attached is a patch to implement a new internal function, 'has_privilege'.
My proposal below explains the reasoning behind this submittal, although I
never did get any feedback -- positive or negative. If the patch is accepted
I'll be happy to do the work to create the system view as descibed.
The patch applies cleanly against cvs tip. One item I was not sure about was
the selection of the OID value for the new function. I chose 1920 for no
other reason that the highest OID in pg_proc.h was 1909, and this seemed
like a safe value. Is there somewhere I should have looked for guidance on
this?
Thanks,
-- Joe
> The recent discussions on pg_statistic got me started thinking about how
to
> implement a secure form of the view. Based on the list discussion, and a
> suggestion from Tom, I did some research regarding how SQL92 and some of
the
> larger commercial database systems allow access to system privilege
> information.
>
> I reviewed the ANSI SQL 92 specification, Oracle, MSSQL, and IBM DB2
> (documentation only). Here's what I found:
>
> ANSI SQL 92 does not have any functions defined for retrieving privilege
> information. It does, however define an "information schema" and
"definition
> schema" which among other things includes a TABLE_PRIVILEGES view.
>
> With this view available, it is possible to discern what privileges the
> current user has using a simple SQL statement. In Oracle, I found this
view,
> and some other variations. According to the Oracle DBA I work with, there
is
> no special function, and a SQL statement on the view is how he would
gather
> this kind of information when needed.
>
> MSSQL Server 7 also has this same view. Additionally, SQL7 has a T-SQL
> function called PERMISSIONS with the following description:
> "Returns a value containing a bitmap that indicates the statement, object,
> or column permissions for the current user.
> Syntax PERMISSIONS([objectid [, 'column']])".
>
> I only looked briefly at the IBM DB2 documentation, but could find no
> mention of TABLE_PRIVILEGES or any privilege specific function. I imagine
> TABLE_PRIVILEGES might be there somewhere since it seems to be standard
> SQL92.
>
> Based on all of the above, I concluded that there is nothing compelling in
> terms of a specific function to be compatible with. I do think that in the
> longer term it makes sense to implement the SQL 92 information schema
views
> in PostgreSQL.
>
> So, now for the proposal. I created a function (attached) which will allow
> any privilege type to be probed, called has_privilege. It is used like
this:
>
> select relname from pg_class where has_privilege(current_user, relname,
> 'update');
>
> or
>
> select has_privilege('postgres', 'pg_shadow', 'select');
>
> where
> the first parameter is any valid user name
> the second parameter can be a table, view, or sequence
> the third parameter can be 'select', 'insert', 'update', 'delete', or
> 'rule'
>
> The function is currently implemented as an external c function and
designed
> to be built under contrib. This function should really be an internal
> function. If the proposal is acceptable, I would like to take on the task
of
> turning the function into an internal one (with guidance, pointers,
> suggestions greatly appreciated). This would allow a secure view to be
> implemented over pg_statistic as:
>
> create view pg_userstat as (
> select
> s.starelid
> ,s.staattnum
> ,s.staop
> ,s.stanullfrac
> ,s.stacommonfrac
> ,s.stacommonval
> ,s.staloval
> ,s.stahival
> ,c.relname
> ,a.attname
> ,sh.usename
> from
> pg_statistic as s
> ,pg_class as c
> ,pg_shadow as sh
> ,pg_attribute as a
> where
> has_privilege(current_user,c.relname,'select')
> and sh.usesysid = c.relowner
> and a.attrelid = c.oid
> and c.oid = s.starelid
> );
>
> Then restrict pg_statistic from public viewing. This view would allow the
> current user to view statistics only on relations for which they already
> have 'select' granted.
>
> Comments?
>
> Regards,
> -- Joe
>
> installation:
>
> place in contrib
> tar -xzvf has_priv.tgz
> cd has_priv
> ./install.sh
> Note: installs the function into template1 by default. Edit install.sh to
> change.
>
>
Attachment
Joe Conway writes:
> The patch applies cleanly against cvs tip. One item I was not sure about was
> the selection of the OID value for the new function. I chose 1920 for no
> other reason that the highest OID in pg_proc.h was 1909, and this seemed
> like a safe value. Is there somewhere I should have looked for guidance on
> this?
~/pgsql/src/include/catalog$ ./unused_oids
3 - 11
90
143
352 - 353
1264
1713 - 1717
1813
1910 - 16383
> > ANSI SQL 92 does not have any functions defined for retrieving privilege
> > information. It does, however define an "information schema" and
> "definition
> > schema" which among other things includes a TABLE_PRIVILEGES view.
Yes, that's what we pretty much want to do once we have schema support.
The function you propose, or one similar to it, will probably be needed to
make this work.
> > select has_privilege('postgres', 'pg_shadow', 'select');
> >
> > where
> > the first parameter is any valid user name
> > the second parameter can be a table, view, or sequence
> > the third parameter can be 'select', 'insert', 'update', 'delete', or
> > 'rule'
This is probably going to blow up when we have the said schema support.
Probably better to reference things by oid. Also, since things other than
relations might have privileges sometime, the function name should
probably imply this; maybe "has_table_privilege".
Implementation notes:
* This function should probably go into backend/utils/adt/acl.c.
* You don't need PG_FUNCTION_INFO_V1 for built-in functions.
* I'm not sure whether it's useful to handle NULL parameters explicitly.
The common approach is to return NULL, which would be semantically right
for this function.
--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes:
> This is probably going to blow up when we have the said schema support.
> Probably better to reference things by oid.
Two versions, one that takes an oid and one that takes a name, might be
convenient. The name version will probably have to accept qualified
names (schema.table) once we have schema support --- but I don't think
that needs to break the function definition. An unqualified name would
be looked up using whatever schema resolution rules would be in effect
for ordinary table references.
We might also want the user to be specified by usesysid rather than
name; and a two-parameter form that assumes user == current_user would
be a particularly useful shorthand.
> Also, since things other than
> relations might have privileges sometime, the function name should
> probably imply this; maybe "has_table_privilege".
Agreed.
> * I'm not sure whether it's useful to handle NULL parameters explicitly.
> The common approach is to return NULL, which would be semantically right
> for this function.
The standard approach for C-coded functions is to mark them
'proisstrict' in pg_proc, and then not waste any code checking for NULL;
the function manager takes care of it for you. The only reason not to
do it that way is if you actually want to return non-NULL for (some
cases with) NULL inputs. Offhand this looks like a strict function to
me...
regards, tom lane
> The standard approach for C-coded functions is to mark them
> 'proisstrict' in pg_proc, and then not waste any code checking for NULL;
> the function manager takes care of it for you. The only reason not to
> do it that way is if you actually want to return non-NULL for (some
> cases with) NULL inputs. Offhand this looks like a strict function to
> me...
>
Thanks for the feedback! To summarize the recommended changes:
- put function into backend/utils/adt/acl.c.
- remove PG_FUNCTION_INFO_V1
- mark 'proisstrict' in pg_proc
- rename to has_table_privilege()
- overload the function name for 6 versions (OIDs 1920 - 1925):
-> has_table_privilege(text username, text relname, text priv)
-> has_table_privilege(oid usesysid, text relname, text priv)
-> has_table_privilege(oid usesysid, oid reloid, text priv)
-> has_table_privilege(text username, oid reloid, text priv)
-> has_table_privilege(text relname, text priv) /* assumes
current_user */
-> has_table_privilege(oid reloid, text priv) /* assumes current_user
*/
New patch forthcoming . . .
-- Joe
Tom Lane writes: > Two versions, one that takes an oid and one that takes a name, might be > convenient. The name version will probably have to accept qualified > names (schema.table) once we have schema support Will you expect the function to do dequoting etc. as well? This might get out of hand. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes:
> Will you expect the function to do dequoting etc. as well? This might get
> out of hand.
Hm. We already have such code available for nextval(), so I suppose
it might be appropriate to invoke that. Not sure. Might be better
to expect the given string to be the correct case already. Let's see
... if you expect the function to be applied to names extracted from
pg_class or other tables, then exact case would be better --- but it'd
be just as easy to invoke the OID form in such cases. For hand-entered
data the nextval convention is probably more convenient.
regards, tom lane
> Thanks for the feedback! To summarize the recommended changes: > > - put function into backend/utils/adt/acl.c. > - remove PG_FUNCTION_INFO_V1 > - mark 'proisstrict' in pg_proc > - rename to has_table_privilege() > - overload the function name for 6 versions (OIDs 1920 - 1925): > -> has_table_privilege(text username, text relname, text priv) > -> has_table_privilege(oid usesysid, text relname, text priv) > -> has_table_privilege(oid usesysid, oid reloid, text priv) > -> has_table_privilege(text username, oid reloid, text priv) > -> has_table_privilege(text relname, text priv) /* assumes > current_user */ > -> has_table_privilege(oid reloid, text priv) /* assumes current_user > */ > Here's a new patch for has_table_privilege( . . .). One change worthy of note is that I added a definition to fmgr.h as follows: #define PG_NARGS (fcinfo->nargs) This allowed me to use two of the new functions to handle both 2 and 3 argument cases. Also different from the above, I used int instead of oid for the usesysid type. I'm also attaching a test script and expected output. I haven't yet looked at how to properly include these into the normal regression testing -- any pointers are much appreciated. Thanks, -- Joe
Attachment
"Joe Conway" <joe@conway-family.com> writes:
> Here's a new patch for has_table_privilege
Looks like you're getting there. Herewith some miscellaneous comments
on minor matters like coding style:
> I used int instead of oid for the usesysid type.
I am not sure if that's a good idea or not. Peter E. has proposed
changing usesysid to type OID or even eliminating it entirely
(identifying users by pg_shadow row OID alone). While this hasn't
happened yet, it'd be a good idea to minimize the dependencies of your
code on which type is used for user ID. In particular I'd suggest using
"name" and "id" in the names of your variant functions, not "text" and
"oid" and "int", so that they don't have to be renamed if the types
change.
> I'm also attaching a test script and expected output.
The script doesn't seem to demonstrate that any attention is paid to the
mode input --- AFAICT all the tested cases are either all privileges
available or no privileges available. It could probably be a lot
shorter without being materially less effective, too; quasi-exhaustive
tests are usually not worth the cycles to run.
> +Datum
> +text_oid_has_table_privilege(PG_FUNCTION_ARGS)
This is just my personal preference, but I'd put the type identifiers at
the end (has_table_privilege_name_id) rather than giving them pride of
place at the start of the name.
> +Datum
> +has_table_privilege(int usesysid, char *relname, char *priv_type)
Since has_table_privilege is just an internal function and is not
fmgr-callable, there's no percentage in declaring it to return Datum;
it should just return bool and not use the PG_RETURN_ macros. You have
in fact called it as though it returned bool, which would be a type
violation if C were not so lax about type conversions.
Actually, though, I'm wondering why has_table_privilege is a function
at all. Its tests for valid usesysid and relname are a waste of cycles;
pg_aclcheck will do those for itself. The only actually useful code in
it is the conversion from a priv_type string to an AclMode code, which
would seem to be better handled as a separate function that just does
that part. The has_table_privilege_foo_bar routines could call
pg_aclcheck for themselves without any material loss of concision.
> + result = pg_aclcheck(relname, usesysid, mode);
> +
> + if (result == 1) {
This is not only non-symbolic, but outright wrong. You should be
testing pg_aclcheck's result to see if it is ACLCHECK_OK or not.
> +/* Privilege names for oid_oid_has_table_privilege */
> +#define PRIV_INSERT "INSERT\0"
> +#define PRIV_SELECT "SELECT\0"
> +#define PRIV_UPDATE "UPDATE\0"
> +#define PRIV_DELETE "DELETE\0"
> +#define PRIV_RULE "RULE\0"
> +#define PRIV_REFERENCES "REFERENCES\0"
> +#define PRIV_TRIGGER "TRIGGER\0"
You need not write these strings with those redundant null terminators.
For that matter, since they're only known in one function, it's not
clear that they should be exported to all and sundry in a header file
in the first place. I'd be inclined to just code
AclMode convert_priv_string (char * str)
{
if (strcasecmp(str, "SELECT") == 0)
return ACL_SELECT;
if (strcasecmp(str, "INSERT") == 0)
return ACL_INSERT;
... etc ...
elog(ERROR, ...);
}
and keep all the knowledge right there in that function. (Possibly it
should take a text* not char*, so as to avoid duplicated conversion code
in the callers, but this is minor.)
Despite all these gripes, it looks pretty good. One more round of
revisions ...
regards, tom lane
Thanks for the detailed feedback, Tom. I really appreciate the pointers on
my style and otherwise. Attached is my next attempt. To summarize the
changes:
- changed usesysid back to Oid. I noticed that the Acl functions all treated
usesysid as an Oid anyway.
- changed function names to has_user_privilege_name_name,
has_user_privilege_name_id, etc
- trimmed down test script, added variety (some privs granted, not all), and
added bad input cases (this already paid off -- see below)
- replaced has_table_privilege(int usesysid, char *relname, char *priv_type)
with
AclMode convert_priv_string (text * priv_type_text)
- changed
if (result == 1) {
PG_RETURN_BOOL(FALSE);
. . .
to
if (result == ACLCHECK_OK) {
PG_RETURN_BOOL(TRUE);
. . .
- removed #define PRIV_INSERT "INSERT\0", etc from acl.h
One item of note -- while pg_aclcheck *does* validate relname for
non-superusers, it *does not* bother for superusers. Therefore I left the
relname check in the has_table_privilege_*_name() functions. Also note that
I skipped has_priv_r3.diff -- that one helped find the superuser/relname
issue.
I hope this version passes muster ;-)
-- Joe
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
> Thanks for the detailed feedback, Tom. I really appreciate the pointers on
> my style and otherwise. Attached is my next attempt. To summarize the
> changes:
>
> - changed usesysid back to Oid. I noticed that the Acl functions all treated
> usesysid as an Oid anyway.
>
> - changed function names to has_user_privilege_name_name,
> has_user_privilege_name_id, etc
>
> - trimmed down test script, added variety (some privs granted, not all), and
> added bad input cases (this already paid off -- see below)
>
> - replaced has_table_privilege(int usesysid, char *relname, char *priv_type)
> with
> AclMode convert_priv_string (text * priv_type_text)
>
> - changed
> if (result == 1) {
> PG_RETURN_BOOL(FALSE);
> . . .
> to
> if (result == ACLCHECK_OK) {
> PG_RETURN_BOOL(TRUE);
> . . .
> - removed #define PRIV_INSERT "INSERT\0", etc from acl.h
>
> One item of note -- while pg_aclcheck *does* validate relname for
> non-superusers, it *does not* bother for superusers. Therefore I left the
> relname check in the has_table_privilege_*_name() functions. Also note that
> I skipped has_priv_r3.diff -- that one helped find the superuser/relname
> issue.
>
> I hope this version passes muster ;-)
>
> -- Joe
>
[ Attachment, skipping... ]
[ Attachment, skipping... ]
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Your patch has been added to the PostgreSQL unapplied patches list at: > http://candle.pha.pa.us/cgi-bin/pgpatches > I will try to apply it within the next 48 hours. It's not approved yet ... regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Your patch has been added to the PostgreSQL unapplied patches list at: > > http://candle.pha.pa.us/cgi-bin/pgpatches > > I will try to apply it within the next 48 hours. > > It's not approved yet ... OK. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026