[PATCH] Reworks for Access Control facilities (r2350)

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:

Please review the new revision, Thanks,

In general, I'm pretty happy with this revision. You still have a
number of places where you have comments about code which does not exist
any more. For example, the comments about the check being removed from
LookupCreationNamespace. I would recommend pulling out those comments
and instead having a comment at the top of the function that says
"namespace creation permission checks are handled in the individual
object ac_*_create() routines".

I don't like having comments that are about code which was removed.
Some of these could be moved to the README if they aren't there already
and they really need to be kept.

There are some other grammatical and spelling issues in the comments,
but I don't believe any of this should hold this patch up from being
ready for committer. At a minimum, I think this really needs to have a
committer comment on it to ensure we're going in the right direction.
I'd be happy to continue working with KaiGai to review his changes going
forward, either with the next set of SE-PG patches or reworking this one
if necessary.

Thanks,

Stephen

Stephen, Thanks for your reviewing comments, although you have busy days.

Stephen Frost wrote:

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:

Please review the new revision, Thanks,

In general, I'm pretty happy with this revision. You still have a
number of places where you have comments about code which does not exist
any more. For example, the comments about the check being removed from
LookupCreationNamespace. I would recommend pulling out those comments
and instead having a comment at the top of the function that says
"namespace creation permission checks are handled in the individual
object ac_*_create() routines".

I don't like having comments that are about code which was removed.
Some of these could be moved to the README if they aren't there already
and they really need to be kept.

OK, I'll check and revise these commenting issues soon.

Please wait for a couple of days at most.

There are some other grammatical and spelling issues in the comments,
but I don't believe any of this should hold this patch up from being
ready for committer. At a minimum, I think this really needs to have a
committer comment on it to ensure we're going in the right direction.
I'd be happy to continue working with KaiGai to review his changes going
forward, either with the next set of SE-PG patches or reworking this one
if necessary.

Thanks,

Stephen

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

The attached patch is a revised one with the following updates:

- rebased to the latest CVS HEAD
- eliminated comments about code which already removed, such as "we had
ACL_xxx checks here, but it is moved to ac_xxx_create()", and some of
notifications are moved to the README.
(comments about LookupCreationNamespace() and CheckRelationOwnership())
- removed ac_relation_permission() invocation from OpenIntoRel()
because the default PG model uses the perspective CREATE TABLE AS is
an atomic operation, due to the defaultACL thread.
(It is already talked with Stephen, and agreed.)
- fixed two bugs:
* ac_index_create() didn't bypass checks on bootstraping mode.
* ac_schema_alter() didn't checks ACL_CREATE on changing owner.

Thanks,

Stephen Frost wrote:

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:

Please review the new revision, Thanks,

In general, I'm pretty happy with this revision. You still have a
number of places where you have comments about code which does not exist
any more. For example, the comments about the check being removed from
LookupCreationNamespace. I would recommend pulling out those comments
and instead having a comment at the top of the function that says
"namespace creation permission checks are handled in the individual
object ac_*_create() routines".

I don't like having comments that are about code which was removed.
Some of these could be moved to the README if they aren't there already
and they really need to be kept.

There are some other grammatical and spelling issues in the comments,
but I don't believe any of this should hold this patch up from being
ready for committer. At a minimum, I think this really needs to have a
committer comment on it to ensure we're going in the right direction.
I'd be happy to continue working with KaiGai to review his changes going
forward, either with the next set of SE-PG patches or reworking this one
if necessary.

Thanks,

Stephen

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

2009/10/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is a revised one with the following updates:

Despite two fairly explicit requests, this patch (and, with the
exception of ECPG, only this patch) has not yet been reviewed by a
committer.

http://archives.postgresql.org/pgsql-hackers/2009-10/msg00591.php
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00652.php

Are any of the committers willing to take a look at this? Tom?
Alvaro, maybe? Bruce?

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Are any of the committers willing to take a look at this? Tom?

I do plan to look at it tomorrow.

regards, tom lane

Robert Haas wrote:

2009/10/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is a revised one with the following updates:

Despite two fairly explicit requests, this patch (and, with the
exception of ECPG, only this patch) has not yet been reviewed by a
committer.

