Re: Updates of SE-PostgreSQL 8.4devel patches (r1668)

Heikki, Thanks for your comments.

Heikki Linnakangas wrote:
> Ok, I've taken a quick look at this too. My first impression is that
> this is actually not a very big patch. Much much smaller than I was
> afraid of. It seems that dropping the row-level security and the other
> change you've already done have helped a great deal.
>
> My first question is, why does the patch need the walker implementation
> to gather all the accessed tables and columns? Can't you hook into the
> usual pg_xxx_aclcheck() functions? In fact, Peter asked that same
> question here:
> http://archives.postgresql.org/pgsql-hackers/2009-01/msg02295.php (among
> other things). Many things have changed since, but I don't think that
> question has been adequately answered. Different handling of permissions
> on views was mentioned, but I think that could be handled with just a
> few extra checks in the rewriter or executor.

Yes, one major reason is to handle views. SE-PostgreSQL need to check
permissions on after it is extracted.

The other one is it has two kind of reader permissions ("select" and "use").
The "select" permission is applied when user tries to read tables/columns
and its contents are returned to the user.
The "use" permission is applied when user tries to read table/columns,
but its contents are consumed internally (not returned to user directly).

For example:
SELECT a, b FROM t WHERE b > 10 and c = 'aaa';

In this case,
db_column:{select} permission is applied on "t.a".
db_column:{select use} permission is applied on "t.b".
db_column:{use} permission is applied on "t.c".
db_table:{select use} permission is applied on "t"

However, I don't absolutely oppose to integrate them into a single
reader "select" permission, because it was originally a single
permission, then "use" is added.

The purpose of "use" permission is to set up a writable table, but
not readable. When we use UPDATE or DELETE statement, it need to
add WHERE clause to make clear its target, but it always requires
reader permission. So, I separated it into two cases.

Thus, it is not a reason as strong as one for views.

I'll check some of corner cases, such as inherited tables, COPY
statement, trigger invocations and others, to consider whether
your suggestion is possible, or not.
Please wait for a while to fix my attitude.

> The hooks in simple_heap_insert also seem a bit weird. Perhaps an
> artifact of the row-level security stuff that's no longer there. ISTM
> that setting the defaults should be done in the same places where the
> defaults for acl columns are filled, e.g in ProcedureCreate.

Its purpose is not set a default security label in the current version.
(It is set in ProcedureCreate and others.)
Its purpose is to check user's privileges on tables, columns, procedures
and databases, and raises an error if violated.

Please note that user's privileges are not limited to create/alter/drop
them. One sensitive permission is "db_procedure:{install}".
It is checked when user defined functions are set up as a function
internally invoked.

For example, "pg_conversion.conproc" is internally invoked to translate
a text, but it does not check pg_proc_aclcheck() in runtime.
We consider all user defined functions should be checked either of:
- "db_procedure:{execute}" permission for the client in runtime
or
- "db_procedure:{install}" permission for the DBA on installation time

Needless to say, "{install}" is more sensitive permission because it
means anyones to invoke it implicitly. So, the default policy only
allows it on functions defined by DBA, but the "execute" permission
is allowed normal users to invoke functions defined by himself.

sepgsqlCheckProcedureInstall() checks this permission called from
the hooks of simple_heap_insert()/_update().
From the viewpoint of security, it is good design to put hooks on
more frequently used point, than checking it many points.

It is same reason why SELinux checks system-calls from applications.
It is the only path to access resources managed by operating system,
so necessary and sufficient.

> PS. s/proselabal/proselabel

Oops,

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