Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

We allow non-superuser database owners to execute CREATE LANGUAGE for a
trusted language (one marked as tmpldbacreate in pg_pltemplate).
Currently, the C-language support functions for the language end up owned
by that non-superuser. This is on the hairy edge of being a security
hole, since generally it's supposed that a function owner can redefine the
function. Could the non-superuser alter the function into a state where
it can be used unsafely? A non-superuser cannot directly execute CREATE
OR REPLACE FUNCTION with LANGUAGE set to C, but what can he do if he owns
a function that already has that setting?

One possible attack path is to use ALTER FUNCTION RENAME, which with a
C language function might be thought to change the target entry point in
the language's shared library, thus leading at least to a server crash and
possibly to undesirable execution of C-level code. But actually that
won't happen, because the target is identified by pg_proc.prosrc which is
set up from the function name at CREATE time and isn't changed by RENAME.
Besides, the functions at issue here are created in the pg_catalog schema,
and non-superusers do not have permission to rename anything in
pg_catalog.

One thing the owner *can* do is use ALTER FUNCTION to change secondary
properties of the function, such as strictness, volatility, SECURITY
DEFINER, etc. So far as I can see, none of these properties are examined
for a PL support function when it is used to call or validate a function
in the language, so this doesn't constitute a security hole either.
Still, it's not very hard to envision innocent-looking extensions to ALTER
FUNCTION that might result in live security holes here.

A different line of attack is to replace the function altogether with
CREATE OR REPLACE FUNCTION, using a non-C-language definition. It would
no longer be a gateway to executing non-SQL code ... but it would still be
referenced by the PL. In this way the function owner could insert
trojan-horse code that would be executed whenever somebody else tried to
define or use a function written in the PL. It turns out that the
placement of the support functions in pg_catalog saves us from this too,
but that seems rather accidental to me; it's not immediately obvious that
a REPLACE operation on an existing object should require CREATE rights on
the containing schema.

In short, neither I nor anybody else on the PG security list have been
able to think of an exploitable security issue here, but we're not
entirely convinced that there isn't one. Can anyone think of an attack
vector we missed?

