Thread: Misusing functions taking "internal" via coincidental signature match

We have got a whole bunch of functions in the system that accept
arguments of type "internal", where the actual meaning of "internal"
varies wildly (it's generally some non-SQL-visible data structure).
While (I believe that) SQL users cannot call any such functions
directly, they could still cause all sorts of trouble if they could
persuade the system to call one with some other kind of "internal"
argument than the function is expecting.

Neither I nor the rest of the pgsql-security list could find any places
where there is a hole of this type today.  However there are several
commands accessible to non-superusers that skirt the edge of danger:

CREATE OPERATOR will accept as "restriction selectivity" function
any function having arguments (internal, oid, internal, int4),
and it'll accept as "join selectivity" function
any function having arguments (internal, oid, internal, int2).
It doesn't bother to check the return type, nor even whether
you have permission to call the function :-(

CREATE CONVERSION will accept as conversion function any function
having arguments (int4, int4, cstring, internal, int4).  It doesn't
check the return type, either, though at least it checks for EXECUTE
permission.  (Not that that helps much given that the default situation
is public execute permission.)

CREATE TYPE accepts receive functions having arguments either (internal)
or (internal,oid,int4).  Fortunately it insists that the result type be
the type being created, so it doesn't seem that any confusion is
possible.  However, it will also accept as "custom analyze" function
anything taking (internal) and returning bool.  We would have a problem
with boolrecv there, except that CREATE TYPE also demands ownership
of the function, and a non-superuser won't own boolrecv ... or,
probably, anything else accepting internal.

Now, so far as I can find there are not any functions in core or contrib
that accidentally match the signature requirements for selectivity
estimators or conversion functions.  It's a bit nervous-making though,
particularly since the signatures for GIST and GIN support functions
aren't totally nailed down (nor, indeed, verified by the system anyway).
But the big point here is not so much whether we have a problem today
as what might happen in future.  All of these system support functions
have call signatures that are subject to change.  (In fact, I'm about
to commit changes to allow an additional internal parameter for join
selectivity estimators.)  So it's not hard at all to think that as
we independently change different parts of the system, we might get
an accidental match of the expected signatures for functions intended to
do very different things.  And then we have a security issue if there
are non-privileged commands to tell the system to call one of these
functions in a particular way.  So it seems like we ought to take some
steps to compartmentalize things a bit better.

The cleanest solution I can think of is to invent some more pseudotypes
that act just like INTERNAL, and then to require non-privileged CREATE
commands to reference functions that take one of these types instead
of bare INTERNAL.  There is a backwards compatibility problem here,
of course, but it wouldn't affect anybody who hadn't written a custom
selectivity estimator, conversion, or analyze function.  Which is
probably only the PostGIS project.

Failing that, we could just try to keep a registry of possible
signatures for internal-accepting functions, and make sure we don't
accept any patches that cause conflicts.  This would avoid creating
backwards compatibility problems, but without any automatic enforcement
it seems pretty dangerous.  (Perhaps a new regression test in the spirit
of opr_sanity could help, though.)

Another thing that seems like a real good idea is to tighten up
the above-mentioned commands to check for a specific return type and
demand execute permissions.  Including the return type in the required
signature is in itself a big improvement in reducing the risk of
accidental matches.  This part we could do without creating any
compatibility issues.  I also thought about demanding ownership
rather than just execute permission on the functions.  That doesn't
seem like it'd fly for selectivity estimators, since it's customary
for user datatypes to re-use the built-in ones, but it might be a
real good idea for conversions.

Comments?
        regards, tom lane

On Aug 15, 2008, at 15:12, Tom Lane wrote:

> The cleanest solution I can think of is to invent some more  
> pseudotypes
> that act just like INTERNAL, and then to require non-privileged CREATE
> commands to reference functions that take one of these types instead
> of bare INTERNAL.  There is a backwards compatibility problem here,
> of course, but it wouldn't affect anybody who hadn't written a custom
> selectivity estimator, conversion, or analyze function.  Which is
> probably only the PostGIS project.

+1 # cleanliness++

> Failing that, we could just try to keep a registry of possible
> signatures for internal-accepting functions, and make sure we don't
> accept any patches that cause conflicts.  This would avoid creating
> backwards compatibility problems, but without any automatic  
> enforcement
> it seems pretty dangerous.  (Perhaps a new regression test in the  
> spirit
> of opr_sanity could help, though.)

That won't help pgFoundry modules, though. And since there's been so  
much talk about having improved third-party module support, it seems  
to me that we ought to try to discourage security holes in such  
modules, too.

> Another thing that seems like a real good idea is to tighten up
> the above-mentioned commands to check for a specific return type and
> demand execute permissions.  Including the return type in the required
> signature is in itself a big improvement in reducing the risk of
> accidental matches.  This part we could do without creating any
> compatibility issues.  I also thought about demanding ownership
> rather than just execute permission on the functions.  That doesn't
> seem like it'd fly for selectivity estimators, since it's customary
> for user datatypes to re-use the built-in ones, but it might be a
> real good idea for conversions.

I did find this strange:

CREATE OR REPLACE FUNCTION citextrecv(internal)
RETURNS citext
AS 'textrecv'
LANGUAGE 'internal' STABLE STRICT;

CREATE OR REPLACE FUNCTION citextsend(citext)
RETURNS bytea
AS 'textsend'
LANGUAGE 'internal' STABLE STRICT;

It'd make a lot more sense to me if bytea was passed to citextrcv,  
rather than internal, even if, internally, it used internal (so to  
speak). Now you didn't mention CREATE FUNCTION as being one of the  
places where this comes up, but it seems to me that it would be  
valuable to hide the internal struct, and force SQL users to use only  
well-defined types.

Just my $0.02, FWIW.

Best,

David