Fw: Isn't pg_statistic a security hole - Solution Proposal

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

"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

[ -> hackers ]

Tom Lane 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(),

IMHO, nextval() isn't the greatest interface in the world. I do like the
alternative (deprecated?) syntax sequence.nextval() because of the
notational resemblence to OO. (We might even be able to turn this into
something like an SQL99 "class" feature.)

As I understand it, currently

relation.function(a, b, c)

ends up as being a function call

function(relation, a, b, c)

where the first argument is "text". This is probably an unnecessary
fragility, since the oid of the relation should already be known by that
time. So perhaps we could change this that the first argument gets passed
in an Oid. Then we'd really only need the Oid version of Joe's
has_*_privilege functions.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:

IMHO, nextval() isn't the greatest interface in the world. I do like the
alternative (deprecated?) syntax sequence.nextval() because of the
notational resemblence to OO.

Try "nonexistent". I too would like a notation like that, because it
would be more transparent to the user w.r.t. case folding and such.
But it doesn't exist now.

Observe, however, that such a notation would work well only for queries
in which the sequence/table name is fixed and known when the query is
written. I don't see a way to use it in the case where the name is
being computed at runtime (eg, taken from a table column). So it
doesn't really solve the problem posed by has_table_privilege.

As I understand it, currently
relation.function(a, b, c)
ends up as being a function call
function(relation, a, b, c)
where the first argument is "text".

Sorry, that has nothing to do with reality. What we actually have is
an equivalence between the two notations
rel.func
func(rel)
where the semantics are that an entire tuple of the relation "rel" is
passed to the function. This doesn't really gain us anything for the
problem at hand (and we'll quite likely have to give it up anyway when
we implement schemas, since SQL has very different ideas about what
a.b.c means than our current parser does).

regards, tom lane

where the semantics are that an entire tuple of the relation "rel" is
passed to the function. This doesn't really gain us anything for the
problem at hand (and we'll quite likely have to give it up anyway when
we implement schemas, since SQL has very different ideas about what
a.b.c means than our current parser does).

I wasn't quite sure if there are changes I can/should make to
has_table_privilege based on this discussion. Is there any action for me on
this (other than finishing the regression test and creating documentation
patches)?

Thanks,

-- Joe

"Joe Conway" <joe@conway-family.com> writes:

I wasn't quite sure if there are changes I can/should make to
has_table_privilege based on this discussion.

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model. (BTW, I only just now added
truncation code to that routine, so look at current CVS. Perhaps the
routine should be renamed and placed somewhere else, so that sequence.c
and has_table_privilege can share it.)

Peter's argument seemed to be that there shouldn't be name-based
variants at all, with which I do not agree; but perhaps that's not
what he meant.

regards, tom lane

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model. (BTW, I only just now added
truncation code to that routine, so look at current CVS. Perhaps the
routine should be renamed and placed somewhere else, so that sequence.c
and has_table_privilege can share it.)

Looking at get_seq_name, it does seem like it should be called something
like get_object_name (or just get_name?) and moved to a common location. Am
I correct in thinking that this function could/should be called by any other
function (internal, C, plpgsql, or otherwise) which accepts a text
representation of a system object name?

What if I rename the get_seq_name function and move it to
backend/utils/adt/name.c (and of course change the references to it in
sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and
truncate.

-- Joe

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model. (BTW, I only just now added
truncation code to that routine, so look at current CVS. Perhaps the
routine should be renamed and placed somewhere else, so that sequence.c
and has_table_privilege can share it.)

Looking at get_seq_name, it does seem like it should be called something
like get_object_name (or just get_name?) and moved to a common location. Am
I correct in thinking that this function could/should be called by any other
function (internal, C, plpgsql, or otherwise) which accepts a text
representation of a system object name?

What if I rename the get_seq_name function and move it to
backend/utils/adt/name.c (and of course change the references to it in
sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and
truncate.

-- Joe

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model. (BTW, I only just now added
truncation code to that routine, so look at current CVS. Perhaps the
routine should be renamed and placed somewhere else, so that sequence.c
and has_table_privilege can share it.)

Looking at get_seq_name, it does seem like it should be called something
like get_object_name (or just get_name?) and moved to a common location. Am
I correct in thinking that this function could/should be called by any other
function (internal, C, plpgsql, or otherwise) which accepts a text
representation of a system object name?

What if I rename the get_seq_name function and move it to
backend/utils/adt/name.c (and of course change the references to it in
sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and
truncate.

-- Joe

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model. (BTW, I only just now added
truncation code to that routine, so look at current CVS. Perhaps the
routine should be renamed and placed somewhere else, so that sequence.c
and has_table_privilege can share it.)

Looking at get_seq_name, it does seem like it should be called something
like get_object_name (or just get_name?) and moved to a common location. Am
I correct in thinking that this function could/should be called by any other
function (internal, C, plpgsql, or otherwise) which accepts a text
representation of a system object name?

What if I rename the get_seq_name function and move it to
backend/utils/adt/name.c (and of course change the references to it in
sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and
truncate.

-- Joe

representation of a system object name?

What if I rename the get_seq_name function and move it to
backend/utils/adt/name.c (and of course change the references to it in
sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and
truncate.

-- Joe

Yikes! Sorry about sending that last message 3 times -- I guess that's what
I get for using an evil mail client ;-)

Joe

Tom Lane writes:

My feeling is that the name-based variants of has_table_privilege should
perform downcasing and truncation of the supplied strings before trying
to use them as tablename or username; see get_seq_name in
backend/commands/sequence.c for a model.

I don't like this approach. It's ugly, non-intuitive, and inconvenient.

Since these functions will primarily be used in building a sort of
information schema and for querying system catalogs, we should use the
approach that is or will be used there: character type values contain the
table name already case-adjusted. Imagine the pain we would have to go
through to *re-quote* the names we get from the system catalogs and
information schema components before passing them to this function.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:

Since these functions will primarily be used in building a sort of
information schema and for querying system catalogs, we should use the
approach that is or will be used there: character type values contain the
table name already case-adjusted.

Weren't you just arguing that such cases could/should use the OID, not
the name at all? ISTM the name-based variants will primarily be used
for user-entered names, and in that case the user can reasonably expect
that a name will be interpreted the same way as if he'd written it out
in a query.

The nextval approach is ugly, I'll grant you, but it's also functional.
We got complaints about nextval before we put that in; we get lots
fewer now.

regards, tom lane