Обсуждение: has_table_privilege for a table in unprivileged schema causes anerror

Hi,

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

 postgres=> select has_table_privilege('myschema.tbl','select');
 ERROR:  permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually.  It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Attached is a patch to modify the function like that. This is WIP patch, so
only has_table_previlege is modified and other familiy functions are left as
they are. Also, there is no additional test yet.

One consern on this patch is that modifying the function can break the 
back-compatibility, so it might be better to add a new parameter to
control the behavior of the function. 

Any comments would be appriciated.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

Yugo Nagata <nagata@sraoss.co.jp> writes:
> I found that has_table_privilege returns an error when a table is specified
> by schema-qualified name and the user doen't have privilege for its schema.

>  postgres=> select has_table_privilege('myschema.tbl','select');
>  ERROR:  permission denied for schema myschema

> I think that this function should return false because the user doesn't have
> the privilege on this table eventually.  It is more useful for users because
> it is not needed to parse the schema-qualified table name and check the
> privilege on the schema in advance.

Sounds reasonable, but if we're going to do that, we should do it for
every one of these functions that concerns a schema-qualifiable object
type.  Not just tables.

Also, looking at the code, why are you bothering with
convert_table_schema_priv_string?  ISTM what's relevant on the schema is
always going to be USAGE privilege, independently of the mode being
checked on the object.  So you shouldn't need a bunch of duplicative
tables.

