Re: [PATCHES] 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.
>
>

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(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)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(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)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(at)conway-family(dot)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(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)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(at)conway-family(dot)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(at)gmx(dot)net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e(at)gmx(dot)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

Tom Lane writes:

> Weren't you just arguing that such cases could/should use the OID, not
> the name at all?

Yes, but if we're going to have name arguments, we should have sane ones.

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

That would be correct if the user were actually entering the name in the
same way, i.e., unquoted or double-quoted.

> The nextval approach is ugly, I'll grant you, but it's also functional.

But it's incompatible with the SQL conventions.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> > 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.
>
> That would be correct if the user were actually entering the name in the
> same way, i.e., unquoted or double-quoted.
>
> > The nextval approach is ugly, I'll grant you, but it's also functional.
>
> But it's incompatible with the SQL conventions.
>

Is the concern that the name-based variants of the function should be called
like:

select has_table_privilege(current_user, pg_class, 'insert');
or
select has_table_privilege(current_user, "My Quoted Relname", 'insert');

instead of

select has_table_privilege(current_user, 'pg_class', 'insert');
or
select has_table_privilege(current_user, '"My Quoted Relname"',
'insert');

?

If so, what would be involved in fixing it?

From an end user's perspective, I wouldn't mind the latter syntax, although
the former is clearly more intuitive. But I'd rather have the second form
than nothing (just MHO).

-- Joe

"Joe Conway" <joe(at)conway-family(dot)com> writes:
> Is the concern that the name-based variants of the function should be called
> like:

> select has_table_privilege(current_user, pg_class, 'insert');
> or
> select has_table_privilege(current_user, "My Quoted Relname", 'insert');

It'd be really nice to do that, but I don't see any reasonable way to do
it. The problem is that the unquoted relation name will probably be
resolved (to the wrong thing) before we discover that the function wants
it to be resolved as a relation OID. Remember that the arguments of a
function have to be resolved before we can even start to look up the
function, since function lookup depends on the types of the arguments.

I have just thought of a possible compromise. Peter is right that we
don't want case conversion on table names that are extracted from
catalogs. But I think we do want it on table names expressed as string
literals. Could we make the assumption that table names in catalogs
will be of type 'name'? If so, it'd work to make two versions of the
has_table_privilege function, one taking type "name" and the other
taking type "text". The "name" version would take its input as-is,
the "text" version would do case folding and truncation. This would
work transparently for queries selecting relation names from the system
catalogs, and it'd also work transparently for queries using unmarked
string literals (which will be preferentially resolved as type "text").
Worst case if the system makes the wrong choice is you throw in an
explicit coercion to name or text. Comments?

