Thread: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal

[ -> 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 notationsrel.funcfunc(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

> 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

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@gmx.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
QuotedRelname", 'insert');
 

instead of
  select has_table_privilege(current_user, 'pg_class', 'insert');    or  select has_table_privilege(current_user, '"My
QuotedRelname"',
 
'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@conway-family.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

> 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@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

> 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

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@gmx.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@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Peter Eisentraut <peter_e@gmx.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@gmx.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.conway@home.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.conway@home.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