Plus, I don't think this implementation approach is going to work for
unqualified table names.  You don't know which schema they're in until you
look them up.  (Although I vaguely remember that the path search logic just
ignores unreadable schemas, so maybe all you have to do with unqualified
names is nothing.  But that's not what this patch is doing now.)

Some test cases would likely be a good idea.

            regards, tom lane

On Thu, 16 Aug 2018 19:37:42 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yugo Nagata <nagata@sraoss.co.jp> writes:
> > I found that has_table_privilege returns an error when a table is specified
> > by schema-qualified name and the user doen't have privilege for its schema.
> 
> >  postgres=> select has_table_privilege('myschema.tbl','select');
> >  ERROR:  permission denied for schema myschema
> 
> > I think that this function should return false because the user doesn't have
> > the privilege on this table eventually.  It is more useful for users because
> > it is not needed to parse the schema-qualified table name and check the
> > privilege on the schema in advance.
> 
> Sounds reasonable, but if we're going to do that, we should do it for
> every one of these functions that concerns a schema-qualifiable object
> type.  Not just tables.

OK. I will fix all of these functions that can take a schema-qualifiable
object name as a parameter.

> 
> Also, looking at the code, why are you bothering with
> convert_table_schema_priv_string?  ISTM what's relevant on the schema is
> always going to be USAGE privilege, independently of the mode being
> checked on the object.  So you shouldn't need a bunch of duplicative
> tables.

I thought we needed to consider also USAGE GRANT OPTION, but I might be
misunderstnding something. I will look into this again.
 
> Plus, I don't think this implementation approach is going to work for
> unqualified table names.  You don't know which schema they're in until you
> look them up.  (Although I vaguely remember that the path search logic just
> ignores unreadable schemas, so maybe all you have to do with unqualified
> names is nothing.  But that's not what this patch is doing now.)

Oops. I overlooked these cases.  I'll fix the patch to allow to handle
unqualified table names.

Thanks,
 
-- 
Yugo Nagata <nagata@sraoss.co.jp>

Hi,

I updated the patch to support other has_*_privilege functions with some
refactoring, but tests for theses fixes are not included yet.

But, while running the regression test after this patch is applied, I found
that my patch doesn't pass privilege test.  One of different behaviours
is as bellow.

 -- Grant on all objects of given type in a schema
 \c -
 
 CREATE SCHEMA testns;
 CREATE TABLE testns.t1 (f1 int);
 CREATE TABLE testns.t2 (f1 int);
 
 SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- false
 
 GRANT ALL ON ALL TABLES IN SCHEMA testns TO regress_priv_user1;
 
 SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- true

This is a part of src/test/regress/sql/privileges.sql. SELECT privilege on testns.t1 
is granted to regress_priv_user1, so the has_table_privilege of the original implementation
returns true.  On the other hand, after applied my patch, the function returns false
because the regress_priv_user1 doesn't have USAGE privilege on schema testns. Accually, 
the user can not access to the table using SELECT statement. 

Although the behavior of the original function would reflect pg_class.relacl, it seems to
me that the function fixed in my patch is more useful for users because this reflects
the actual accessibility during query execution.

Is there chance to change the behaviour of the functions as I am proposing?

If is is not allowed to change it to keep back-compatibility, I would like to propose
to add a parameter to the function to consider the privilege of the schema, for
example as bellow. Assuming false as the default values will keep the back-compatibility.

 has_table_privilege(user, table, privilege[, consider_schema=false]) 

On Fri, 17 Aug 2018 12:02:57 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> On Thu, 16 Aug 2018 19:37:42 -0400
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Yugo Nagata <nagata@sraoss.co.jp> writes:
> > > I found that has_table_privilege returns an error when a table is specified
> > > by schema-qualified name and the user doen't have privilege for its schema.
> > 
> > >  postgres=> select has_table_privilege('myschema.tbl','select');
> > >  ERROR:  permission denied for schema myschema
> > 
> > > I think that this function should return false because the user doesn't have
> > > the privilege on this table eventually.  It is more useful for users because
> > > it is not needed to parse the schema-qualified table name and check the
> > > privilege on the schema in advance.
> > 
> > Sounds reasonable, but if we're going to do that, we should do it for
> > every one of these functions that concerns a schema-qualifiable object
> > type.  Not just tables.
> 
> OK. I will fix all of these functions that can take a schema-qualifiable
> object name as a parameter.

In addition to has_table_privilege, has_any_column_privilege, has_column_privilege,
has_function_privilege, and has_sequence_privilege are fixed.

> > 
> > Also, looking at the code, why are you bothering with
> > convert_table_schema_priv_string?  ISTM what's relevant on the schema is
> > always going to be USAGE privilege, independently of the mode being
> > checked on the object.  So you shouldn't need a bunch of duplicative
> > tables.
> 
> I thought we needed to consider also USAGE GRANT OPTION, but I might be
> misunderstnding something. I will look into this again.

Fixed to check only USAGE privilege on the schema.

> > Plus, I don't think this implementation approach is going to work for
> > unqualified table names.  You don't know which schema they're in until you
> > look them up.  (Although I vaguely remember that the path search logic just
> > ignores unreadable schemas, so maybe all you have to do with unqualified
> > names is nothing.  But that's not what this patch is doing now.)
> 
> Oops. I overlooked these cases.  I'll fix the patch to allow to handle
> unqualified table names.

Fixed.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

On Fri, Aug 24, 2018 at 5:31 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> Although the behavior of the original function would reflect pg_class.relacl, it seems to
> me that the function fixed in my patch is more useful for users because this reflects
> the actual accessibility during query execution.

I'm not sure that it's a good idea to change this behavior.

In the case of an unqualified name, the permissions on the schemas in
the search path can affect which table is chosen in the first place.
For instance, suppose bob has search_path = a, b and has usage
permission on b but not on a.  In that case, if both a.x and b.x
exist, bob's reference to x will latch onto b.x, ignoring a.x
completely.  So, if I'm not mistaken, this change will never make any
difference for an unqualified name, because if you don't have usage
permission on the schema, you'll never decide that an unqualified name
references something in that schema in the first place.  So I think
this only matters for qualified names.

And if you've got a qualified name, you know what schema it's in.  If
you are concerned about a.b, nothing keeps you from writing a test
against schema a's permissions as well as a test against table a.b's
permissions.  But on the other hand, if for some reason you want to
know about pg_class.relacl specifically, then having the function
consider both that and the schema's ACL could be awkward.

Also, the current system generally tries not to reveal any information
about the contents of schemas for which you have no permissions.  For
instance, suppose there is a table a.x:

rhaas=> select has_table_privilege('a.x', 'select');
ERROR:  permission denied for schema a
rhaas=> select has_table_privilege('a.nonexistent', 'select');
ERROR:  permission denied for schema a

With this change, you could potentially learn something about which
tables exist in a schema to which you have no access.  You could argue
that this is harmless; after all, right now, an unprivileged user can
run 'select * from pg_class', so the jig is up anyway.  But that might
be something upon which we'd like to improve someday.

I admit that these are only mild disadvantages, but I think we should
reject breaking backward compatibility unless the result is a clear
improvement, and if anything this seems slightly worse to me.  YMMV,
of course....

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> I'm not sure that it's a good idea to change this behavior.

> In the case of an unqualified name, the permissions on the schemas in
> the search path can affect which table is chosen in the first place.
> ... So I think this only matters for qualified names.

Yeah, that agrees with my expectations.

> Also, the current system generally tries not to reveal any information
> about the contents of schemas for which you have no permissions.

I don't think that argument holds up, at least not if this is implemented
the way I'd expect.  It would change the results for a schema on which
you lack usage permission from "permission denied for schema a" to
"false", but it still would not matter whether there is such a table
inside "a".

> And if you've got a qualified name, you know what schema it's in.  If
> you are concerned about a.b, nothing keeps you from writing a test
> against schema a's permissions as well as a test against table a.b's
> permissions.  But on the other hand, if for some reason you want to
> know about pg_class.relacl specifically, then having the function
> consider both that and the schema's ACL could be awkward.

Mmm ... maybe, but I don't think that exactly holds water either, given
that the current behavior is to fail if you lack permission on schema a.
Yes, you can write "case when has_schema_privilege() then
has_table_privilege() else false end", but if you're worried that you
might possibly lack schema privilege, then the current behavior of
has_table_privilege is useless to you: it doesn't matter whether or not
you would like to know about pg_class.relacl specifically, because you
won't be allowed to find out.

Also, in some use-cases the extra test would require writing code that can
split a qualified name into pieces, which isn't really all that easy in
SQL.

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here.  And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

            regards, tom lane

On Sat, 25 Aug 2018 23:29:27 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robert Haas <robertmhaas@gmail.com> writes:
> > I'm not sure that it's a good idea to change this behavior.
> 
> > In the case of an unqualified name, the permissions on the schemas in
> > the search path can affect which table is chosen in the first place.
> > ... So I think this only matters for qualified names.
> 
> Yeah, that agrees with my expectations.

Yes. I consider only the cases of qualified names and the patch doesn't 
change any behavior about unqualified name cases.

> > Also, the current system generally tries not to reveal any information
> > about the contents of schemas for which you have no permissions.
> 
> I don't think that argument holds up, at least not if this is implemented
> the way I'd expect.  It would change the results for a schema on which
> you lack usage permission from "permission denied for schema a" to
> "false", but it still would not matter whether there is such a table
> inside "a".

Yes, Tom's explanation is right. The proposal functions doesn't reveal
any information about the contents of unprivileged schemas, either.

> 
> > And if you've got a qualified name, you know what schema it's in.  If
> > you are concerned about a.b, nothing keeps you from writing a test
> > against schema a's permissions as well as a test against table a.b's
> > permissions.  But on the other hand, if for some reason you want to
> > know about pg_class.relacl specifically, then having the function
> > consider both that and the schema's ACL could be awkward.
> 
> Mmm ... maybe, but I don't think that exactly holds water either, given
> that the current behavior is to fail if you lack permission on schema a.
> Yes, you can write "case when has_schema_privilege() then
> has_table_privilege() else false end", but if you're worried that you
> might possibly lack schema privilege, then the current behavior of
> has_table_privilege is useless to you: it doesn't matter whether or not
> you would like to know about pg_class.relacl specifically, because you
> won't be allowed to find out.
> 
> Also, in some use-cases the extra test would require writing code that can
> split a qualified name into pieces, which isn't really all that easy in
> SQL.

This is a reason why we proposed to fix the function. However, with regard to
splitting a qualified name, making a new SQL function to do this might resolve
it, for example, as below.
 
 select case when has_schema_privilege(x.nspname) 
   then has_table_privilege(x.objname)
   else false end
  from pg_split_qualified_name(tablename) x;

> There's a backwards-compatibility argument for not changing this behavior,
> sure, but I don't buy the other arguments you made here.  And I don't
> think there's all that much to the backwards-compatibility argument,
> considering that the current behavior is to fail.

With regarding to keeping the backwards-compatibility, to add a new paramater
to has_*_privilege functions is a solution as proposed previously.

 has_table_privilege(user, table, privilege[, consider_schema=false]) 

How do you think this proposal?

Regards,

-- 
Yugo Nagata <nagata@sraoss.co.jp>

Yugo Nagata <nagata@sraoss.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There's a backwards-compatibility argument for not changing this behavior,
>> sure, but I don't buy the other arguments you made here.  And I don't
>> think there's all that much to the backwards-compatibility argument,
>> considering that the current behavior is to fail.

> With regarding to keeping the backwards-compatibility, to add a new paramater
> to has_*_privilege functions is a solution as proposed previously.
>  has_table_privilege(user, table, privilege[, consider_schema=false])
> How do you think this proposal?

I think that'd be a disaster, because we already have variants of these
functions with several different parameter counts.  Adding optional
arguments to them will cause ambiguous-function errors where there were
none before.

Also, it's just plain ugly.  We should either decide we want this change,
or decide we don't.  Trying to have it both ways is not going to be
better.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Sat, Aug 25, 2018 at 8:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There's a backwards-compatibility argument for not changing this behavior,
>> sure, but I don't buy the other arguments you made here.  And I don't
>> think there's all that much to the backwards-compatibility argument,
>> considering that the current behavior is to fail.

> +1; any code using these functions can reasonably be expected to handle
> true and false responses.  Converting a present error into a false under
> that presumption results in similar, and arguably improved, semantics.

I took a closer look at what's going on here, and realized that the
existing code is a bit confused, or confusing, about whose privileges
it's testing.  Consider this exchange (with HEAD, not the patch):

regression=# CREATE SCHEMA testns;
CREATE SCHEMA
regression=# CREATE TABLE testns.t1 (f1 int);
CREATE TABLE
regression=# CREATE USER regress_priv_user1;
CREATE ROLE
regression=# SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT');
 has_table_privilege
---------------------
 f
(1 row)

regression=# \c - regress_priv_user1
You are now connected to database "regression" as user "regress_priv_user1".
regression=> SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT');
ERROR:  permission denied for schema testns

That is, whether you get an error or not depends on your *own*
privileges for the schema, not those of the user you're supposedly
testing.  This seems rather inconsistent.  If we apply the proposed
patch, then (I'd expect, but haven't tested) you should always get
the same results from has_table_privilege('u', 's.t', 'p') as from
doing has_table_privilege('s.t', 'p') as user u.

However ... if we do that, then Robert's previous concern about
information leakage comes to life, because an unprivileged user
could run has_table_privilege('postgres', 'testns.t1', 'SELECT')
and thereby find out whether t1 exists regardless of whether he
has any privilege for testns.

Mind you, I think that argument is mostly bunk, because that
same user can trivially find out whether t1 exists, and what
its privilege grants are, just by looking into pg_catalog.
So there's no real security improvement from disallowing this.

Anyway, it seems to me that the principle of least astonishment
suggests that the results of has_table_privilege should depend only
on the privileges of the user allegedly being tested, not those of
the calling user.  Or if we decide differently, somebody has to write
some doc text explaining that it's not so.

Getting all the way to that might be a bit difficult though.
For example, in

SELECT has_function_privilege('someuser', 'func(schema.type)', 'usage');

the lookup and permission check for "schema" are buried a long
way down from the has_function_privilege code.  It'd take a
lot of refactoring to keep it from throwing an error.  I guess
maybe you could argue that privileges on the type are a different
question from privileges on the function, but it still seems ugly.

A related thought is that it's not clear whether there should be
any behavioral difference between

SELECT has_function_privilege('someuser', 'func(schema.type)'::text, 'usage');

SELECT has_function_privilege('someuser', 'func(schema.type)'::regprocedure, 'usage');

In the latter case, it's entirely unsurprising that the schema lookup
is done as the current user.  However, if we define the first case
as being equivalent to the second, then the error that Yugo-san is
complaining of is something we can't/shouldn't fix, because certainly
"SELECT 'testns.t1'::regclass" is going to throw an error if you lack
usage privilege for testns.

So at this point I'm not sure what to think, other than that things
are inconsistent (and underdocumented).

            regards, tom lane

On Sun, Nov 18, 2018 at 06:32:42PM -0500, Tom Lane wrote:
> So at this point I'm not sure what to think, other than that things
> are inconsistent (and underdocumented).

Nagata-san, do you have some plans to do something about the comments
raised.  The thread has been inactive for a couple of months now, so I
am marking the patch as returned with feedback.
--
Michael