Re: has_language_privilege returns incorrect answer for non-superuser

I noticed today that has_language_privilege() returns incorrect answer
for non-superuser, e.g.:

8<---------------------------------------------------
select has_language_privilege('nobody',
'plperlu',
'usage');
has_language_privilege
------------------------
t
(1 row)

test1=# \c - nobody
You are now connected to database "test1" as user "nobody".

create function f() returns text as $$ $$ language plperlu;
ERROR: permission denied for language plperlu
8<---------------------------------------------------

I verified this behavior on head as well as 9.1 (didn't bother looking
any further back). Looks like the reason is that CreateFunction()
correctly checks lanpltrusted, whereas pg_language_aclmask() does not.

Seems like a bug to me -- opinions?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

On Tue, Jul 10, 2012 at 3:23 AM, Joe Conway <mail(at)joeconway(dot)com> wrote:
> I noticed today that has_language_privilege() returns incorrect answer
> for non-superuser, e.g.:
>
> 8<---------------------------------------------------
> select has_language_privilege('nobody',
> 'plperlu',
> 'usage');
> has_language_privilege
> ------------------------
> t
> (1 row)
>
> test1=# \c - nobody
> You are now connected to database "test1" as user "nobody".
>
> create function f() returns text as $$ $$ language plperlu;
> ERROR: permission denied for language plperlu
> 8<---------------------------------------------------
>
> I verified this behavior on head as well as 9.1 (didn't bother looking
> any further back). Looks like the reason is that CreateFunction()
> correctly checks lanpltrusted, whereas pg_language_aclmask() does not.
>
> Seems like a bug to me -- opinions?

Definitely seems like a bug to me, yes.

And while I haven't verified that the suggested fix actually fixes it
for me, it sounds reasonable :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 07/10/2012 12:50 AM, Magnus Hagander wrote:
> On Tue, Jul 10, 2012 at 3:23 AM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> I verified this behavior on head as well as 9.1 (didn't bother looking
>> any further back). Looks like the reason is that CreateFunction()
>> correctly checks lanpltrusted, whereas pg_language_aclmask() does not.
>>
>> Seems like a bug to me -- opinions?
>
> Definitely seems like a bug to me, yes.
>
> And while I haven't verified that the suggested fix actually fixes it
> for me, it sounds reasonable :)

I realized there is a somewhat analogous situation with schema objects
and schema USAGE permission. I.e. I find this understandable but surprising:

8<----------------------------------
test1=> \c - postgres
You are now connected to database "test1" as user "postgres".
test1=# select has_table_privilege('nobody','sf.foo','select');
has_table_privilege
---------------------
f
(1 row)

test1=# grant select on table sf.foo to nobody;
GRANT
test1=# select has_table_privilege('nobody','sf.foo','select');
has_table_privilege
---------------------
t
(1 row)

test1=# \c - nobody
You are now connected to database "test1" as user "nobody".
test1=> select * from sf.foo;
ERROR: permission denied for schema sf
LINE 1: select * from sf.foo;
8<----------------------------------

So I think this boils down to what we think the output of the various
has_*_privilege() functions *should* tell you:

1) privileges possessed even though they may not
be usable
-or-
2) privileges possessed and usable

Personally I'm interested in answering the latter question -- what are
all the things role X can do and see.

But historically (and perhaps correctly) these functions have always
done the former -- so maybe all we need are some words of warning in the
documentation of these functions?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

On tis, 2012-07-10 at 15:28 -0700, Joe Conway wrote:
> So I think this boils down to what we think the output of the various
> has_*_privilege() functions *should* tell you:
>
> 1) privileges possessed even though they may not
> be usable
> -or-
> 2) privileges possessed and usable
>
> Personally I'm interested in answering the latter question -- what are
> all the things role X can do and see.
>
> But historically (and perhaps correctly) these functions have always
> done the former -- so maybe all we need are some words of warning in
> the documentation of these functions?

The second question is much more difficult to answer than the first.
You could have sepgsql in the way, for example.

The functions very clearly check whether a privilege is being held, and
elsewhere it is documented what you can do with these privileges. A
particular action might very well require multiple privileges.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tis, 2012-07-10 at 15:28 -0700, Joe Conway wrote:
>> But historically (and perhaps correctly) these functions have always
>> done the former -- so maybe all we need are some words of warning in
>> the documentation of these functions?

> The second question is much more difficult to answer than the first.
> You could have sepgsql in the way, for example.

> The functions very clearly check whether a privilege is being held, and
> elsewhere it is documented what you can do with these privileges. A
> particular action might very well require multiple privileges.

