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

Robert Haas <robertmhaas@gmail.com> writes:
> I'm not clear that I understand why it would be necessary for
> row-level security to touch every part of the code.

OK, I admit it's not "every" part, just every place that fetches,
inserts, updates, or deletes tuples.  There are quite a few ;-)

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.

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.

> 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.

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.
        regards, tom lane