regards, tom lane

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(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

> I have just thought of a possible compromise. Peter is right that we
> don't want case conversion on table names that are extracted from
> catalogs. But I think we do want it on table names expressed as string
> literals. Could we make the assumption that table names in catalogs
> will be of type 'name'? If so, it'd work to make two versions of the
> has_table_privilege function, one taking type "name" and the other
> taking type "text". The "name" version would take its input as-is,
> the "text" version would do case folding and truncation. This would
> work transparently for queries selecting relation names from the system
> catalogs, and it'd also work transparently for queries using unmarked
> string literals (which will be preferentially resolved as type "text").
> Worst case if the system makes the wrong choice is you throw in an
> explicit coercion to name or text. Comments?

OK -- here's take #5.

It "make"s and "make check"s clean against current cvs tip.

There are now both Text and Name variants, and the regression test support
is rolled into the patch. Note that to be complete wrt Name based variants,
there are now 12 user visible versions of has_table_privilege:

has_table_privilege(Text usename, Text relname, Text priv_type)
has_table_privilege(Text usename, Name relname, Text priv_type)
has_table_privilege(Name usename, Text relname, Text priv_type)
has_table_privilege(Name usename, Name relname, Text priv_type)
has_table_privilege(Text relname, Text priv_type) /* assumes current_user */
has_table_privilege(Name relname, Text priv_type) /* assumes current_user */
has_table_privilege(Text usename, Oid reloid, Text priv_type)
has_table_privilege(Name usename, Oid reloid, Text priv_type)
has_table_privilege(Oid reloid, Text priv_type) /* assumes current_user */
has_table_privilege(Oid usesysid, Text relname, Text priv_type)
has_table_privilege(Oid usesysid, Name relname, Text priv_type)
has_table_privilege(Oid usesysid, Oid reloid, Text priv_type)

For the Text based inputs, a new internal function, get_Name is used
(shamelessly copied from get_seq_name in sequence.c) to downcase if not
quoted, or remove quotes if quoted, and truncate. I also added a few test
cases for the downcasing, quote removal, and Name based variants to the
regression test.

Only thing left (I hope!) is documentation. I'm sure I either have or can
get the DocBook tools, but I've never used them. Would it be simpler to
clone and hand edit one of the existing docs? Any suggestions to get me
started?

Thanks,

-- Joe

> I have just thought of a possible compromise. Peter is right that we
> don't want case conversion on table names that are extracted from
> catalogs. But I think we do want it on table names expressed as string
> literals. Could we make the assumption that table names in catalogs
> will be of type 'name'? If so, it'd work to make two versions of the
> has_table_privilege function, one taking type "name" and the other
> taking type "text". The "name" version would take its input as-is,
> the "text" version would do case folding and truncation. This would
> work transparently for queries selecting relation names from the system
> catalogs, and it'd also work transparently for queries using unmarked
> string literals (which will be preferentially resolved as type "text").
> Worst case if the system makes the wrong choice is you throw in an
> explicit coercion to name or text. Comments?

Seems you are adding a distinction between name and text that we never
had before. Is it worth it to fix this case?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

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.

> > I have just thought of a possible compromise. Peter is right that we
> > don't want case conversion on table names that are extracted from
> > catalogs. But I think we do want it on table names expressed as string
> > literals. Could we make the assumption that table names in catalogs
> > will be of type 'name'? If so, it'd work to make two versions of the
> > has_table_privilege function, one taking type "name" and the other
> > taking type "text". The "name" version would take its input as-is,
> > the "text" version would do case folding and truncation. This would
> > work transparently for queries selecting relation names from the system
> > catalogs, and it'd also work transparently for queries using unmarked
> > string literals (which will be preferentially resolved as type "text").
> > Worst case if the system makes the wrong choice is you throw in an
> > explicit coercion to name or text. Comments?
>
> OK -- here's take #5.
>
> It "make"s and "make check"s clean against current cvs tip.
>
> There are now both Text and Name variants, and the regression test support
> is rolled into the patch. Note that to be complete wrt Name based variants,
> there are now 12 user visible versions of has_table_privilege:
>
> has_table_privilege(Text usename, Text relname, Text priv_type)
> has_table_privilege(Text usename, Name relname, Text priv_type)
> has_table_privilege(Name usename, Text relname, Text priv_type)
> has_table_privilege(Name usename, Name relname, Text priv_type)
> has_table_privilege(Text relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Name relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Text usename, Oid reloid, Text priv_type)
> has_table_privilege(Name usename, Oid reloid, Text priv_type)
> has_table_privilege(Oid reloid, Text priv_type) /* assumes current_user */
> has_table_privilege(Oid usesysid, Text relname, Text priv_type)
> has_table_privilege(Oid usesysid, Name relname, Text priv_type)
> has_table_privilege(Oid usesysid, Oid reloid, Text priv_type)
>
> For the Text based inputs, a new internal function, get_Name is used
> (shamelessly copied from get_seq_name in sequence.c) to downcase if not
> quoted, or remove quotes if quoted, and truncate. I also added a few test
> cases for the downcasing, quote removal, and Name based variants to the
> regression test.
>
> Only thing left (I hope!) is documentation. I'm sure I either have or can
> get the DocBook tools, but I've never used them. Would it be simpler to
> clone and hand edit one of the existing docs? Any suggestions to get me
> started?
>
> Thanks,
>
> -- Joe
>
>
>
>
>
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

I don't know about other people but the 48 hours notice email and the
web page of outstanding patches seems to be working well for me.

> > I have just thought of a possible compromise. Peter is right that we
> > don't want case conversion on table names that are extracted from
> > catalogs. But I think we do want it on table names expressed as string
> > literals. Could we make the assumption that table names in catalogs
> > will be of type 'name'? If so, it'd work to make two versions of the
> > has_table_privilege function, one taking type "name" and the other
> > taking type "text". The "name" version would take its input as-is,
> > the "text" version would do case folding and truncation. This would
> > work transparently for queries selecting relation names from the system
> > catalogs, and it'd also work transparently for queries using unmarked
> > string literals (which will be preferentially resolved as type "text").
> > Worst case if the system makes the wrong choice is you throw in an
> > explicit coercion to name or text. Comments?
>
> OK -- here's take #5.
>
> It "make"s and "make check"s clean against current cvs tip.
>
> There are now both Text and Name variants, and the regression test support
> is rolled into the patch. Note that to be complete wrt Name based variants,
> there are now 12 user visible versions of has_table_privilege:
>
> has_table_privilege(Text usename, Text relname, Text priv_type)
> has_table_privilege(Text usename, Name relname, Text priv_type)
> has_table_privilege(Name usename, Text relname, Text priv_type)
> has_table_privilege(Name usename, Name relname, Text priv_type)
> has_table_privilege(Text relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Name relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Text usename, Oid reloid, Text priv_type)
> has_table_privilege(Name usename, Oid reloid, Text priv_type)
> has_table_privilege(Oid reloid, Text priv_type) /* assumes current_user */
> has_table_privilege(Oid usesysid, Text relname, Text priv_type)
> has_table_privilege(Oid usesysid, Name relname, Text priv_type)
> has_table_privilege(Oid usesysid, Oid reloid, Text priv_type)
>
> For the Text based inputs, a new internal function, get_Name is used
> (shamelessly copied from get_seq_name in sequence.c) to downcase if not
> quoted, or remove quotes if quoted, and truncate. I also added a few test
> cases for the downcasing, quote removal, and Name based variants to the
> regression test.
>
> Only thing left (I hope!) is documentation. I'm sure I either have or can
> get the DocBook tools, but I've never used them. Would it be simpler to
> clone and hand edit one of the existing docs? Any suggestions to get me
> started?
>
> Thanks,
>
> -- Joe
>
>
>
>
>
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

>
> I don't know about other people but the 48 hours notice email and the
> web page of outstanding patches seems to be working well for me.
>

Sorry -- my "into the ether" comment wasn't griping about you :-)

I've been having serious problems with my "mail.com" account. I originally
sent this post Saturday afternoon, but it didn't even make it to the
pgsql-patches list
until Monday morning (and I only know that because I started reading the
comp.databases.postgresql.patches news feed).
:(

Joe

> >
> > I don't know about other people but the 48 hours notice email and the
> > web page of outstanding patches seems to be working well for me.
> >
>
> Sorry -- my "into the ether" comment wasn't griping about you :-)
>
> I've been having serious problems with my "mail.com" account. I originally
> sent this post Saturday afternoon, but it didn't even make it to the
> pgsql-patches list
> until Monday morning (and I only know that because I started reading the
> comp.databases.postgresql.patches news feed).
> :(

No, I didn't see any gripe. I was just saying I like making the
announcement, having it in the queue on a web page everyone can see, and
later applying it. Seems like a good procedure, and added because
people were complaining I was not consistenly waiting for comments
before applying patches.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Tom Lane writes:

> Could we make the assumption that table names in catalogs
> will be of type 'name'?

I wouldn't want to guarantee it for the information schema.

> If so, it'd work to make two versions of the has_table_privilege
> function, one taking type "name" and the other taking type "text".
> The "name" version would take its input as-is, the "text" version
> would do case folding and truncation.

Target typing is ugly.

I've tried to look up the supposed \paraphrase{we had enough problems
before we added the existing behaviour to setval, etc.} discussion but
couldn't find it. My experience on the mailing list is that it goes the
other way.

The identifier quoting rules are already surprising enough for the
uninitiated, but it's even more surprising that they even apply when
syntactically no identifier is present. Between the behaviour of "what
you see is what you get" and "this language is kind of confusing so you
have to quote your strings twice with two different quoting characters"
the choice is obvious to me.

I'm also arguing for consistency with the standard. According to that,
users will be able to do

SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = 'SoMeThInG';

and a load of similar queries. You can't change the case folding rules
here unless you really want to go out of your way, and then you have
really confused the heck out of users.

We could make relname.func() work without breaking compatibility, ISTM,
and then we only need the Oid version. For computing the relation name at
execution time, the "plain" version is going to be more useful anyway.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> Tom Lane writes:
>
> > Could we make the assumption that table names in catalogs
> > will be of type 'name'?
>
> I wouldn't want to guarantee it for the information schema.
>
> > If so, it'd work to make two versions of the has_table_privilege
> > function, one taking type "name" and the other taking type "text".
> > The "name" version would take its input as-is, the "text" version
> > would do case folding and truncation.
>
> Target typing is ugly.
>
> I've tried to look up the supposed \paraphrase{we had enough problems
> before we added the existing behaviour to setval, etc.} discussion but
> couldn't find it. My experience on the mailing list is that it goes the
> other way.

I am confused. What are you suggesting as far as having a name and text
version of the functions?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> Could we make the assumption that table names in catalogs
>> will be of type 'name'?

> I wouldn't want to guarantee it for the information schema.

Your objections are not without merit, and in the interest of bringing
this thing to closure I'll concede for now. I want to get on with this
so that I can wrap up the pg_statistic view that started the whole
thread.

What I suggest we do is apply the portions of Joe's latest patch that
support has_table_privilege with OID inputs and with NAME inputs,
omitting the combinations that take TEXT inputs and do casefolding.
We can add that part later if it proves that people do indeed want it.

I have specific reasons for wanting to keep the functions accepting
NAME rather than TEXT: that will save a run-time type conversion in the
common case where one is reading the input from a system catalog, and
it will at least provide automatic truncation of overlength names when
one is accepting a literal. (I trust Peter won't object to that ;-).)

We will probably have to revisit this territory when we implement
schemas: there will need to be a way to input qualified table names
like foo.bar, and a way to input NON qualified names like "foo.bar".
But we can cross that bridge when we come to it.

Comments, objections?

regards, tom lane

Tom Lane writes:

> What I suggest we do is apply the portions of Joe's latest patch that
> support has_table_privilege with OID inputs and with NAME inputs,
> omitting the combinations that take TEXT inputs and do casefolding.
> We can add that part later if it proves that people do indeed want it.

Okay.

> We will probably have to revisit this territory when we implement
> schemas: there will need to be a way to input qualified table names
> like foo.bar, and a way to input NON qualified names like "foo.bar".
> But we can cross that bridge when we come to it.

I figured we would add another argument to the function.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> What I suggest we do is apply the portions of Joe's latest patch that
> support has_table_privilege with OID inputs and with NAME inputs,
> omitting the combinations that take TEXT inputs and do casefolding.
> We can add that part later if it proves that people do indeed want it.
>
> I have specific reasons for wanting to keep the functions accepting
> NAME rather than TEXT: that will save a run-time type conversion in the
> common case where one is reading the input from a system catalog, and
> it will at least provide automatic truncation of overlength names when
> one is accepting a literal. (I trust Peter won't object to that ;-).)
>

I'll rework the patch per the above and resend.

Thanks,

-- Joe

"Joe Conway" <joseph(dot)conway(at)home(dot)com> writes:
> I'll rework the patch per the above and resend.

Too late ;-). I just finished ripping out the unneeded parts and
applying.

I made a few minor changes too, mostly removing unnecessary code
(you don't need to call nameout, everyone else just uses NameStr)
and trying to line up stylistically with other code. One actual
bug noted: you were doing this in a couple of places:

+ tuple = SearchSysCache(RELOID, ObjectIdGetDatum(reloid), 0, 0, 0);
+ if (!HeapTupleIsValid(tuple)) {
+ elog(ERROR, "has_table_privilege: invalid relation oid %d", (int) reloid);
+ }
+
+ relname = NameStr(((Form_pg_class) GETSTRUCT(tuple))->relname);
+
+ ReleaseSysCache(tuple);

Since relname is just a pointer into the tuple, expecting it to still
be valid after you release the syscache entry is not kosher. There are
several ways to deal with this, but what I actually did was to make use
of lsyscache.c's get_rel_name, which pstrdup()s its result to avoid this
trap.

regards, tom lane

>
> Too late ;-). I just finished ripping out the unneeded parts and
> applying.

Thanks! I take it I still need to do the documentation though ;)

>
> I made a few minor changes too, mostly removing unnecessary code
> (you don't need to call nameout, everyone else just uses NameStr)
> and trying to line up stylistically with other code. One actual
> bug noted: you were doing this in a couple of places:
>

Once again, thanks for the "important safety tips". I saw references to this
trap in the comments, and therefore should have known better. I guess only
practice makes perfect (hopefully!).

-- Joe

"Joe Conway" <joseph(dot)conway(at)home(dot)com> writes:
>> Too late ;-). I just finished ripping out the unneeded parts and
>> applying.

> Thanks! I take it I still need to do the documentation though ;)