That's a fair argument, but I think it's reasonable to expect that
(1) the privileges required to do something are easily identified and
can be checked from the SQL level; (2) there's a reasonable amount of
consistency in the behavior for different object types.

In these terms, the example of needing schema usage privilege seems
like a different case from lanpltrusted. We have
has_schema_privilege(), so there's support for queries to probe that
component of privilege; and the issue is common across all object types
that live within schemas. Furthermore, client-side code would probably
need to be aware of the schema-privilege angle anyway, because if you
don't have schema usage privilege on "s", you aren't even going to be
able to name table "s.t" to the has_table_privilege function, at least
not to the name-based variants of it.

So it seems arguably reasonable to me for has_language_privilege()
to take superuserness and lanpltrusted into account, without thereby
concluding that other privilege() functions must do more than they
do today. If we don't want it to do that, then I think we ought to
offer some other function that *does* consider those things ... but
I'm not seeing the value of separating it out.

Having said that, I do think your argument has some merit with respect
to the internal pg_language_aclcheck() function. That is, I'd want
to see any code changes here made in the has_language_privilege
functions, not at the aclcheck level.

The sepgsql point is worth discussing too. I have not been paying
close attention to the sepgsql patches, but I have the distinct
impression that they create a non-examinable privilege barrier,
ie there's no way to inquire whether you have the privilege to do
X except by actually trying it. Is that really the way we want
things to go?

regards, tom lane

On tor, 2012-07-12 at 01:40 -0400, Tom Lane wrote:
> So it seems arguably reasonable to me for has_language_privilege()
> to take superuserness and lanpltrusted into account, without thereby
> concluding that other privilege() functions must do more than they
> do today. If we don't want it to do that, then I think we ought to
> offer some other function that *does* consider those things ... but
> I'm not seeing the value of separating it out.

As long as we're spending time on this, I'd propose getting rid of
lanplistrusted, at least for access checking. Instead, just don't
install USAGE privileges by default for those languages.

The reason is that there is value in having a role that can deploy
schemas, possibly containing functions in untrusted languages, without
having to be a full superuser. Just like you can have a user that can
create roles without being a superuser.

> The sepgsql point is worth discussing too. I have not been paying
> close attention to the sepgsql patches, but I have the distinct
> impression that they create a non-examinable privilege barrier,
> ie there's no way to inquire whether you have the privilege to do
> X except by actually trying it. Is that really the way we want
> things to go?

Well, that's how SELinux works too. You can inspect the labels and all
that, but nobody really knows what's going to happen until you try it.

Which is ultimately the recommended way anyway. has_*_privilege is a
bit like the access() function, which has caveats associated with it.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> As long as we're spending time on this, I'd propose getting rid of
> lanplistrusted, at least for access checking. Instead, just don't
> install USAGE privileges by default for those languages.

There's definitely something to that idea --- certainly lanpltrusted
dates from before we had a robust object-permissions system, and looks
like a bit of a wart now that we do have one.

I guess we could redefine the default privileges for languages as "none"
and then have the TRUSTED keyword mean to install public usage
privilege. Or maybe it would be safer for upgrade purposes if we kept
the default interpretation as-is and did an automatic REVOKE when
TRUSTED wasn't specified.

regards, tom lane

On 07/12/2012 02:53 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> As long as we're spending time on this, I'd propose getting rid of
>> lanplistrusted, at least for access checking. Instead, just don't
>> install USAGE privileges by default for those languages.
>
> There's definitely something to that idea --- certainly lanpltrusted
> dates from before we had a robust object-permissions system, and looks
> like a bit of a wart now that we do have one.
>
> I guess we could redefine the default privileges for languages as "none"
> and then have the TRUSTED keyword mean to install public usage
> privilege. Or maybe it would be safer for upgrade purposes if we kept
> the default interpretation as-is and did an automatic REVOKE when
> TRUSTED wasn't specified.

+1

I'll take a look at the latter option sometime in the next few weeks and
submit for the next commitfest.

Is it still worth backpatching a change to has_language_privilege as a
bug fix?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

Joe Conway <mail(at)joeconway(dot)com> writes:
> Is it still worth backpatching a change to has_language_privilege as a
> bug fix?

I think back-patching a behavioral change in this area is probably a
bad idea. We can fix it (in one way or another) going forward, but
changing this sort of thing in a minor release seems likely to have
more bad consequences than good ones.

regards, tom lane