http://archives.postgresql.org/pgsql-hackers/2009-10/msg00591.php
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00652.php

Are any of the committers willing to take a look at this? Tom?
Alvaro, maybe? Bruce?

In actually, I cannot believe this patch to be perfectly commitable
by the 15-Oct due to the remaining time, but it is necessary to be
comittable at the head of the next commit fest.
In other word, I strongly want to continue the discussion and revising
the patch, even if it will be actually commited at the 15-Nov.

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

On Wed, Oct 14, 2009 at 9:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Are any of the committers willing to take a look at this?  Tom?

I do plan to look at it tomorrow.

Oh, great. You've done an impressive job slogging through a bunch of
big, complex patches in the last week.

Thanks,

...Robert

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch r2363 ]

I promised I would review this today, but I just can't make myself do it
in any detail. This is too large, too ugly, and it is going in a
direction that I do not like or want to spend any of my time on.

The overwhelming impression after a brief read-through is that the
code has been hacked apart with a chainsaw and reassembled into a
Frankenstein's monster --- it's alive, but man is it ugly. Code
comments that refer to something "above" or "below" are still there,
but the referent is no longer close enough for that to be a reasonable
way of referring to it. It's impossible to follow what's going on or
why, either in the shim functions or in the callers --- in the original
coding there was context for the aclcheck calls, now there isn't.

I don't have any confidence that this is a sane way to proceed forward.
The shim layer knows everything about everything --- there may still
be a few backend .h files it doesn't include, but that's not for lack
of trying. The direction this is heading in is an unmaintainable
giant-bowl-of-spaghetti security module, rather than something that can
be divided into understandable parts. And I don't think it's really
removed any complexity from the callers, nor do I believe that it's
going to be a useful basis for imposing a different security policy
than the one we have now. Two specific examples of why not:

* The "skip permissions checks" arguments that have been added to
various random functions suggest strongly that the factoring still isn't
right --- I especially don't believe in that in the context of
performDeletion and friends.

* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c. What if that code were still in contrib? How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules? Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.

Maybe if I weren't burned out after a month of CommitFesting, I could
muster a more positive reaction, but right now I just can't summon any
enthusiasm for this.

regards, tom lane

Tom Lane wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

[ patch r2363 ]

I promised I would review this today, but I just can't make myself do it
in any detail. This is too large, too ugly, and it is going in a
direction that I do not like or want to spend any of my time on.

The overwhelming impression after a brief read-through is that the
code has been hacked apart with a chainsaw and reassembled into a
Frankenstein's monster --- it's alive, but man is it ugly. Code
comments that refer to something "above" or "below" are still there,
but the referent is no longer close enough for that to be a reasonable
way of referring to it. It's impossible to follow what's going on or
why, either in the shim functions or in the callers --- in the original
coding there was context for the aclcheck calls, now there isn't.

Quite frankly, I felt disappointed that we have to repeat these kind of
design level issues again. :-(

The purpose of this patch is to provide function entrypoints for the
upcoming SE-PostgreSQL feature, because I got a few comments that we
hesitate to put sepgsql_xxx() hooks on the main routines directly in
the first commit fest. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.
Then, we made a direction to add an abstraction layer for the purpose
of access controls which can be available both of DAC and MAC.

Apart from this patch, we need to consider the preferable way to host
additional security models in PostgreSQL again.

I don't have any confidence that this is a sane way to proceed forward.

Indeed, ac_xxx_*() routines needs a large scale changes for the core.
However, we didn't have any other way, if both of security model have
to use common entry points.

In the original design, I put sepgsqlCheckXXX() hooks which does not
affect anything if disabled on the main routines, and it works well.
I would like to consider why reviewers felt these hooks are (possibly)
hard to maintain again.

One reason was the hooks reflected individual SELinux permissions.

e.g) sepgsqlCheckProcedureInstall(Oid procOid);

It checks user's privilege to use a certain function as a system internal
stuff which is executed on runtime without individual execution permission
checks, like an implementation of conversion.

However, it may need future contributors to understand the intention
why the hooks were deployed here, without enough knowledge about SELinux.
In other word, the hooks represented how SELinux makes its decision
(method), not what SELinux make its decision on (purpose).

