Re: Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Rather than trying to enforce this in the
>> ALTER FUNCTION implementation, maybe we should just advise that if
>> you're going to grant ownership of a C function to a role which
>> might accidentally or maliciously allow it to be called with NULL
>> input, the C function should return NULL in that case.
>
> Yeah, the just-code-defensively option is worth considering too.

After rereading this thread, I think I agree with Kevin as well. In
order to have a problem, you have to (a) be superuser (i.e. be a
person who already has many ways of compromising the security and
integrity of the system), (b) decide to grant ownership of a C
function to a non-superuser, and (c) fail to verify that the function
is coded in a sufficiently defensive fashion to survive whatever that
user might do with it. So it seems plausible to simply define this
problem as administrator error rather than a failure of security.

Having said that, I do believe that answer is to some extent a
cop-out. It's practical to define things that way here because the
cost of checking for NULL inputs is pretty darn small. But suppose
ALTER FUNCTION had a facility to change the type of a function
argument. Would we then insist that any C function intended to be
used in this way must check that the type of each argument matches the
function's expectation? Fortunately, we don't have to decide that
right now because ALTER FUNCTION doesn't allow that and probably never
will. But it would certainly be awkward if we did someday allow that,
because now any C function that is intended to be used in this way has
to include this extra check or be labelled insecure. Short of
allowing the C code to advertise the function's expectations so that
CREATE/ALTER FUNCTION can cross-check them, I don't see a way out of
that problem.... because on the flip side, the C code could - not to
put too fine a point on it - be relying on just about anything. It
could assert that work_mem is less than 1GB (and the user could crash
it by increasing work_mem). It could crash on any day except Tuesday
(we always do payroll here on Tuesdays, so why would you call it on
any other day?). It could erase the entire database unless it's
marked immutable. We would surely blame any of those failure modes on
the person who wrote the C code, and if we make the opposite decision
here, then I think we put ourselves in the awkward position of trying
to adjudicate what is and is not reasonably for third parties to do in
their C code. I don't really want to go there, but I do think this
points to the need for extreme caution if we ever create any more
function properties that C coders might be tempted to rely on, or
allow any properties that C code may already be relying on to be
changed in new ways.

I'm going to mark this patch as rejected in the CF app, which is not
intended to foreclose debate on what to do about this, but just to
state that there seems to be no consensus to solve this problem in the
manner proposed.

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