Обсуждение: Permissions checks for range-type support functions

It strikes me that we are rather lacking in $SUBJECT.  There are a
couple of distinct issues:

1. Since no permissions checks are applied at runtime, a nefarious
person could bypass ACL_EXECUTE checks for any function that happens
to match the signature for a subtype_diff function.  Just create a
range type that specifies that function as subtype_diff, and think
up an index operation that will call it with the arguments you want.
In principle one could bypass permissions checks on canonical functions
as well, though in practice I think that's uninteresting because of the
restrictions on the function signature.

In the analogous situation for base types, we don't worry about this
because only superusers can define base types; we just presume (and
document) that the superuser is effectively granting public access
permissions on all functions referenced in a base type definition.
This will clearly not fly for range types.

The nearest analogy I can see for non-superusers is use of functions
within aggregate definitions.  There, we check that the aggregate
definer has permission to call the referenced function at aggregate
definition time, and then recheck it at the start of any query that
uses the aggregate.  (So, the aggregate definer can give away permission
to call the target function, but he could do that anyway by creating
a SECURITY DEFINER wrapper function.)

For the range-type support functions, it would be simple enough to check
call permission at range-type definition time.  But I really don't want
to put in a run-time check, because there doesn't seem to be a very
practical way to do it less often than every call, and besides that it's
not very clear who to blame an index operation on.  Is it good enough to
test this only at range-type creation time?  The implication of course is
that you might not be able to revoke execute permission from a bad guy,
short of dropping your function.  This might be all right given the
relatively narrow cross-section of functions that could be called this
way.

2. The ANALYZE option is flat out dangerous, because it allows any
function with the signature "f(internal) returns bool" to be called as
though it's a typanalyze function.  There are a couple of such functions
in the catalogs already, and either of them will probably crash the
backend if invoked as typanalyze on a range column.

Again, this isn't a hazard for the existing use with base types,
because only superusers (presumed to be responsible adults) can
define base types.  But for range types it's an easy DOS attack for
less responsible persons.

I'm inclined to think that the right solution for this one is to drop
the ANALYZE option altogether, and just have DefineRange automatically
install a system-wide typanalyze function for ranges.  Under what
circumstances is range-type-specific knowledge going to be needed for
typanalyze, anyway?  Especially since the functions that would use the
results will be system-wide selectivity functions associated with the
ANYRANGE operators.
        regards, tom lane

I wrote:
> For the range-type support functions, it would be simple enough to check
> call permission at range-type definition time.  But I really don't want
> to put in a run-time check, because there doesn't seem to be a very
> practical way to do it less often than every call, and besides that it's
> not very clear who to blame an index operation on.  Is it good enough to
> test this only at range-type creation time?  The implication of course is
> that you might not be able to revoke execute permission from a bad guy,
> short of dropping your function.  This might be all right given the
> relatively narrow cross-section of functions that could be called this
> way.

On further reflection I think we can get away with this, because of the
additional check that's already in there that insists the support
functions be IMMUTABLE.  That means they can't have any side-effects,
which makes the potential security consequences of unauthorized calls
very minimal.  It's conceivable that the return value of a misused
subtype_diff function could be interesting (think some sort of
decryption function); but the system doesn't offer any way for a user
to see that return value.  It will only factor into GiST index page
split decisions, and in a pretty indirect way at that.  So I don't see
any interesting security risk for a misused subtype_diff function;
and there's not likely to be any meaningful hole for abusing canonical
functions either, due to the restrictions on argument/result datatype.

I still think we ought to add a creation-time permission check, just
for pro-forma purposes.

> 2. The ANALYZE option is flat out dangerous,

But this is still true.
        regards, tom lane

On Tue, Nov 22, 2011 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 2. The ANALYZE option is flat out dangerous, because it allows any
> function with the signature "f(internal) returns bool" to be called as
> though it's a typanalyze function.  There are a couple of such functions
> in the catalogs already, and either of them will probably crash the
> backend if invoked as typanalyze on a range column.

It's always seemed mildly insane to me that we don't distinguish
between different flavors of "internal".  That seems like an accident
waiting to happen.

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

Excerpts from Robert Haas's message of mié nov 23 11:01:50 -0300 2011:
> On Tue, Nov 22, 2011 at 6:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 2. The ANALYZE option is flat out dangerous, because it allows any
> > function with the signature "f(internal) returns bool" to be called as
> > though it's a typanalyze function.  There are a couple of such functions
> > in the catalogs already, and either of them will probably crash the
> > backend if invoked as typanalyze on a range column.
>
> It's always seemed mildly insane to me that we don't distinguish
> between different flavors of "internal".  That seems like an accident
> waiting to happen.

Well, before we had INTERNAL, there was only OPAQUE which conflated even
more things that we now distinguish (at least trigger and cstring, not
sure if there were others).

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

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Robert Haas's message of mié nov 23 11:01:50 -0300 2011:
>> It's always seemed mildly insane to me that we don't distinguish
>> between different flavors of "internal".  That seems like an accident
>> waiting to happen.

> Well, before we had INTERNAL, there was only OPAQUE which conflated even
> more things that we now distinguish (at least trigger and cstring, not
> sure if there were others).

Yeah, we previously subdivided OPAQUE to get rid of exactly this type of
problem.  Not sure if it's worth going further.

The case that would be problematic would be if we had two different
calling contexts that invoked internal-using functions, both accessible
for untrusted users to set up which function gets called, and sharing
identical function signatures.  ATM I believe the only calling contexts
that untrusted users can control are operator selectivity functions
(restriction and join flavors) and encoding conversion functions;
and those three cases have signatures that are distinct from each other
and from all privileged cases.  But this certainly is something that
could accidentally get broken in future.

I don't think we want to split INTERNAL into the number of distinct
pseudotypes that would be required to cover everything, but it might
be worth inventing a couple more for the selectivity cases, if we ever
change those APIs again.
        regards, tom lane