Instead of this ac_xxx_*() routines and previous sepgsqlCheckXXX()
routines, I would like to propose SE-PG hooks which reflects the
purpose of security checks.

e.g) sepgsql_relation_create(char *relName, Oid namespace_oid, ...);

It internally compute the default security context of the new table
and checks permission on the table itself and the namespace to be
created on. The series of checks consists of a permission check to
create a new table in totally.

I think it is not a major issue whether this patch is applied, or not.
What is important is to point out the right direction to host SELinux
security model correctly.

At the PGcon2008 keynote, Bruce talked that our road to the summit is
similar to a bendy road. It means our development does not always
go into the right direction, but we are certainly getting near to the
summit.
I've tried several approaches for more than two years, but I cannot
feel we are getting near to the summit yet.

At least, we need to decide where we should go on the next at the
end of this commit fest.

The shim layer knows everything about everything --- there may still
be a few backend .h files it doesn't include, but that's not for lack
of trying. The direction this is heading in is an unmaintainable
giant-bowl-of-spaghetti security module, rather than something that can
be divided into understandable parts. And I don't think it's really
removed any complexity from the callers, nor do I believe that it's
going to be a useful basis for imposing a different security policy
than the one we have now. Two specific examples of why not:

* The "skip permissions checks" arguments that have been added to
various random functions suggest strongly that the factoring still isn't
right --- I especially don't believe in that in the context of
performDeletion and friends.

The reason why we needed to put permission checks on the routines in
dependency.c is that we cannot know what objects are dropped due to
the cascaded deletion.
But some of purely internal stuffs (such as cleaning up temporary
objects) uses the routines without any necessity of permission checks.
So, the flag to control permission check is necessary.

* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c. What if that code were still in contrib? How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules? Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.

Back to the definition of access controls (or reference monitor).
It prevents violated accesses launched by user's requests (SQL).
It is not a job to protect something from malicious internal modules.
The loadable kernel module is a good analogy. It is allowed anything
because kernel module can access directly without system-call invocations.
However, OS checks whether the admin has an appropriate privilege, or not,
when the kernel module tries to be loaded.
It is also DBA's decision whether he allows to load a third-party module,
or not.

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

On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe if I weren't burned out after a month of CommitFesting, I could
muster a more positive reaction, but right now I just can't summon any
enthusiasm for this.

Based on this review, I am marking this patch Rejected.

For what it's worth, I took a quick look at this just to see if I had
any reason to disagree with your conclusions. I don't.

...Robert

Robert Haas wrote:

On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe if I weren't burned out after a month of CommitFesting, I could
muster a more positive reaction, but right now I just can't summon any
enthusiasm for this.

Based on this review, I am marking this patch Rejected.

Basically, I need to agree in spite of Stephen's efforts.

For what it's worth, I took a quick look at this just to see if I had
any reason to disagree with your conclusions. I don't.

Sorry, please make clear the "your conclusions"?

Does it mean that Tom's comment that this reworking does not go into
the right direction? Or, my comment on the last message?

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

KaiGai Kohei wrote:

The purpose of this patch is to provide function entrypoints for the
upcoming SE-PostgreSQL feature, because I got a few comments that we
hesitate to put sepgsql_xxx() hooks on the main routines directly in
the first commit fest. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.

Can you elaborate that? It might well be that you need to adapt the
SE-PostgreSQL security model to the one that's there already. Putting
SE-PG hooks into existing pg_xxx_aclcheck functions is the only
low-impact way I can see to implement SE-PostgreSQL.

* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c. What if that code were still in contrib? How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules? Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.

Back to the definition of access controls (or reference monitor).
It prevents violated accesses launched by user's requests (SQL).
It is not a job to protect something from malicious internal modules.

The issue isn't malicious modules, but modules that have pg_xxx_aclcheck
calls in them and haven't been modified to do SE-pgsql checks like you
modified all the backend code. As the patch stands, they would perform
just the regular acl checks and bypass SE-pgsql.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2009/10/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:

. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.

I thought the last discussion ended with a pretty strong conclusion
that we didn't want differences in the security models.

The first step is to add hooks which don't change the security model
at all, just allow people to control the existing checks from their SE
configuration. Only as a second step we would look into making
incremental changes to the postgres security model to add support for
privileges SE users might expect to find, eventually possibly
including per-row permissions.