As of 9.2, there is a new hazard of the same ilk, namely the constructor
functions for range types, which are language INTERNAL and are being
created as owned by the type's creator. This seems potentially a worse
hole than the CREATE LANGUAGE case, in that any random SQL user can create
a range type, not only the database owner (who might be assumed to be at
least somewhat trustworthy). Furthermore, because these functions aren't
created in pg_catalog but in the type's creation schema, the protections
afforded to functions in pg_catalog don't help us. I still don't see
any exploitable security hole from ALTER FUNCTION, but it is definitely
possible for a range type's creator to replace a constructor function with
a trojan horse. The significance of that is debatable though, since you
more or less have to trust a type's creator anyway if you use any of its
functions. (An example is that a domain's creator can trivially insert
trojan horse functions into the domain's CHECK constraints.)

Whether or not there is a live security hole in existing releases,
it seems clear that we could easily create one by accident in future.
To forestall that, I suggest that we should modify CREATE LANGUAGE and
CREATE RANGE TYPE to mark the support functions as owned by the bootstrap
superuser, not the caller of the CREATE operation. This would ensure that
non-superusers couldn't muck around with the function definitions.
(Dropping the language or type still works, since cascade deletions don't
pay attention to who owns an object that the delete cascades to.)
At present it seems sufficient to patch this in HEAD, along the lines of
the attached proposed patch.

If anyone can think of a workable attack against existing releases, we
will need to back-patch some form of this change, and also advise DBAs
to manually alter the ownership of existing language support functions.
That would be enough of a pain in the rear that I don't want to counsel
DBAs to do it unless there's a demonstrable need.

Another point here is that if we replace the current pg_pltemplate-based
method of creating trusted languages, we will need to be sure that the
created C functions always end up owned by a superuser, else the problem
comes back again. That will be a matter to consider when we think about
how CREATE EXTENSION works for that case.

Comments?

regards, tom lane

On Wed, May 30, 2012 at 12:02:06PM -0400, Tom Lane wrote:
> One thing the owner *can* do is use ALTER FUNCTION to change secondary
> properties of the function, such as strictness, volatility, SECURITY
> DEFINER, etc. So far as I can see, none of these properties are examined
> for a PL support function when it is used to call or validate a function
> in the language, so this doesn't constitute a security hole either.
> Still, it's not very hard to envision innocent-looking extensions to ALTER
> FUNCTION that might result in live security holes here.

I wondered about ALTER FUNCTION SET guc = '...' and tried to test it:

CREATE FUNCTION f(out ret text) RETURNS text LANGUAGE plpgsql AS
'BEGIN ret := current_setting(''work_mem''); END';
ALTER FUNCTION plpgsql_call_handler() SET work_mem = '2MB';
SELECT f();

However, that test hit a SIGSEGV with stack corruption:

#0 DirectFunctionCall1Coll (func=0x477004 <hashoid>, collation=0, arg1=65551) at fmgr.c:1018
#1 0x00000000007246c5 in CatalogCacheComputeHashValue (cache=0xc61398, nkeys=<value optimized out>, cur_skey=0x7fff026ed390) at catcache.c:210
#2 0x00000000007257c6 in SearchCatCache (cache=0xc61398, v1=65551, v2=0, v3=0, v4=0) at catcache.c:1091
#3 0x00000000007304e2 in SearchSysCache (cacheId=0, key1=65551, key2=0, key3=0, key4=0) at syscache.c:859
#4 0x000000000073d382 in fmgr_info_cxt_security (functionId=0, finfo=0x7fff026ed650, mcxt=<value optimized out>, ignore_security=0 '\000') at fmgr.c:214
#5 0x000000000073d939 in fmgr_info_cxt (functionId=4681732, finfo=0x0, mcxt=0x1000f) at fmgr.c:171
#6 0x000000000073d94e in fmgr_info (functionId=4681732, finfo=0x0) at fmgr.c:161
#7 0x000000000073d890 in fmgr_info_other_lang (functionId=65555, finfo=0x7fcc68bd54d8, mcxt=<value optimized out>, ignore_security=<value optimized out>) at fmgr.c:408
#8 fmgr_info_cxt_security (functionId=65555, finfo=0x7fcc68bd54d8, mcxt=<value optimized out>, ignore_security=<value optimized out>) at fmgr.c:290
#9 0x000000000073e8dd in fmgr_security_definer (fcinfo=0xccae20) at fmgr.c:901
#10 0x000000000073eb24 in fmgr_security_definer (fcinfo=0x477004) at fmgr.c:968
#11 0x000000000073eb24 in fmgr_security_definer (fcinfo=0x477004) at fmgr.c:968
...
#2526 0x000000000073eb24 in fmgr_security_definer (fcinfo=0x477004) at fmgr.c:968
...

> In short, neither I nor anybody else on the PG security list have been
> able to think of an exploitable security issue here, but we're not
> entirely convinced that there isn't one. Can anyone think of an attack
> vector we missed?

Stepping outside the special case of language support functions, it follows
that ALTER FUNCTION OWNER TO on a C-language function conveys more trust than
meets the eye:

BEGIN;
CREATE ROLE alice;
CREATE FUNCTION mylen(text) RETURNS integer LANGUAGE internal IMMUTABLE STRICT AS 'textlen';
ALTER FUNCTION mylen(text) OWNER TO alice;
COMMIT;

SET SESSION AUTHORIZATION alice;
ALTER FUNCTION mylen(text) CALLED ON NULL INPUT;
SELECT mylen(NULL); -- SIGSEGV

CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
user's untrusted-language SECURITY DEFINER function. ALTER FUNCTION CALLED ON
NULL INPUT ought to require that the user be eligible to redefine the function
completely. (The argument types for procedural language support functions
keep this from affecting them directly.)

> Whether or not there is a live security hole in existing releases,
> it seems clear that we could easily create one by accident in future.
> To forestall that, I suggest that we should modify CREATE LANGUAGE and
> CREATE RANGE TYPE to mark the support functions as owned by the bootstrap
> superuser, not the caller of the CREATE operation.

Sounds like a clear improvement for CREATE LANGUAGE, seeing those functions go
into pg_catalog. I'm less sure about CREATE RANGE TYPE, but I cannot think of
a concrete reason not to do so.

Thanks,
nm

Noah Misch <noah(at)leadboat(dot)com> writes:
> I wondered about ALTER FUNCTION SET guc = '...' and tried to test it:

> CREATE FUNCTION f(out ret text) RETURNS text LANGUAGE plpgsql AS
> 'BEGIN ret := current_setting(''work_mem''); END';
> ALTER FUNCTION plpgsql_call_handler() SET work_mem = '2MB';
> SELECT f();

Huh, interesting. I coulda sworn I tried exactly that case a couple
days ago, but I must have done something different.

> However, that test hit a SIGSEGV with stack corruption:

It's not so much that the stack is corrupted as that it recurses until
the stack overflows. fmgr_info_cxt_security sees that f() is of a
non-builtin language, so it calls fmgr_info_other_lang, which calls
fmgr_info for the call handler, which goes back to
fmgr_info_cxt_security, which sees that the call handler has SET
options, so it opts to use fmgr_security_definer for invoking the
call handler. But back at fmgr_info_other_lang, we expect fn_addr
to be pointing directly at the call handler. So instead of running
the call handler, we run fmgr_security_definer. It thinks that
the function it's supposed to call is identified by
fcinfo->flinfo->fn_oid, ie the original function f() not the call
handler, so it now tries to call that function, and starts the
whole lookup process over again. Lather, rinse, repeat.

So this seems to be a shortcoming in the fmgr.c stuff: it's not really
prepared for the possibility that a PL handler function has got any
of the attributes that would trigger use of fmgr_security_definer.

I think the most expedient fix for this would be to just ignore those
attributes, by having fmgr_info_other_lang invoke fmgr_info_cxt_security
with ignore_security = true while looking up the PL handler. We could
alternatively add a test to see if we got back a pointer to
fmgr_security_definer, but that would require doing a function pointer
comparison which I believe to not be very reliable.

So this is a security issue after all, to the extent that you can crash
the server this way --- there's definitely no possibility of doing
something else, since the recursion-to-overflow is certain.

regards, tom lane

On Wed, May 30, 2012 at 07:34:16PM -0400, Noah Misch wrote:
> ALTER FUNCTION OWNER TO on a C-language function conveys more trust than
> meets the eye:
>
> BEGIN;
> CREATE ROLE alice;
> CREATE FUNCTION mylen(text) RETURNS integer LANGUAGE internal IMMUTABLE STRICT AS 'textlen';
> ALTER FUNCTION mylen(text) OWNER TO alice;
> COMMIT;
>
> SET SESSION AUTHORIZATION alice;
> ALTER FUNCTION mylen(text) CALLED ON NULL INPUT;
> SELECT mylen(NULL); -- SIGSEGV
>
> CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
> user's untrusted-language SECURITY DEFINER function. ALTER FUNCTION CALLED ON
> NULL INPUT ought to require that the user be eligible to redefine the function
> completely.

Here's a patch implementing that restriction. To clarify, I see no need to
repeat *all* the CREATE-time checks; for example, there's no need to recheck
permission to use the return type. The language usage check is enough.

I didn't feel the need to memorialize a test like the above in an actual
regression test, but that's the one I used to verify the change.

Considering the crash potential, I'd recommend backpatching this.

Thanks,
nm

Noah Misch <noah(at)leadboat(dot)com> writes:
>> CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
>> user's untrusted-language SECURITY DEFINER function. ALTER FUNCTION CALLED ON
>> NULL INPUT ought to require that the user be eligible to redefine the function
>> completely.

> Here's a patch implementing that restriction. To clarify, I see no need to
> repeat *all* the CREATE-time checks; for example, there's no need to recheck
> permission to use the return type. The language usage check is enough.

This seems bizarre and largely unnecessary. As you stated to begin
with, granting ownership of a function implies some degree of trust.
I do not want to get into the business of parsing exactly which variants
of ALTER FUNCTION ought to be considered safe. And I definitely don't
want to add a check that enforces restrictions against cases that have
got nothing whatever to do with C-language functions, as this patch
does.

regards, tom lane

On Mon, Jun 11, 2012 at 11:03:10PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> >> CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
> >> user's untrusted-language SECURITY DEFINER function. ALTER FUNCTION CALLED ON
> >> NULL INPUT ought to require that the user be eligible to redefine the function
> >> completely.
>
> > Here's a patch implementing that restriction. To clarify, I see no need to
> > repeat *all* the CREATE-time checks; for example, there's no need to recheck
> > permission to use the return type. The language usage check is enough.
>
> This seems bizarre and largely unnecessary. As you stated to begin
> with, granting ownership of a function implies some degree of trust.

Yes, but I would never expect that level of trust to include access to crash
the server as a consequence of the function's reliance on STRICT.

> I do not want to get into the business of parsing exactly which variants
> of ALTER FUNCTION ought to be considered safe.

Fair concern.

> And I definitely don't
> want to add a check that enforces restrictions against cases that have
> got nothing whatever to do with C-language functions, as this patch
> does.

We don't have a principled basis for assuming that this hazard cannot apply to
third-party untrusted languages. We could add another pg_language flag to
make the distinction for languages like plperlu. They're untrusted by virtue
of granted access beyond the database, but no mismatch between the function
definition and the function implementation can crash the server or similar.
Adding such a thing at this point seems excessive to me.

On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > Here's a patch implementing that restriction.  To clarify, I see no need to
>> > repeat *all* the CREATE-time checks; for example, there's no need to recheck
>> > permission to use the return type.  The language usage check is enough.
>>
>> This seems bizarre and largely unnecessary.  As you stated to begin
>> with, granting ownership of a function implies some degree of trust.
>
> Yes, but I would never expect that level of trust to include access to crash
> the server as a consequence of the function's reliance on STRICT.

+1. Crashes are bad.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>> This seems bizarre and largely unnecessary. As you stated to begin
>>> with, granting ownership of a function implies some degree of trust.

>> Yes, but I would never expect that level of trust to include access to crash
>> the server as a consequence of the function's reliance on STRICT.

> +1. Crashes are bad.

C functions, by definition, carry a risk of crashing the server.
I cannot fathom the reasoning why we should consider that granting
ownership of one to an untrustworthy user is ever a good idea, let alone
something we promise to protect you from any bad consequences of.

Even if I accepted that premise, this patch is a pretty bad
implementation of it, because it restricts cases that there is no
reason to think are unsafe.

A less bizarre and considerably more future-proof restriction, IMO,
would simply refuse any attempt to give ownership of a C function
to a non-superuser.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> A less bizarre and considerably more future-proof restriction,
> IMO, would simply refuse any attempt to give ownership of a C
> function to a non-superuser.

We have C replication trigger functions where this would be a bad
thing. They can't work properly as SECURITY INVOKER, and I see it
as a big step backwards in security to make the only other option
SECURITY DEFINER with a superuser as the owner. It's not too hard
to come up with other use cases where you want to grant one class of
users rights to do something only through a certain function, not
directly.

So there is clearly a need to support ownership of functions,
including C functions, by users who are effectively at an
"intermediate" level of trust. We could conceivably use the
database owner for that role, but that seem unnecessarily limiting.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> A less bizarre and considerably more future-proof restriction,
>> IMO, would simply refuse any attempt to give ownership of a C
>> function to a non-superuser.

> We have C replication trigger functions where this would be a bad
> thing. They can't work properly as SECURITY INVOKER, and I see it
> as a big step backwards in security to make the only other option
> SECURITY DEFINER with a superuser as the owner.

Could you provide more details about that? If nothing else, this
could be handled with a non-C wrapper function, but I'm not clear
on the generality of the use-case.

> It's not too hard
> to come up with other use cases where you want to grant one class of
> users rights to do something only through a certain function, not
> directly.

Generally I'd imagine that that has something to do with permission
to call the function, not with who owns it.

Basically, if we go down the road Noah is proposing, I foresee a steady
stream of security bugs and ensuing random restrictions on what function
owners can do. I do not like that future.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:

>> We have C replication trigger functions where this would be a bad
>> thing. They can't work properly as SECURITY INVOKER, and I see
>> it as a big step backwards in security to make the only other
>> option SECURITY DEFINER with a superuser as the owner.
>
> Could you provide more details about that?

We have a capture_replication_data() trigger function that we attach
to each table which is to be replicated as the first AFTER EACH ROW
trigger for INSERT OR UPDATE OR DELETE. It records the data
involved in the primitive operation against the row for logical
replication at the row level. We don't want users to be able to
modify or even view the captured data in the replication tables
except through this function. (It's actually a bit more complicated
than that because of transaction metadata, but the overall concept
is the same.)

We currently use the database owner for the owner of these SECURITY
DEFINER functions, but it seems to me that there could be legitimate
reasons to create a special user with more limited rights than the
database owner in some cases -- just to ensure that a mistake in the
coding of a function doesn't open up an unnecessarily large security
hole.

> If nothing else, this could be handled with a non-C wrapper
> function, but I'm not clear on the generality of the use-case.

I'm not so sure that this would work for a generalized trigger
function that can be attached to any table like this.

>> It's not too hard to come up with other use cases where you want
>> to grant one class of users rights to do something only through a
>> certain function, not directly.
>
> Generally I'd imagine that that has something to do with
> permission to call the function, not with who owns it.

Well, it's a matter of fail-safe techniques. You grant execute
permission for the function to a the role(s) which should be allowed
to do it only through the function. But do you then necessarily
want the function to execute with unlimited rights, or with the most
restricted set of rights which allows it to perform the intended
function?

> Basically, if we go down the road Noah is proposing, I foresee a
> steady stream of security bugs and ensuing random restrictions on
> what function owners can do. I do not like that future.

I do see your point, but I don't like the solution you proposed.

As I understand it, the problem Noah is trying to address is that if
we created a "replication_manager" role for owning these functions,
instead of using the database owner, that role could alter a C
function which isn't coded to handle NULL input to allow it to be
called on NULL input anyway. Is that right?

The first solution which comes to mind for me is to allow a C
function to be compiled with that limitation -- so that *nobody*
could set the wrong option for it. Then I realize that you already
can test for this in a C function and return NULL if any inputs are
NULL if you want to. Rather than trying to enforce this in the
ALTER FUNCTION implementation, maybe we should just advise that if
you're going to grant ownership of a C function to a role which
might accidentally or maliciously allow it to be called with NULL
input, the C function should return NULL in that case.

-Kevin

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> > It's not too hard
> > to come up with other use cases where you want to grant one class of
> > users rights to do something only through a certain function, not
> > directly.
>
> Generally I'd imagine that that has something to do with permission
> to call the function, not with who owns it.

What I believe Kevin is getting at here is this:

There's no way to say "run this function as user X" except by making it
SECURITY DEFINER and owned by the user you want the function to run as.

I believe everyone agrees that only a superuser should be allowed
CHANGE a C-language function. Unfortunately, being the 'OWNER' conveys
more than just the ability to change the function.

If we had an independent way to have the function run as a specific
user, where that user DIDN'T own the function, I think Kevin's use case
would be satisfied.

I'm not sure if it's be reasonable for a C-language function to just go
ahead and decide to change the user it's running as in the database..
I have to admit that I don't think I've ever tried to do that.

> Basically, if we go down the road Noah is proposing, I foresee a steady
> stream of security bugs and ensuing random restrictions on what function
> owners can do. I do not like that future.

I agree that we don't want to have to police what a function owner can
do to a function, and therefore untrusted language functions should only
be owned by superusers, but I feel that means we need to look at what
function ownership currently implies and allows and consider if those
operations should be broken out and made independently grantable. When
it comes to 'SECURITY DEFINER' and it's relationship to 'OWNER', I think
we have to provide some kind of solution that doesn't require those
functions to be run as superuser simply because the function has to be
owned by a superuser.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> What I believe Kevin is getting at here is this:

> There's no way to say "run this function as user X" except by making it
> SECURITY DEFINER and owned by the user you want the function to run as.

> If we had an independent way to have the function run as a specific
> user, where that user DIDN'T own the function, I think Kevin's use case
> would be satisfied.

Interesting thought. I'm not exactly sure who should be allowed to
apply the "RUN AS other-user" option to a function, but I can see the
possible value of separating the right to modify the function's
definition from the user the function runs as. Kevin, does this seem
like it would address your concern?

regards, tom lane

>Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> If we had an independent way to have the function run as a
> specific user, where that user DIDN'T own the function, I think
> Kevin's use case would be satisfied.

I agree. I'm not sure quite what that would look like, but maybe
SECURITY ROLE <rolename> or some such could be an alternative to
SECURITY INVOKER and SECURITY DEFINER. (I haven't looked to see
what the standard has here.)

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Could you provide more details about that?

> We have a capture_replication_data() trigger function that we attach
> to each table which is to be replicated as the first AFTER EACH ROW
> trigger for INSERT OR UPDATE OR DELETE. It records the data
> involved in the primitive operation against the row for logical
> replication at the row level. We don't want users to be able to
> modify or even view the captured data in the replication tables
> except through this function. (It's actually a bit more complicated
> than that because of transaction metadata, but the overall concept
> is the same.)

> We currently use the database owner for the owner of these SECURITY
> DEFINER functions, but it seems to me that there could be legitimate
> reasons to create a special user with more limited rights than the
> database owner in some cases -- just to ensure that a mistake in the
> coding of a function doesn't open up an unnecessarily large security
> hole.

I'm not entirely following here. If the function is coded in C, then
it can pretty much do what it pleases independently of what user ID
is thought to be executing it at the SQL level. That would really only
matter if you were doing some SQL stuff via the SPI interface --- and
if that's the case, couldn't the C function set the appropriate role
to use for itself, anyway? (In other words, it's not that hard to build
a "RUN AS other-user" feature into a C function, even without any support
from the rest of the system.)

> Rather than trying to enforce this in the
> ALTER FUNCTION implementation, maybe we should just advise that if
> you're going to grant ownership of a C function to a role which
> might accidentally or maliciously allow it to be called with NULL
> input, the C function should return NULL in that case.

Yeah, the just-code-defensively option is worth considering too.

regards, tom lane

Excerpts from Kevin Grittner's message of mar jun 12 17:08:09 -0400 2012:
> >Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> > If we had an independent way to have the function run as a
> > specific user, where that user DIDN'T own the function, I think
> > Kevin's use case would be satisfied.
>
> I agree. I'm not sure quite what that would look like, but maybe
> SECURITY ROLE <rolename> or some such could be an alternative to
> SECURITY INVOKER and SECURITY DEFINER. (I haven't looked to see
> what the standard has here.)

We talked briefly about this a year ago:
http://wiki.postgresql.org/wiki/PgCon_2011_Developer_Meeting#Authorization_Issues
(Not quite the same thing, but it's closely related).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I'm not exactly sure who should be allowed to
> apply the "RUN AS other-user" option to a function, but I can see the
> possible value of separating the right to modify the function's
> definition from the user the function runs as.

When it comes to 'who can set it'- my first reaction is "the owner".
The next question is- what rights does the owner have to have on the
"other-user" role, and I would suggest "membership". This could be
extremely useful for non-C functions as well, consider this:

I'm Bob. I have an 'audit' role which is granted to me. I'd like to
create a function that runs as 'audit' (which has various rights granted
to it which are less than the rights of 'Bob'), but which only I can
modify. If I've been granted the 'audit' role, then I can create a
function which is owned by 'audit' (set role audit; create function
...), and I could make it security definer, therefore I should be able
to create a function which is owned by me and runs as 'audit'.

Writing this a bit off-the-cuff, so apologies if there are obvious flaws
in this logic. :)

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> (In other words, it's not that hard to build
> a "RUN AS other-user" feature into a C function, even without any support
> from the rest of the system.)

I was considering this and a bit concerned about what would happen if
the C function actually did this and if we'd clean things up properly at
the end or if the function would be required to handle that clean-up
(if it was written as SECUURITY INVOKER, which is what's being suggested
here)...

In general, I'd certainly rather have the database handle that cleanly
and consistently than expect my function to clean up after itself.

Alvaro's point about the discussion of a stack of roles is certainly
something else to consider, though I feel that the 'run-as' option is
pretty straight-forward and could be done more-or-less identically to
how we do secuirty definer now, it's just changing where we get the role
to change to before running the function.

Thanks,

Stephen

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I'm not entirely following here. If the function is coded in C,
> then it can pretty much do what it pleases independently of what
> user ID is thought to be executing it at the SQL level. That
> would really only matter if you were doing some SQL stuff via the
> SPI interface

Yeah, there is a lot of SPI through cached prepared statements.

> if that's the case, couldn't the C function set the appropriate
> role to use for itself, anyway?

I suppose it could, though I never really thought about that aspect
of the issue before.

> (In other words, it's not that hard to build a "RUN AS other-user"
> feature into a C function, even without any support from the rest
> of the system.)

Similar to what Stephen says in his post that came through while I
was typing this, I would be less comfortable with this being a
hand-rolled feature of individual C functions than having some more
systematic way to handle it. Even if there is the possibility that
someone could subvert the more systematic way of doing things with
clever C code, I think the systematic approach reduces risk from
error and would tend to make hostile code in C functions stand out
more. And having the information at the catalog level would make
the intended usages easier to manage.

-Kevin

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> (In other words, it's not that hard to build
>> a "RUN AS other-user" feature into a C function, even without any support
>> from the rest of the system.)

> I was considering this and a bit concerned about what would happen if
> the C function actually did this and if we'd clean things up properly at
> the end or if the function would be required to handle that clean-up
> (if it was written as SECUURITY INVOKER, which is what's being suggested
> here)...

It would have to remember to restore the previous role on normal exit,
but I believe that the system would take care of cleanup if an error is
thrown. Looking at fmgr_security_definer, there are just a couple of
lines involved with changing the active role. (There's a boatload of
*other* crap that's been shoved into that function over time, but the
part of it that actually does what it's named for is pretty darn small.)

> Alvaro's point about the discussion of a stack of roles is certainly
> something else to consider, though I feel that the 'run-as' option is
> pretty straight-forward and could be done more-or-less identically to
> how we do secuirty definer now, it's just changing where we get the role
> to change to before running the function.

Yes, it would surely not be much more code, just a bit added here:

if (procedureStruct->prosecdef)
fcache->userid = procedureStruct->proowner;

regards, tom lane

On Tue, Jun 12, 2012 at 03:13:26PM -0400, Tom Lane wrote:
> Even if I accepted that premise, this patch is a pretty bad
> implementation of it, because it restricts cases that there is no
> reason to think are unsafe.
>
> A less bizarre and considerably more future-proof restriction, IMO,
> would simply refuse any attempt to give ownership of a C function
> to a non-superuser.

That just moves the collateral damage to a different set of victims.

Hardcoding the list of vulnerable languages isn't so "future-proof". I'd
otherwise agree in principle if we were designing a system from scratch, but
it doesn't fit with the need to harmoniously protect existing systems. Adding
this restriction now would make some existing databases fail to restore from
dumps. That is grave enough at any juncture, let alone for a backpatched fix.

On Tue, Jun 12, 2012 at 04:14:48PM -0400, Tom Lane wrote:
> Basically, if we go down the road Noah is proposing, I foresee a steady
> stream of security bugs and ensuing random restrictions on what function
> owners can do. I do not like that future.

Then let's have every ALTER FUNCTION require the same language usage check as
CREATE FUNCTION. Or, if you insist, do so only for the hardcoded cases of
INTERNALlanguageId and ClanguageId, and document that no third-party PL may
rely on STRICT to the extent they do. This of course forbids more than
necessary but still strictly less than your proposal of blocking the original
ownership change.

nm

On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Rather than trying to enforce this in the
>> ALTER FUNCTION implementation, maybe we should just advise that if
>> you're going to grant ownership of a C function to a role which
>> might accidentally or maliciously allow it to be called with NULL
>> input, the C function should return NULL in that case.
>
> Yeah, the just-code-defensively option is worth considering too.

After rereading this thread, I think I agree with Kevin as well. In
order to have a problem, you have to (a) be superuser (i.e. be a
person who already has many ways of compromising the security and
integrity of the system), (b) decide to grant ownership of a C
function to a non-superuser, and (c) fail to verify that the function
is coded in a sufficiently defensive fashion to survive whatever that
user might do with it. So it seems plausible to simply define this
problem as administrator error rather than a failure of security.

Having said that, I do believe that answer is to some extent a
cop-out. It's practical to define things that way here because the
cost of checking for NULL inputs is pretty darn small. But suppose
ALTER FUNCTION had a facility to change the type of a function
argument. Would we then insist that any C function intended to be
used in this way must check that the type of each argument matches the
function's expectation? Fortunately, we don't have to decide that
right now because ALTER FUNCTION doesn't allow that and probably never
will. But it would certainly be awkward if we did someday allow that,
because now any C function that is intended to be used in this way has
to include this extra check or be labelled insecure. Short of
allowing the C code to advertise the function's expectations so that
CREATE/ALTER FUNCTION can cross-check them, I don't see a way out of
that problem.... because on the flip side, the C code could - not to
put too fine a point on it - be relying on just about anything. It
could assert that work_mem is less than 1GB (and the user could crash
it by increasing work_mem). It could crash on any day except Tuesday
(we always do payroll here on Tuesdays, so why would you call it on
any other day?). It could erase the entire database unless it's
marked immutable. We would surely blame any of those failure modes on
the person who wrote the C code, and if we make the opposite decision
here, then I think we put ourselves in the awkward position of trying
to adjudicate what is and is not reasonably for third parties to do in
their C code. I don't really want to go there, but I do think this
points to the need for extreme caution if we ever create any more
function properties that C coders might be tempted to rely on, or
allow any properties that C code may already be relying on to be
changed in new ways.

I'm going to mark this patch as rejected in the CF app, which is not
intended to foreclose debate on what to do about this, but just to
state that there seems to be no consensus to solve this problem in the
manner proposed.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Yeah, the just-code-defensively option is worth considering too.

> After rereading this thread, I think I agree with Kevin as well. ...
> Having said that, I do believe that answer is to some extent a
> cop-out.

I agree with that --- doing nothing at all doesn't seem like the best
option here.

> ... on the flip side, the C code could - not to
> put too fine a point on it - be relying on just about anything.

And with that too. The STRICT option is a fairly obvious security
hazard, but who's to say there are not others? I think it'd be easier
to make a case for forbidding a non-superuser from altering *any*
property of a C function. I'd rather start from the point of allowing
only what is clearly safe than disallowing only what is clearly unsafe.

Taking a step or two back, I think that the real use-case we should
be considering here is allowing non-superusers to own (or at least
install) extensions that contain C functions. We would probably want
the non-superuser to be able to drop the extension again, maybe
ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
not too darn much else. Fooling with any of the contained objects
doesn't seem like something we want to permit, since it's likely that
something like a datatype is going to have dependencies on not just
specific objects' properties but their interrelationships.

One possible approach to that is to say that the nominal owner of such
an extension only owns the extension itself, and ownership of the
contained objects is held by, say, the bootstrap superuser. There are
other ways too of course, but this way would bypass the problem of
figuring out how to restrict what an object's nominal owner can do
to it.

regards, tom lane

On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Yeah, the just-code-defensively option is worth considering too.
>
>> After rereading this thread, I think I agree with Kevin as well. ...
>> Having said that, I do believe that answer is to some extent a
>> cop-out.
>
> I agree with that --- doing nothing at all doesn't seem like the best
> option here.
>
>> ... on the flip side, the C code could - not to
>> put too fine a point on it - be relying on just about anything.
>
> And with that too. The STRICT option is a fairly obvious security
> hazard, but who's to say there are not others? I think it'd be easier
> to make a case for forbidding a non-superuser from altering *any*
> property of a C function. I'd rather start from the point of allowing
> only what is clearly safe than disallowing only what is clearly unsafe.

That seems like a fairly drastic overreaction. Are you going to ban
renaming it or changing the owner, which are in completely different
code paths? Yuck. Even if you only ban it for the main ALTER
FUNCTION code path, it seems pretty draconian, because it looks to me
like nearly everything else that's there is perfectly safe. I mean,
assuming the guy who wrote the C code didn't do anything completely
insane or malicious, setting GUCs or whatever should be perfectly OK.
Honestly, if you want to change something in the code, I'm not too
convinced that there's anything better than what Noah proposed
originally.

> Taking a step or two back, I think that the real use-case we should
> be considering here is allowing non-superusers to own (or at least
> install) extensions that contain C functions. We would probably want
> the non-superuser to be able to drop the extension again, maybe
> ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
> not too darn much else. Fooling with any of the contained objects
> doesn't seem like something we want to permit, since it's likely that
> something like a datatype is going to have dependencies on not just
> specific objects' properties but their interrelationships.

Moreover, it breaks dump-and-restore.

> One possible approach to that is to say that the nominal owner of such
> an extension only owns the extension itself, and ownership of the
> contained objects is held by, say, the bootstrap superuser. There are
> other ways too of course, but this way would bypass the problem of
> figuring out how to restrict what an object's nominal owner can do
> to it.

I don't particularly care for that solution; it seems like a kludge.
I've kind of wondered whether we ought to have checks in all the ALTER
routines that spit up if you try to ALTER an extension member from any
place other than an extension upgrade script... but that still
wouldn't prevent the extension owner from dropping the members out of
the extension and then modifying them afterwards. I'm not sure we
want to prevent that in general, but maybe there could be some
locked-down mode that has that effect.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I don't particularly care for that solution; it seems like a kludge.
> I've kind of wondered whether we ought to have checks in all the ALTER
> routines that spit up if you try to ALTER an extension member from any
> place other than an extension upgrade script... but that still
> wouldn't prevent the extension owner from dropping the members out of
> the extension and then modifying them afterwards. I'm not sure we
> want to prevent that in general, but maybe there could be some
> locked-down mode that has that effect.

Right, I wasn't too clear about that, but I meant that we'd have some
sort of locked-down state for an extension that would forbid fooling
with its contents. For development purposes, or for anybody that "knows
what they're doing", adding/subtracting/modifying member objects is
mighty handy. But a non-superuser who's loaded an extension that
contains C functions ought not have those privileges for it.

regards, tom lane

On Fri, Jul 20, 2012 at 3:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I don't particularly care for that solution; it seems like a kludge.
>> I've kind of wondered whether we ought to have checks in all the ALTER
>> routines that spit up if you try to ALTER an extension member from any
>> place other than an extension upgrade script... but that still
>> wouldn't prevent the extension owner from dropping the members out of
>> the extension and then modifying them afterwards. I'm not sure we
>> want to prevent that in general, but maybe there could be some
>> locked-down mode that has that effect.
>
> Right, I wasn't too clear about that, but I meant that we'd have some
> sort of locked-down state for an extension that would forbid fooling
> with its contents. For development purposes, or for anybody that "knows
> what they're doing", adding/subtracting/modifying member objects is
> mighty handy. But a non-superuser who's loaded an extension that
> contains C functions ought not have those privileges for it.

I could see having such a mode. I'm not sure that it would eliminate
people's desire to manually give away functions, though. In fact,
thinking about a couple of our customers, I'm pretty sure it wouldn't.
Now whether it's a good idea is another question, but...

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

On Fri, Jul 20, 2012 at 03:39:33PM -0400, Robert Haas wrote:
> On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > And with that too. The STRICT option is a fairly obvious security
> > hazard, but who's to say there are not others? I think it'd be easier
> > to make a case for forbidding a non-superuser from altering *any*
> > property of a C function. I'd rather start from the point of allowing
> > only what is clearly safe than disallowing only what is clearly unsafe.
>
> That seems like a fairly drastic overreaction. Are you going to ban
> renaming it or changing the owner, which are in completely different
> code paths? Yuck. Even if you only ban it for the main ALTER
> FUNCTION code path, it seems pretty draconian, because it looks to me
> like nearly everything else that's there is perfectly safe. I mean,
> assuming the guy who wrote the C code didn't do anything completely
> insane or malicious, setting GUCs or whatever should be perfectly OK.
> Honestly, if you want to change something in the code, I'm not too
> convinced that there's anything better than what Noah proposed
> originally.

How about a compromise of blocking GUC and STRICT changes while allowing
everything else? We add PGC_USERSET GUCs in most releases. As long as
non-superuser owners of trusted-language functions can change attached GUC
settings, review for each new GUC really ought to consider that scenario.
That will be easy to forget. I'm already wary about allowing changes to GUCs
like sql_inheritance and search_path. By contrast, the list of ALTER FUNCTION
alterations has grown slowly; the last addition before PostgreSQL 9.2 came in
PostgreSQL 8.3. Anyone implementing a new alteration will be modifying
AlterFunction() and have ample opportunity to notice from surrounding code the
need to identify a suitable permissions check. Also, like you say, the other
existing alterations are clearly safe.

Thanks,
nm