On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
> On 07/12/2012 02:53 PM, Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> >> As long as we're spending time on this, I'd propose getting rid of
> >> lanplistrusted, at least for access checking. Instead, just don't
> >> install USAGE privileges by default for those languages.
> >
> > There's definitely something to that idea --- certainly lanpltrusted
> > dates from before we had a robust object-permissions system, and looks
> > like a bit of a wart now that we do have one.
> >
> > I guess we could redefine the default privileges for languages as "none"
> > and then have the TRUSTED keyword mean to install public usage
> > privilege. Or maybe it would be safer for upgrade purposes if we kept
> > the default interpretation as-is and did an automatic REVOKE when
> > TRUSTED wasn't specified.
>
> +1
>
> I'll take a look at the latter option sometime in the next few weeks and
> submit for the next commitfest.

Any news on this?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 08/30/2012 07:23 PM, Bruce Momjian wrote:
> On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
>> I'll take a look at the latter option sometime in the next few weeks and
>> submit for the next commitfest.
>
> Any news on this?

Not yet -- OBE. I'll try to set aside some time on the long weekend.

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

On Thu, Aug 30, 2012 at 07:59:02PM -0700, Joe Conway wrote:
> On 08/30/2012 07:23 PM, Bruce Momjian wrote:
> > On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
> >> I'll take a look at the latter option sometime in the next few weeks and
> >> submit for the next commitfest.
> >
> > Any news on this?
>
> Not yet -- OBE. I'll try to set aside some time on the long weekend.

Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Aug 30, 2012 at 07:59:02PM -0700, Joe Conway wrote:
> On 08/30/2012 07:23 PM, Bruce Momjian wrote:
> > On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
> >> I'll take a look at the latter option sometime in the next few weeks and
> >> submit for the next commitfest.
> >
> > Any news on this?
>
> Not yet -- OBE. I'll try to set aside some time on the long weekend.

Any news on this?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 1/24/13 6:25 PM, Bruce Momjian wrote:
> On Thu, Aug 30, 2012 at 07:59:02PM -0700, Joe Conway wrote:
>> On 08/30/2012 07:23 PM, Bruce Momjian wrote:
>>> On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
>>>> I'll take a look at the latter option sometime in the next few weeks and
>>>> submit for the next commitfest.
>>>
>>> Any news on this?
>>
>> Not yet -- OBE. I'll try to set aside some time on the long weekend.
>
> Any news on this?

https://commitfest.postgresql.org/action/patch_view?id=1048

On Fri, Jan 25, 2013 at 04:55:48PM -0500, Peter Eisentraut wrote:
> On 1/24/13 6:25 PM, Bruce Momjian wrote:
> > On Thu, Aug 30, 2012 at 07:59:02PM -0700, Joe Conway wrote:
> >> On 08/30/2012 07:23 PM, Bruce Momjian wrote:
> >>> On Thu, Jul 12, 2012 at 06:01:00PM -0700, Joe Conway wrote:
> >>>> I'll take a look at the latter option sometime in the next few weeks and
> >>>> submit for the next commitfest.
> >>>
> >>> Any news on this?
> >>
> >> Not yet -- OBE. I'll try to set aside some time on the long weekend.
> >
> > Any news on this?
>
> https://commitfest.postgresql.org/action/patch_view?id=1048

Joe emailed me privately to say he would work on it early next week,
"promise".

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Joe Conway wrote:
> I noticed today that has_language_privilege() returns incorrect answer
> for non-superuser, e.g.:
>
> 8<---------------------------------------------------
> select has_language_privilege('nobody',
> 'plperlu',
> 'usage');
> has_language_privilege
> ------------------------
> t
> (1 row)

Funnily enough, this is still the case in 9.6, four years later. Have
we made any inroads in fixing this?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Joe Conway wrote:
>> I noticed today that has_language_privilege() returns incorrect answer
>> for non-superuser, e.g.:
>>
>> 8<---------------------------------------------------
>> select has_language_privilege('nobody',
>> 'plperlu',
>> 'usage');
>> has_language_privilege
>> ------------------------
>> t
>> (1 row)

> Funnily enough, this is still the case in 9.6, four years later. Have
> we made any inroads in fixing this?

The reason for the discrepancy is that the check actually enforced by
CreateFunction (functioncmds.c:948) is not about USAGE if it's an
untrusted language. The user does actually have USAGE, so far as the
standard privilege system is concerned, but we're still disallowing the
function creation.

I suppose that this is one of the things that Stephen Frost would like
to normalize to be completely driven by the standard privilege system.

Possibly we could simplify CREATE FUNCTION to just check USAGE all the
time, and instead have CREATE LANGUAGE auto-revoke public USAGE if
it's not a trusted language.

regards, tom lane