Fixing insecure security definer functions

* Peter Eisentraut (peter_e@gmx.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@snowman.net> writes:

* Peter Eisentraut (peter_e@gmx.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@snowman.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@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.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@sss.pgh.pa.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@gmail.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@sss.pgh.pa.us> wrote:

"Merlin Moncure" <mmoncure@gmail.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@sss.pgh.pa.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@gmail.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@snowman.net> wrote:

* Merlin Moncure (mmoncure@gmail.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@snowman.net> writes:

* Merlin Moncure (mmoncure@gmail.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@sss.pgh.pa.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