Re: Fixing insecure security definer functions

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