Re: How to get SE-PostgreSQL acceptable - Mailing list pgsql-hackers

Robert Haas wrote:
> On Wed, Jan 28, 2009 at 6:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As an example, the present patch imagines that it will have adequate
>> control over inserts by putting hooks into simple_heap_insert and the
>> (rather small number of) places that call heap_insert directly.  But
>> there are dozens of calls of simple_heap_insert and no way for the
>> function itself to know why it is being called or on whose behalf.
>> The patch's hook function tries to work around the fact that it hasn't
>> got that information by means of heuristics.  Aside from the question of
>> whether there are bugs in those heuristics today (I'm certain there
>> are), every time we accept a patch that adds another call of
>> simple_heap_insert, we're going to have to revisit the hook to see
>> if it needs to be twiddled.
> 
> I agree that that's no good.

My concern is that superuser is allowed to modify system catalog
by hand, like:
  UPDATE pg_proc SET probin = '/tmp/malicious_library.so'     WHERE oid = ...;

It is logically same as ALTER FUNCTION.

Even if I remove a hook from simple_heap_xxxx(), it is necessary
to check queries from clients.


>> Another problem is that since the hook only knows the parameters to
>> simple_heap_insert plus global state (such as current_user), it can't
>> cope very well with security-related context changes.  We have already
>> heard that situations involving views are simply broken in the patch as
>> it stands --- row-level permissions are checked against current_user
>> and not the view owner, and there's no good way to fix that.
> 
> I thought that was intentional, and I sort of think that it's the
> right decision.
From the viewpoint of SELinux, it is not a matter because core
PostgreSQL does not change client's security context.
But, it is indeed controversial for Row-level ACLs.
(We have a time about this issue.)

>>> It seems to me that the crucial decision is "Where are we
>>> trying to erect the security wall?".  If we're trying to put it
>>> between the client and the postgres backend, then maybe the right
>>> thing to do is apply some sort of processing step to each query that
>>> is submitted, essentially rewriting "SELECT * FROM a, b" as "SELECT *
>>> FROM a, b WHERE you_can_see(a.securityid) AND
>>> you_can_see(b.securityid)".  Maybe you (Tom) still won't like this
>>> because it breaks SQL semantics, but it seems like it would at least
>>> centralize the code.
>> Well, it wouldn't break SQL semantics from the point of view of the
>> planner, so that's one demerit taken off the books.  However, I seem
>> to recall that exactly that approach was taken in a older version of
>> the patch (many moons ago) and we found fatal problems in it too.
> 
> Can you (or someone) provide a pointer to the archives?  I can't
> immediately see any reason why that problem wouldn't be fixable.

IIRC, 0racle or M$ has a patent to rewrite WHERE clause for security
purpose, so Tom suggested it should be implemented using a hook
deployed within executor.
At least, it also enables code more simple.

>> The "where's the wall" point is a good one.  I think part of the issue
>> is that the patch is to some extent trying to erect a security wall
>> that's between the data and large parts of the backend C code.  Which is
>> an interesting idea and maybe could have been made to work if it'd been
>> designed in from the beginning, but to retrofit it today seems pretty
>> impractical.
> 
> Agreed.  With all respect to Kaigai-san, I am suspicious that some of
> this may be unintentional.  It may be that cleaning this up and
> putting the wall in one well-defined spot will allay a number of your
> concerns.

A wall between the data and _backend C code_ is not my intention.

Its purpose is a wall between the data and clients including superuser
via SQL queries. It is a basic assumption we cannot acquire any accesses
from system internal entities, as SELinux do nothing for kernel loadable
modules.

However, please note that making a decision at more hot point is more
good design, because it enables to reduce potential bypasses.
At the begining, I choosed simple_heap_xxxx() as a most hot point.
However, it can be replacable, if we can find any other place without
omission and consistent.

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>