--
greg

Greg Stark <gsstark@mit.edu> writes:

The first step is to add hooks which don't change the security model
at all, just allow people to control the existing checks from their SE
configuration.

This is in fact what the presented patch is meant to do. The issue is
about whether the hook placement is sane/useful/extensible. The main
problem I've got with the design is that it doesn't appear to work for
privilege checks made by add-on modules.

regards, tom lane

On Fri, Oct 16, 2009 at 12:45 PM, Greg Stark <gsstark@mit.edu> wrote:

2009/10/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:

. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.

I thought the last discussion ended with a pretty strong conclusion
that we didn't want differences in the security models.

The first step is to add hooks which don't change the security model
at all, just allow people to control the existing checks from their SE
configuration. Only as a second step we would look into making
incremental changes to the postgres security model to add support for
privileges SE users might expect to find, eventually possibly
including per-row permissions.

I think we sort of came to the conclusion that even a basic
implementation of SE-PostgreSQL might have some requirements that
didn't quite square with the existing PostgreSQL security model. The
charter of this patch AIUI was to refactor things so that they were
square up, but I the patch is substantially more complex and invasive
than what I thought would be necessary and it's not clear that it
solves the problem. Rather than refactoring the existing checks to
provide a cleaner abstraction layer, it seems to provide a layer that,
if it's anything, is just a place-holder for an SE-PostgreSQL
implementation, and there's no guarantee that it's adequate even for
that, much less for anything else we might want to do.

...Robert

Heikki Linnakangas wrote:

KaiGai Kohei wrote:

The purpose of this patch is to provide function entrypoints for the
upcoming SE-PostgreSQL feature, because I got a few comments that we
hesitate to put sepgsql_xxx() hooks on the main routines directly in
the first commit fest. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.

Can you elaborate that? It might well be that you need to adapt the
SE-PostgreSQL security model to the one that's there already. Putting
SE-PG hooks into existing pg_xxx_aclcheck functions is the only
low-impact way I can see to implement SE-PostgreSQL.

We can show several examples that pg_xxx_aclcheck() routines are not
suitable to implement SELinux's security model.
Please note that it is not a defect of the default PG's security model
needless to say. It is just a different in standpoints.

1) creation of a database object

In SELinux model, when a user tries to create a new object (not limited
to database object, like a file or socket), a default security context
is assigned on the new object, then SELinux checks whether the user has
privileges to create a new object labeled with the security context, or not.

When we create a new table, the default PG model checks ACL_CREATE privilege
on the namespace which is supposed to own the new table. DefineRelation()
invokes pg_namespace_aclcheck() with OID of the namespace, but we cannot
see any properties of the new table from inside of pg_namespace_aclcheck().
It checks permissions on the couple of a user and a namespace.

On the other hand, SE-PG model follows the above principle. When we create
a new table, SE-PG compute a default security context to be assigned on,
then it checks the security policy whether the user is allowed to create
a new table labeled with the context, or not.
It checks permissions on the couple of a user and a new table itself.

The caller does not provide enough information to the pg_xxx_aclcheck(),
so we decided to create an abstraction layer which can provide enough
informations to both of security models. Then, the ac_xxx_*() routines
were implemented.

2) AND-condition for all the privileges

When a certain action requires multiple permissions at one time,
the principle of SELinux is that all the permissions have to be checked.
If one of them is not allowed, it disallows the required action.
In other word, all the conditions are chained by AND.

This principle enables us to analyze the data flows between users and
resources with the security policy, without implementation details.
If a certain permission (e.g db_table:{select}) can override any other
permission (e.g db_column:{select}), it also implicitly means a possibility
of infotmation leaks/manipulations, even if the security policy said this
user cannot read a data from the column.

On the other hand, the default PG model allows to bypass checks on
certain objects. For example, column-level privileges are only checked
when a user does not have enough permissions on the target table.
If "SELECT a,b FROM t" is given, pg_attribute_aclcheck() may not invoked
when user has needed privileges on the table t.

3) superuser is not an exception of access control.

It is the similar issue to the 2).
The following code is a part of AlterFunctionOwner_internal().

