Re: Fixing insecure security definer functions

Regarding the advisory on possibly insecure security definer functions
that I just sent out (by overriding the search path you can make the
function do whatever you want with the privileges of the function
owner), the favored solution after some initial discussion in the core
team was to save the search path at creation time with each function.
This measure will arguably also increase the robustness of functions in
general, and it seems to be desirable as part of the effort to make
plan invalidation work.

Quite probably, there will be all sorts of consequences in terms of
backward compatibility and preserving the possibility of valid uses of
the old behavior and so on. So I'm inviting input on how to fix the
problem in general and how to avoid the mentioned follow-up problems in
particular.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> Regarding the advisory on possibly insecure security definer functions
> that I just sent out (by overriding the search path you can make the
> function do whatever you want with the privileges of the function
> owner), the favored solution after some initial discussion in the core
> team was to save the search path at creation time with each function.

Would this be done only on security-definer functions?

> This measure will arguably also increase the robustness of functions in
> general, and it seems to be desirable as part of the effort to make
> plan invalidation work.
>
> Quite probably, there will be all sorts of consequences in terms of
> backward compatibility and preserving the possibility of valid uses of
> the old behavior and so on. So I'm inviting input on how to fix the
> problem in general and how to avoid the mentioned follow-up problems in
> particular.

It'll break most of the functions that we have in our production
systems... They're not security definer functions but it's routine for
us to switch between different schemas to run a function on. I
certainly don't want to have to have seperate functions for these either
as it'd be completely duplicated code with just different search paths
set. I'm honestly not the least bit impressed with this solution to
the problem.

