Re: [PATCH] SE-PgSQL/tiny rev.2193

2009/7/16 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Updated SE-PgSQL patch is here:
>
>  http://sepgsql.googlecode.com/files/sepgsql-01-tiny-8.5devel-r2196.patch.gz
>
> Unused definitions of SELinux's permissions are ripped out from
> the permission table.

OK, I'm looking at this version of the patch, and my first reaction is
that it appears to be completely useless. Standard PostgreSQL has
grantable privileges on 10 classes of objects: TABLE, COLUMN,
SEQUENCE, DATABASE, FOREIGN DATA WRAPPER, FOREIGN SERVER, FUNCTION,
LANGUAGE, SCHEMA, and TABLESPACE. This patch, on the other hand,
implements SE-pgsql privileges for 3 classes of objects: DATABASE,
NAMESPACE, and FUNCTION. That doesn't seem to jive in any conceivable
way with the advice that has been given repeatedly and in every single
review of this patch, which is to make the scope of SE-pgsql line up
exactly with the standard permissions that PostgreSQL already
implements. In earlier versions, there were checks on all sorts of
extra things (like rows, and various DDL operations) that invented
whole new classes of permission checks. Now you've gone to the
opposite extreme of checking almost nothing.

The goal here is not to pare this patch down to nothing: it's to
implement a coherent feature set that matches what PostgreSQL already
does. Here is what I wrote 4 days ago:

"Another problem that I have with this patch set is that it STILL
doesn't have parity with the DAC permissions scheme (despite previous
requests to make it have parity)"

Here is what I wrote 3 days ago:

"Yes: to repeat what has been said multiple times previously, you
should postpone everything that isn't a mirror of the current security
model: there should only be permission checks in places where there
are permissions checks now, and they should be mirror images of the
current DAC checks."

Here is what I wrote yesterday:

"So the point we keep repeating here is that SEPostgreSQL should be
doing the same kinds of permissions checks as regular PostgreSQL using
the same names, code paths, etc. I don't know how to say it any more
clearly than that."

And just for reference, here is what Peter wrote 5 months ago, which
is basically saying the same thing:

"If I had to do this, I would first write a patch for #1: A patch that
additionally executes existing privilege checks against an SELinux
policy. Existing privilege checks are a well-defined set: they mostly
happen through pg_xxx_aclcheck() functions. Hook your checks in
there."

I really don't understand why this is so difficult. But I don't think
you should bother resubmitting this patch until you've taken this
advice. I am kind of running out patience here. I've reviewed this
patch 3 times and found the exact same issue each time.

...Robert