----------------
/* Superusers can always do it */
if (!superuser())
{
/* Otherwise, must be owner of the existing object */
if (!pg_proc_ownercheck(procOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
NameStr(procForm->proname));

/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), newOwnerId);

/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(procForm->pronamespace,
newOwnerId,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
get_namespace_name(procForm->pronamespace));
}
----------------

From perspective of the default PG model, this code perfectly correct.
Both of pg_proc_ownercheck() and pg_namespace_aclcheck() always returns
ACLCHECK_OK, so these invocations are bypassable.

However, if SE-PG's hooks are deployed on pg_xxx_aclcheck() routines,
it means that we cannot check correct MAC permissions when a client is
allowed to apply superuser privilege.
Please remind that SELinux requires AND-condition for all the privileges
required to a certain action. When a root user tries to read a certain
file without DAC permisions, it requires both of capability:{dac_override}
and file:{read} permissions in operating system.

These are a part of reasons why we had to design such a large patch.
If we stick around the common entrypoints of access controls, such kind of
reworks are necessary. The current pg_xxx_aclcheck() routines are designed
for the database ACL model. It works fine and correctly.
However, it is not suitable to host the SELinux's model within the hooks.

* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c. What if that code were still in contrib? How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules? Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.

Back to the definition of access controls (or reference monitor).
It prevents violated accesses launched by user's requests (SQL).
It is not a job to protect something from malicious internal modules.

The issue isn't malicious modules, but modules that have pg_xxx_aclcheck
calls in them and haven't been modified to do SE-pgsql checks like you
modified all the backend code. As the patch stands, they would perform
just the regular acl checks and bypass SE-pgsql.

OK, I can understand what he wanted to say.
In the kernel module cases, we can find out modules which provide its own
checks based on user/group identifier, so it also means using the loadable
module allows to bypass MAC checks. However, the loadable module is loaded
at first, and the administrator (or init script) is allowed to load such
a loadable module which bypasses MAC checks.
In other word, the security policy basically controls whole of the usage
of these modules. Needless to say, we can add security hooks if necessary.

If we look at the SE-PgSQL project on the greater scale, it also can be
considered as an efforts to add MAC checks on the module which applied
its own access controls, but bypassed MAC checks.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Greg Stark wrote:

2009/10/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:

. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.

I thought the last discussion ended with a pretty strong conclusion
that we didn't want differences in the security models.

It is not a fact. Because the SE-PG patch is a bit large to review,
I got a suggestion to implement a part of permissions checks which
can be invoked from the pg_xxx_aclcheck() without any breaks for
SELinux's security model, at the first step.
In other word, I tried to implement only union part of the security
models.

The first step is to add hooks which don't change the security model
at all, just allow people to control the existing checks from their SE
configuration. Only as a second step we would look into making
incremental changes to the postgres security model to add support for
privileges SE users might expect to find, eventually possibly
including per-row permissions.

I already did it on the first CF...
However, most of permission checks had gone at the first step.
It was commented it is same as checks nothing.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

KaiGai Kohei wrote:

1) creation of a database object

In SELinux model, when a user tries to create a new object (not limited
to database object, like a file or socket), a default security context
is assigned on the new object, then SELinux checks whether the user has
privileges to create a new object labeled with the security context, or not.

When we create a new table, the default PG model checks ACL_CREATE privilege
on the namespace which is supposed to own the new table. DefineRelation()
invokes pg_namespace_aclcheck() with OID of the namespace, but we cannot
see any properties of the new table from inside of pg_namespace_aclcheck().
It checks permissions on the couple of a user and a namespace.

On the other hand, SE-PG model follows the above principle. When we create
a new table, SE-PG compute a default security context to be assigned on,
then it checks the security policy whether the user is allowed to create
a new table labeled with the context, or not.
It checks permissions on the couple of a user and a new table itself.

I don't think I buy that argument. Can't we simply decide that in
PostgreSQL, the granularity is different, and you can only create
policies governing creation of objects on the basis of schema+user
combination, not on the properties of the new object. AFAICS it wouldn't
violate the principle of Mandatory Access Control.

2) AND-condition for all the privileges

When a certain action requires multiple permissions at one time,
the principle of SELinux is that all the permissions have to be checked.
If one of them is not allowed, it disallows the required action.
In other word, all the conditions are chained by AND.