What about pushing all the in-function references down to the
specific objects referenced at plan creation time (err, I thought this
was done?)? In most cases what we're doing is building up queries and
then running them with execute so I *think* that'd still work for us.
Also, it seems like that's the way to deal with the plan-invocation
problem (since that's really going to have to go down to object level
anyway eventually, isn't it?) rather than trying to handle using the
search path to figure out if it's invalidated now or not based on what's
currently there.

Note that I'm not suggesting users would change their source code for
this but rather that the 'create function' command would implicitly do
this ala what create view does. I really could have sworn something
like this was done (where OIDs are saved).

Another option might be to modify the 'create function' syntax to have
an option of 'search path' to set what search path the function should
have at the start. Then, for security definer functions at least, issue
a WARNING if that isn't being set at CREATE FUNCTION time. I'm pretty
sure that in most cases (certainly for us) that'd be noticed and at
least investigated. Another option would be to ERROR if it's not
provided and allow the previous behaviour by allowing it to be set to
NULL (again, mainly on security definer functions.. maybe warning on
others or something).

Hope these thoughts help...

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
>> Regarding the advisory on possibly insecure security definer functions
>> that I just sent out (by overriding the search path you can make the
>> function do whatever you want with the privileges of the function
>> owner), the favored solution after some initial discussion in the core
>> team was to save the search path at creation time with each function.

> Would this be done only on security-definer functions?

I would like to see it done for all functions, security-definer or not.
There are efficiency reasons: if you keep the search path from thrashing
then you can cache plans more effectively. (Currently, plpgsql's plan
caching doesn't pay any attention to whether the search path has
changed, but it's impossible to argue that that's not broken.)

I would suggest that the search path be added as an explicit parameter
to CREATE FUNCTION, with a default of the current setting. The main
reason for this is that it's going to be a real PITA for pg_dump if we
don't allow an explicit specification.

It might also be worth allowing "PATH NULL" or some such locution to
specify the current behavior, for those who really want it. (In
particular, most C functions would want this to avoid useless overhead
for calls to things that aren't affected by search path.)

Bottom line here is that this feature is really orthogonal to SECURITY
DEFINER ...

regards, tom lane

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> It'll break most of the functions that we have in our production
> systems... They're not security definer functions but it's routine for
> us to switch between different schemas to run a function on.

> What about pushing all the in-function references down to the
> specific objects referenced at plan creation time (err, I thought this
> was done?)?

Wouldn't that break exactly the cases you're worried about? It would be
an enormous amount of work, too.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > It'll break most of the functions that we have in our production
> > systems... They're not security definer functions but it's routine for
> > us to switch between different schemas to run a function on.
>
> > What about pushing all the in-function references down to the
> > specific objects referenced at plan creation time (err, I thought this
> > was done?)?
>
> Wouldn't that break exactly the cases you're worried about? It would be
> an enormous amount of work, too.

No, because what we tend to do is build up a query in a string and
then call it using execute. It doesn't matter to the execute'd string
if the references in the functions are mapped to oids or not at creation
time (since the query being built in the string couldn't possibly be
affected). If the search path is forced to something that'll screw up
the query being execute'd tho.

The calls to build up the query don't use things in the current search
path much (they're generally refering to a seperate specific reference
schema). Once the command is built it's then run, but it could be run
in a number of different schemas (because they all have basically the
exact same set of tables) which is based on the search path. This
allows us to have one set of functions (I think we're up to around 80
now) which can work against a number of schemas.

Indeed, what I tend to do is set up the search path something like:

set search_path = user1_tables, user1_results, func_schema;
select do_scan();

set search_path = user2_tables, user2_results, func_schema;
select do_scan();

etc, etc. The queries are run against each user's tables and the
results put into a seperate schema for each user.

Thanks,

Stephen

Peter Eisentraut wrote:
> Regarding the advisory on possibly insecure security definer functions
> that I just sent out (by overriding the search path you can make the
> function do whatever you want with the privileges of the function
> owner), the favored solution after some initial discussion in the core
> team was to save the search path at creation time with each function.
> This measure will arguably also increase the robustness of functions in
> general, and it seems to be desirable as part of the effort to make
> plan invalidation work.
>
> Quite probably, there will be all sorts of consequences in terms of
> backward compatibility and preserving the possibility of valid uses of
> the old behavior and so on. So I'm inviting input on how to fix the
> problem in general and how to avoid the mentioned follow-up problems in
> particular.

Maybe we need an option on CREATE ... SECURITY DEFINER to allow the
function to inherit the caller's search path.

cheers

andrew

> Regarding the advisory on possibly insecure security definer functions

> that I just sent out (by overriding the search path you can make the
> function do whatever you want with the privileges of the function
> owner), the favored solution after some initial discussion in the core

> team was to save the search path at creation time with each function.

Have you considered hardcoding the schema for each object where it was
found at creation time ? This seems more intuitive to me. Also using a
search
path, leaves the possibility to inject an object into a previous schema.

Andreas

Am Mittwoch, 14. Februar 2007 10:21 schrieb Zeugswetter Andreas ADI SD:
> Have you considered hardcoding the schema for each object where it was
> found at creation time ?

Unless anyone has solved the halting problem lately, I don't think it's
possible to determine at creation time which objects will be accessed. At
least not for all languages.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Tue, 2007-02-13 at 20:01 -0500, Tom Lane wrote:
> I would suggest that the search path be added as an explicit parameter
> to CREATE FUNCTION, with a default of the current setting. The main
> reason for this is that it's going to be a real PITA for pg_dump if we
> don't allow an explicit specification.
>
> It might also be worth allowing "PATH NULL" or some such locution to
> specify the current behavior, for those who really want it. (In
> particular, most C functions would want this to avoid useless overhead
> for calls to things that aren't affected by search path.)
>

It might also be useful to allow something such as PATH CURRENT to
attach the current schema as the search path for all calls of that
function.

This would be useful because then SQL scripts for installing 3rd party
modules could install nicely into any schema by merely setting
search_path before running the script.

For instance, PostGIS doesn't support installing into a schema other
than "public" because they want to have a static SQL install script
rather than generate one based on your desired search path.

Regards,
Jeff Davis

Andreas,

> Have you considered hardcoding the schema for each object where it was
> found at creation time ? This seems more intuitive to me.

This isn't practical. Consider the schema qualification syntax for
operators.

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

On 2/13/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I would suggest that the search path be added as an explicit parameter
> to CREATE FUNCTION, with a default of the current setting. The main
> reason for this is that it's going to be a real PITA for pg_dump if we
> don't allow an explicit specification.

yikes!

If you guys go through with forcing functions to attach to objects
when they are created, it will break almost every project I've ever
worked on :(. The schema/function combo fits into all kinds of de
facto partitioning strategies and organization methods.

I can understand invalidating plans when the search_path changes, though.

merlin

"Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> yikes!

> If you guys go through with forcing functions to attach to objects
> when they are created, it will break almost every project I've ever
> worked on :(. The schema/function combo fits into all kinds of de
> facto partitioning strategies and organization methods.

If you read a bit further, I did suggest providing an option to retain
the current behavior. I don't think it should be the default though.

regards, tom lane

On 2/15/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Merlin Moncure" <mmoncure(at)gmail(dot)com> writes:
> > yikes!
>
> > If you guys go through with forcing functions to attach to objects
> > when they are created, it will break almost every project I've ever
> > worked on :(. The schema/function combo fits into all kinds of de
> > facto partitioning strategies and organization methods.
>
> If you read a bit further, I did suggest providing an option to retain
> the current behavior. I don't think it should be the default though.

Yeah, I saw that, but the issue is really deeper, functions that
create functions, etc. changing the default behavior affects how
functions work in a really fundamental way...all pl/pgsql code that
can float over schemas would have to be checked. In the worst case,
this could mean converting large libraries to dynamic sql or creating
thousands of additional functions...ugh.

Maybe there could be a GUC setting(function default function schema
path=current path/path null)? It would seem only appropriate to have
security definer raise a warning/error for path null though. Then we
could debate about how that should be set by default but nobody really
loses that argument.

merlin

As was pointed out awhile ago
http://archives.postgresql.org/pgsql-general/2007-02/msg00673.php
it's insecure to run a SECURITY DEFINER function with a search_path
setting that's under the control of someone who wishes to subvert
the function. Even for non-security-definer functions, it seems
useful to be able to select the search path for the function to use;
we've had requests for that before. Right now, this is possible but
tedious and slow, because you really have to use a subtransaction
to ensure that the path is reset correctly on exit:

BEGIN
SET LOCAL search_path = ...;
... useful work here ...
EXCEPTION
END

(In fact it's worse than that, since you can't write an EXCEPTION
without at least one WHEN clause, which is maybe something to change?)
Also, this approach isn't available in plain SQL functions.

I would like to fix this for 8.3. I don't have a patch yet but want
to get buy-in on a design before feature freeze. I propose the
following, fully-backward-compatible design:

1. Add a text column "propath" to pg_proc. It can be either NULL or
a search path specification (interpreted the same as values for the
search_path GUC variable). NULL means use the caller's setting, ie,
current behavior.

2. When fmgr.c sees either prosecdef or propath set for a function to be
called, it will insert the fmgr_security_definer hook into the call.
fmgr_security_definer will be responsible for establishing the correct
current-user and/or path settings and restoring them on exit. (We could
use two independent hooks, but since these features will often be used
together, implementing both with just one hook seems reasonable.)

3. Add optional clauses to CREATE FUNCTION and ALTER FUNCTION to specify
the propath value. I suggest, but am not wedded to,
PATH 'foo, bar'
PATH NONE
Since PATH NONE is the default, it's not really needed in CREATE
FUNCTION, but it seems useful to allow it for ALTER FUNCTION.

Comments?

regards, tom lane

On 3/29/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> As was pointed out awhile ago
> http://archives.postgresql.org/pgsql-general/2007-02/msg00673.php
> it's insecure to run a SECURITY DEFINER function with a search_path
> setting that's under the control of someone who wishes to subvert
> the function. Even for non-security-definer functions, it seems
> useful to be able to select the search path for the function to use;
> we've had requests for that before. Right now, this is possible but
> tedious and slow, because you really have to use a subtransaction
> to ensure that the path is reset correctly on exit:
>
> BEGIN
> SET LOCAL search_path = ...;
> ... useful work here ...
> EXCEPTION
> END
>
> (In fact it's worse than that, since you can't write an EXCEPTION
> without at least one WHEN clause, which is maybe something to change?)
> Also, this approach isn't available in plain SQL functions.
>
> I would like to fix this for 8.3. I don't have a patch yet but want
> to get buy-in on a design before feature freeze. I propose the
> following, fully-backward-compatible design:
>
> 1. Add a text column "propath" to pg_proc. It can be either NULL or
> a search path specification (interpreted the same as values for the
> search_path GUC variable). NULL means use the caller's setting, ie,
> current behavior.
>
> 2. When fmgr.c sees either prosecdef or propath set for a function to be
> called, it will insert the fmgr_security_definer hook into the call.
> fmgr_security_definer will be responsible for establishing the correct
> current-user and/or path settings and restoring them on exit. (We could
> use two independent hooks, but since these features will often be used
> together, implementing both with just one hook seems reasonable.)
>
> 3. Add optional clauses to CREATE FUNCTION and ALTER FUNCTION to specify
> the propath value. I suggest, but am not wedded to,
> PATH 'foo, bar'
> PATH NONE
> Since PATH NONE is the default, it's not really needed in CREATE
> FUNCTION, but it seems useful to allow it for ALTER FUNCTION.

fwiw, I think this is a great solution...because the default behavior
is preserved you get through without any extra guc settings (although
you may want to add one anyways).

maybe security definer functions should raise a warning for implicit
PATH NONE, and possibly even deprecate that behavior and force people
to type it out in future (8.4+) releases.

merlin

* Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
> fwiw, I think this is a great solution...because the default behavior
> is preserved you get through without any extra guc settings (although
> you may want to add one anyways).

I agree that the proposed solution looks good.

> maybe security definer functions should raise a warning for implicit
> PATH NONE, and possibly even deprecate that behavior and force people
> to type it out in future (8.4+) releases.

While I agree that raising a warning makes sense I don't believe it
should be forced. There may be cases where, even in security definer
functions, the current search_path should be used (though, of course,
care must be taken in writing such functions).

Thanks,

Stephen

On 3/29/07, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
> > fwiw, I think this is a great solution...because the default behavior
> > is preserved you get through without any extra guc settings (although
> > you may want to add one anyways).
>
> I agree that the proposed solution looks good.
>
> > maybe security definer functions should raise a warning for implicit
> > PATH NONE, and possibly even deprecate that behavior and force people
> > to type it out in future (8.4+) releases.
>
> While I agree that raising a warning makes sense I don't believe it
> should be forced. There may be cases where, even in security definer
> functions, the current search_path should be used (though, of course,
> care must be taken in writing such functions).

I agree...I'm just suggesting to make you explicitly write 'PATH NONE'
for security definer functions because of the security risk...just a
thought though.

merlin

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
>> maybe security definer functions should raise a warning for implicit
>> PATH NONE, and possibly even deprecate that behavior and force people
>> to type it out in future (8.4+) releases.

> While I agree that raising a warning makes sense I don't believe it
> should be forced.

A WARNING seems reasonable to me too. I'd just do it on the combination
of SECURITY DEFINER with PATH NONE, regardless of how you typed it
exactly. ALTERing a function into that configuration should draw the
same warning.

regards, tom lane

Stephen Frost wrote:
> While I agree that raising a warning makes sense I don't believe it
> should be forced.  There may be cases where, even in security definer
> functions, the current search_path should be used (though, of course,
> care must be taken in writing such functions).

I really wonder whether such a use case exists. What would it be?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I would like to fix this for 8.3. I don't have a patch yet but want
> to get buy-in on a design before feature freeze. I propose the
> following, fully-backward-compatible design:
[...]
> Comments?

In talking w/ Jeff Davis on IRC today, another thought occurred to us at
about the same time when thinking about this PATH option to CREATE
FUNCTION in relation to packages (in this specific case, PostGIS).

It would be useful to have a function which could be passed a relative
(to the caller's search path) object name and would return the fully
qualified name of that object. In this way, functions could be written
which take relative arguments from the user but *only* those explicitly
checked for.

ie:

CREATE FUNCTION myfunc(text) RETURNS NULL AS
$_$
declate
relative_name alias for $1;
full_name text;
myval int;

begin;
full_name := pg_getfullpath(relative_name);

myval := select val from full_name;

end;
$_$
WITH PATH 'pkg_schema,pg_catalog'
LANG 'plpgsql';

Personally, I really like this idea and it'd handle the specific
use-cases I was expecting to have to use 'PATH NONE' for, but in a much
better, cleaner and clearer way.

On the flip side, I'm not sure how easy that's going to be to get given
the implementation you were describing...

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> It would be useful to have a function which could be passed a relative
> (to the caller's search path) object name and would return the fully
> qualified name of that object. In this way, functions could be written
> which take relative arguments from the user but *only* those explicitly
> checked for.

Your example doesn't seem to be doing anything interesting ... am I
misunderstanding, or did you omit the actual checking? Also, if the
search path is controlled by the function, what good is this ---
wouldn't it always result in a trusted schema?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > It would be useful to have a function which could be passed a relative
> > (to the caller's search path) object name and would return the fully
> > qualified name of that object. In this way, functions could be written
> > which take relative arguments from the user but *only* those explicitly
> > checked for.
>
> Your example doesn't seem to be doing anything interesting ... am I
> misunderstanding, or did you omit the actual checking? Also, if the
> search path is controlled by the function, what good is this ---
> wouldn't it always result in a trusted schema?

The idea was that 'pg_getfullpath()' would return the full path to an
object relative to the caller's path while in a function which has an
explicit search_path defined. Probably more sensible is an example of
what I'm thinking the function would do:

postgres=# create schema myschema;
CREATE SCHEMA
postgres=# set search_path=myschema;
SET
postgres=# create table abc (a int);
CREATE TABLE
postgres=# select pg_getfullpath('abc');
fullpath
----------
myschema.abc

The 'special' bit here is that pg_getfullpath() would work relative to
the caller's search_path even inside of a function which has its 'PATH'
set. That's really the only thing about it that makes it very
interesting.

Of course, thinking a bit farther along, this would only be useful when
building up an SQL statement as a string to then execute, but you have
to do that if you're getting a table passed in as an argument anyway
(and is exactly what I'm doing).

Hope that helps.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> The 'special' bit here is that pg_getfullpath() would work relative to
> the caller's search_path even inside of a function which has its 'PATH'
> set.

Eeek. *Which* caller's search_path? The string you're handed might've
come from multiple levels up.

There might be some point in allowing the caller itself to fully qualify
the name (before passing it down) with more ease than now. We have
regclass and so forth, but those make a point of stripping schema
qualification when it's "unnecessary" according to the current search
path. And yet on the third hand --- how often would it be the case that
this was an issue and yet the caller doesn't know which schema it has in
mind?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > The 'special' bit here is that pg_getfullpath() would work relative to
> > the caller's search_path even inside of a function which has its 'PATH'
> > set.
>
> Eeek. *Which* caller's search_path? The string you're handed might've
> come from multiple levels up.

I would say the outer-most. If people inbetween want to mess with
things, let them qualify it before handing it down. Clearly, an
already-qualified object would be left alone.

> There might be some point in allowing the caller itself to fully qualify
> the name (before passing it down) with more ease than now. We have
> regclass and so forth, but those make a point of stripping schema
> qualification when it's "unnecessary" according to the current search
> path. And yet on the third hand --- how often would it be the case that
> this was an issue and yet the caller doesn't know which schema it has in
> mind?

At least at the moment in our application code the search_path is set
quite far apart from the function call. Additionally, we depend on the
fact that we can set a multi-schema search_path with a specific order
and have the correct thing happen. A function which qualified an object
based on the current search_path would probably work for us in this
application but seems quite counter-intuitive to a user who is calling
functions by hand (for whatever reason).

ie:

select error_scan(pg_getfullname('default_error_list'));

vs.

select error_scan('default_error_list');

As a user, it's pretty ingrained that unqualified table names follow the
current search_path and having to explicitly qualify tables when passing
them to functions (with a helper function or not) just comes across as
broken.

Thanks,

Stephen

Stephen, Tom,

> > Eeek. *Which* caller's search_path? The string you're handed
> > might've come from multiple levels up.
>
> I would say the outer-most. If people inbetween want to mess with
> things, let them qualify it before handing it down. Clearly, an
> already-qualified object would be left alone.

Based on further IRC, I can personally see a solution which would be
generally useful. Further, this solution doesn't require (or shouldn't)
any modification of the existing function_path solution. It just requires
two functions, which could be developed now or later:

1) pg_get_caller_path() == returns the search_path of the calling session,
which presumably is being stored somewhere for reversion when the final
nested function exits.

2) pg_get_object_fullname(name, path) == returns the fully-qualified object
name of an object based on the path supplied.

In addition to Stephen's peculiar application, I can see these functions
being useful for a variety of applications which might mix path-set
functions with pathless functions, or which use hundreds of schema to
isolate user objects.

However, I don't see this as being anything which would hold up 8.3, since
function_path doesn't break anything which was already working.

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Tom, Josh,

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> Based on further IRC, I can personally see a solution which would be
> generally useful. Further, this solution doesn't require (or shouldn't)
> any modification of the existing function_path solution. It just requires
> two functions, which could be developed now or later:

I agree.

> 1) pg_get_caller_path() == returns the search_path of the calling session,
> which presumably is being stored somewhere for reversion when the final
> nested function exits.

I'd be happy to take a look at this but it depends on knowing where in
the backend the caller's search_path is stashed (which it has to be to
be reset at the end, I'd expect...), which depends on the implementation
of function_path. I don't recall seeing that committed to CVS yet (though
perhaps I missed it).

> 2) pg_get_object_fullname(name, path) == returns the fully-qualified object
> name of an object based on the path supplied.

I think this particular function would be useful in a number of cases,
honestly. :)

> However, I don't see this as being anything which would hold up 8.3, since
> function_path doesn't break anything which was already working.

Agreed, never intended it to imply there was something wrong with
function_path, just a nice addition to better support certain
applications (mine, at the least :).

Thanks!

Stephen

> 3. Add optional clauses to CREATE FUNCTION and ALTER FUNCTION to specify
> the propath value. I suggest, but am not wedded to,
> PATH 'foo, bar'
> PATH NONE
> Since PATH NONE is the default, it's not really needed in CREATE
> FUNCTION, but it seems useful to allow it for ALTER FUNCTION.

I think NONE may be a bit misleading, as if path will be empty.
CURRENT sounds better for this.

Add
PATH SAVED
as shorthand to
PATH current_setting('search_path')
as well.

Default should be SAVED for SECURITY DEFINER functions.
A parameter to set the default for SECURITY INVOKER functions would be
nice too.