Not quite a security hole: CREATE LANGUAGE for non-superusers

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@leadboat.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@leadboat.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@leadboat.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@leadboat.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@gmail.com> writes:

On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch <noah@leadboat.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@sss.pgh.pa.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.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.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@sss.pgh.pa.us) wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.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@snowman.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@snowman.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.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.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@snowman.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@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Tom,

* Tom Lane (tgl@sss.pgh.pa.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@sss.pgh.pa.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@sss.pgh.pa.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@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.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