This principle enables us to analyze the data flows between users and
resources with the security policy, without implementation details.
If a certain permission (e.g db_table:{select}) can override any other
permission (e.g db_column:{select}), it also implicitly means a possibility
of infotmation leaks/manipulations, even if the security policy said this
user cannot read a data from the column.

On the other hand, the default PG model allows to bypass checks on
certain objects. For example, column-level privileges are only checked
when a user does not have enough permissions on the target table.
If "SELECT a,b FROM t" is given, pg_attribute_aclcheck() may not invoked
when user has needed privileges on the table t.

Hmm, I see. Yes, it does seem like we'd need to change such permission
checks to accommodate both models.

3) superuser is not an exception of access control.

It is the similar issue to the 2).

Yeah.

The following code is a part of AlterFunctionOwner_internal().

----------------
/* Superusers can always do it */
if (!superuser())
{
/* Otherwise, must be owner of the existing object */
if (!pg_proc_ownercheck(procOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
NameStr(procForm->proname));

/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), newOwnerId);

/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(procForm->pronamespace,
newOwnerId,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
get_namespace_name(procForm->pronamespace));
}
----------------

From perspective of the default PG model, this code perfectly correct.
Both of pg_proc_ownercheck() and pg_namespace_aclcheck() always returns
ACLCHECK_OK, so these invocations are bypassable.

However, if SE-PG's hooks are deployed on pg_xxx_aclcheck() routines,
it means that we cannot check correct MAC permissions when a client is
allowed to apply superuser privilege.
Please remind that SELinux requires AND-condition for all the privileges
required to a certain action. When a root user tries to read a certain
file without DAC permisions, it requires both of capability:{dac_override}
and file:{read} permissions in operating system.

We need to ask ourselves, is that a realistic goal, given how widespread
such "if (superuser())" calls are? And more imporantly, unless you
sprinkle additional fine-grained permission checks to all the places
that currently just check "if (superuser())", it will be possible to
circumvent the system with LOAD or any of the other commands that are
inherently dangerous. We don't want such additional fine-grained
permissions, not for now at least.

Seems a lot simpler and also easier to understand if there's a single
superuser privilege that trumps all other permission checks.

If we look at the SE-PgSQL project on the greater scale, it also can be
considered as an efforts to add MAC checks on the module which applied
its own access controls, but bypassed MAC checks.

Yeah, it seems like any external modules need to be modified or at least
verified to comply with the MAC requirements. Your point 2) about
whether permissions are ANDed or ORred together seem to be the key here.

This raises an important point: We need *developer documentation* on how
to write SE-Pgsql compliant permission checks. Not only for authors of
3rd party modules but for developers of PostgreSQL itself. Point 2)
above needs to be emphasized, it's a big change in the way permission
checks have to be programmed. One that I hadn't realized before. I
haven't been paying much attention, but neither is most other
developers, so we need clear documentation.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

This raises an important point: We need *developer documentation* on how
to write SE-Pgsql compliant permission checks. Not only for authors of
3rd party modules but for developers of PostgreSQL itself. Point 2)
above needs to be emphasized, it's a big change in the way permission
checks have to be programmed. One that I hadn't realized before. I
haven't been paying much attention, but neither is most other
developers, so we need clear documentation.

This is a good point. All throughout these discussions, there has
been a concern that whatever is implemented here will be
unmaintainable because we don't have any committers who are familiar
with the ins and outs of SE-Linux and MAC (and not too many other
community members interested in the topic, either). So some developer
documentation seems like it might help.

On the other hand, KaiGai has made several attempts at documentation
and several attempts at patches and we're not really any closer to
having SE-PostgreSQL in core than we were a year ago. I think that's
partly because KaiGai tried to bite off far too much initially
(still?), partly because of technical problems with the patches,
partly because the intersection of people who are experts in
PostgreSQL and people who are experts in MAC seems to be empty, and
partly because, as much as people sorta kinda like this feature,
nobody other than KaiGai has really been willing to step up and pour
into this project the kind of resources that it will likely require to
be successful.

I have to admit that I'm kind of giving up hope. We seem to be going
in circles, and I don't think anything new is being said on this
thread that hasn't been said before.

...Robert