I put in a few words in func.sgml, but feel free to improve on it.

regards, tom lane

Patch applied by Tom for oid and Name versions.

> > I have just thought of a possible compromise. Peter is right that we
> > don't want case conversion on table names that are extracted from
> > catalogs. But I think we do want it on table names expressed as string
> > literals. Could we make the assumption that table names in catalogs
> > will be of type 'name'? If so, it'd work to make two versions of the
> > has_table_privilege function, one taking type "name" and the other
> > taking type "text". The "name" version would take its input as-is,
> > the "text" version would do case folding and truncation. This would
> > work transparently for queries selecting relation names from the system
> > catalogs, and it'd also work transparently for queries using unmarked
> > string literals (which will be preferentially resolved as type "text").
> > Worst case if the system makes the wrong choice is you throw in an
> > explicit coercion to name or text. Comments?
>
> OK -- here's take #5.
>
> It "make"s and "make check"s clean against current cvs tip.
>
> There are now both Text and Name variants, and the regression test support
> is rolled into the patch. Note that to be complete wrt Name based variants,
> there are now 12 user visible versions of has_table_privilege:
>
> has_table_privilege(Text usename, Text relname, Text priv_type)
> has_table_privilege(Text usename, Name relname, Text priv_type)
> has_table_privilege(Name usename, Text relname, Text priv_type)
> has_table_privilege(Name usename, Name relname, Text priv_type)
> has_table_privilege(Text relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Name relname, Text priv_type) /* assumes current_user */
> has_table_privilege(Text usename, Oid reloid, Text priv_type)
> has_table_privilege(Name usename, Oid reloid, Text priv_type)
> has_table_privilege(Oid reloid, Text priv_type) /* assumes current_user */
> has_table_privilege(Oid usesysid, Text relname, Text priv_type)
> has_table_privilege(Oid usesysid, Name relname, Text priv_type)
> has_table_privilege(Oid usesysid, Oid reloid, Text priv_type)
>
> For the Text based inputs, a new internal function, get_Name is used
> (shamelessly copied from get_seq_name in sequence.c) to downcase if not
> quoted, or remove quotes if quoted, and truncate. I also added a few test
> cases for the downcasing, quote removal, and Name based variants to the
> regression test.
>
> Only thing left (I hope!) is documentation. I'm sure I either have or can
> get the DocBook tools, but I've never used them. Would it be simpler to
> clone and hand edit one of the existing docs? Any suggestions to get me
> started?
>
> Thanks,
>
> -- Joe
>
>
>
>
>
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026