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

Robert Haas wrote:
> 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.

Yes, the tiny version will not give any advantages in security without
future enhancements.
It is not difficult to add object classes and permissions.
If necessary, I'll add checks them with corresponding permissions.

One anxiety is PostgreSQL specific object class, such as LANGUAGE.
It's not clear for me whether the maintainer of the SELinux security
policy accept these kind of object classes, or not.
I would like to implement them except for PostgreSQL specific object
class in this phase.

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

Here is a few differences in access control model between PostgreSQL and
SELinux, so I could not map all the SELinux permissions on the pg_xxx_aclcheck()
mechanism.

For example, ExecCheckRTEPerms() checks permissions on the tables and
columns appeared in the user given query. When the user have SELECT
permission on the required table, it bypasses to check permissions on
the columns.
SELinux's security model needs to check permissions on all the required
objects. For example, "SELECT A,B FROM T" requires the client to have
db_table:{select} on T and db_column:{select} on A and B.

For other example, some of pg_xxx_aclcheck() is bypassed when the client
has superuser privilege. In this case, SELinux requires the client to
have both of db_database:{superuser} and a certain permission.

If we deploy security hooks at the pg_xxx_aclcheck() or related stuff
at the first version, it is necessary to move them more appropriate
points later.
I can agree to deploy security hooks on pg_xxx_aclcheck(), if we can
move them to other points which can implement security model correctly
in the later version.

Sorry, I could not read it from the previous suggestions.
If you have been suggesting it repeatedly, I'm sorry so much.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>