Re: Reworks for Access Control facilities (r2363)

The attached patch is a revised version based on the previous
discussions at:

http://archives.postgresql.org/message-id/20090929105431.GO17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC1EA9E.3080907@kaigai.gr.jp
http://archives.postgresql.org/message-id/20090929173049.GP17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC2BDD0.7050906@ak.jp.nec.com
http://archives.postgresql.org/message-id/20090930105911.GS17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC40133.4080509@ak.jp.nec.com

Please review the new revision, Thanks,

* List of updates

- code base was updated to the latest CVS HEAD.
- reverted changes on FindConversion() and EnableDisableRule().
these changes are discussed in the different topics.
- removed uncertain comment at the restrict_grant().
- added comment about SQL specifications for each ac_xxx_grant().
- eliminate MEMO: and FIXME: prefix
- moved ac_language_create() prior to the CreateProcedure() because
it may update the pg_proc system catalog.
- removed ac_schema_search() invocations when the target namespace is
obviously temporary namespace. And, added a comment to bypass checks
for both of DAC/MAC on temporary namespaces.
- uncommented "ac_object_drop() should be here", and added actual
ac_object_drop() at the performDeletion() and performMultipleDeletion().
The 'permission' argument was added to these functions.
- uncommented "ac_attribute_xxxx() should be here", and put actual
ac_attribute_create() and ac_attribute_drop() calls here.
- ac_aggregate_execute() function was added.
- add a memo for minor behavior changes at src/backend/security/README
(It is a initial description, so needs more brushing up)

$ diffstat sepgsql-01-base-8.5devel-r2350.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 254 !
backend/catalog/dependency.c | 31
backend/catalog/heap.c | 2
backend/catalog/namespace.c | 54
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 29
backend/catalog/pg_shdepend.c | 13
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 44
backend/commands/alter.c | 78
backend/commands/analyze.c | 5
backend/commands/cluster.c | 11
backend/commands/comment.c | 125
backend/commands/conversioncmds.c | 73
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 150
backend/commands/functioncmds.c | 132
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 246 !
backend/commands/operatorcmds.c | 72
backend/commands/proclang.c | 63
backend/commands/schemacmds.c | 62
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 370 -
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 43
backend/commands/tsearchcmds.c | 182 !
backend/commands/typecmds.c | 143 !
backend/commands/user.c | 183 !
backend/commands/vacuum.c | 5
backend/commands/view.c | 7
backend/executor/execMain.c | 208 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 38
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 42
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/postmaster/autovacuum.c | 2
backend/rewrite/rewriteDefine.c | 5
backend/rewrite/rewriteRemove.c | 8
backend/security/Makefile | 10
backend/security/README | 294 ++
backend/security/access_control.c | 4593 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 15
include/catalog/dependency.h | 4
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 348 ++
58 files changed, 5747 insertions(+), 914 deletions(-), 1986 modifications(!)

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

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2009/10/13 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

On Wed, Oct 14, 2009 at 9:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)mit(dot)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(at)mit(dot)edu> wrote:
> 2009/10/16 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

Greg Stark wrote:
> 2009/10/16 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)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(dot)linnakangas(at)enterprisedb(dot)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

Heikki Linnakangas wrote:
> 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.

No, it violates the principle.
I omitted a case for simplification of explanations.
When we create a new object, we can provide an explicit security context
to be assigned on the new object, instead of the default one.
In this case, SELinux checks privilege to create the object with the
given security context. (If it is disallowed, this creation will be
failed.)

If we check MAC permission to create a new object based on a couple
of user and schema which owns the new one, it also allows users to
create a new object with arbitrary security context, because this
check is not applied on security context of the new object itself.

It is a reason why SELinux is MAC. It never allows to create a new
object with a violated security context. The only way to control
this policy is to check privileges on the pair of user and the new
object. Thus, SELinux defines its permission to create a new object
on various kind of "objects"; not limited to database objects such
as files, sockets, IPC, x-window and so on.

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

I'm not clear why we need to rework the permission checks here.
DAC and MAC perform orthogonally and independently.
DAC allows to override column-level privileges by table-level privileges
according to the default PG's model. It seems to me fine.
On the other hand, MAC checks both of permissions. It is also fine.

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

It may be an ideal goal, but it is far from what we tries to do.
Some of actions can be allowed without any MAC checks more than
"if (superuser())" which internally checks SE-PgSQL's permission
to perform superuser in DAC.

The most significant purpose of MAC is to control data-flows between
processes via shared resources (such as files, databases).
So, SELinux/SE-PgSQL primarily focuses on the operation to access
database objects. An important thing is to make clear the priority
what actions should be also checked by MAC, not only signle superuser
privilege.

I don't believe a single patch which add checks on various kind of
database objects is acceptable within a reasonable time-frame.
But we can adopt incremental approach. It is not necessary to put
SE-PgSQL's hooks near the all of "if (superuser())" at beginning.

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

Yes, I also think we need a documentation from developer viewpoint.
(not only user documentation)

I think it should contains the following items.
* overview and architecture
(including differences from the default PG's model?)
* what permissions are defined in SELinux model
* when/where they should be checked
* specification of SE-PgSQL hooks

What item should be described in the developer documentation any other?
In generally, what I want to describe may not match with what people want
to know.

Thanks,

BTW, I have to allocate my activity on Japan Linux Symposium in this week.
So, my response may be delayed. Sorry.

http://events.linuxfoundation.org/events/japan-linux-symposium/schedule

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

Robert Haas wrote:
> On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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.

We may not be always able to find out the right way to the mountain summit.
Indeed, it seems that we returned to the original design which deploys
SE-PgSQL's hooks on the strategic points.
But there is a significant improvement. We learned several designs
which we already tried were on the rocky path, although they look like
an easy path at first.

I agrre to the Heikki's suggestion.
Not only user documentation, we need another documentation from the
viewpoint of developer, which describes what permissions are defined,
what is the purpose of SE-PgSQL's hooks and when/where these are called.

Thanks,

BTW, as I noted in the last message, I have to allocate my activities
to Japan Linux Symposium in this week. So, responses may delay, Sorry.

http://events.linuxfoundation.org/events/japan-linux-symposium/schedule

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

KaiGai Kohei wrote:
> When we create a new object, we can provide an explicit security context
> to be assigned on the new object, instead of the default one.

To get started, do we really need that feature? It would make for a
significantly smaller patch if there was no explicit security labels on
objects.

>>> 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.
>
> I'm not clear why we need to rework the permission checks here.
> DAC and MAC perform orthogonally and independently.
> DAC allows to override column-level privileges by table-level privileges
> according to the default PG's model. It seems to me fine.
> On the other hand, MAC checks both of permissions. It is also fine.

I meant we need to refactor the code doing the permission checks. The
existing checks are doing the right thing for DAC, but as you point out,
if the MAC checks are within pg_*_aclcheck() functions,
pg_attribute_aclcheck() needs to be called even if you have privilege on
the table.

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

Heikki Linnakangas wrote:
> KaiGai Kohei wrote:
>> When we create a new object, we can provide an explicit security context
>> to be assigned on the new object, instead of the default one.
>
> To get started, do we really need that feature? It would make for a
> significantly smaller patch if there was no explicit security labels on
> objects.

The importance of the feature is relatively minor than MAC itself.
So, I can agree to omit code corresponding to statement support
from the first patch. (IIRC, about 300-400 lines can be reduced.)
But it will be necessary feature at the next step, because DBA cannot
create a special purpose table without statement support.

For example, if security policy allows DBA to create read-writable
table (in default) and read-only table. He cannot set up read-only
table without explicit security label support.

>>>> 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.
>> I'm not clear why we need to rework the permission checks here.
>> DAC and MAC perform orthogonally and independently.
>> DAC allows to override column-level privileges by table-level privileges
>> according to the default PG's model. It seems to me fine.
>> On the other hand, MAC checks both of permissions. It is also fine.
>
> I meant we need to refactor the code doing the permission checks. The
> existing checks are doing the right thing for DAC, but as you point out,
> if the MAC checks are within pg_*_aclcheck() functions,
> pg_attribute_aclcheck() needs to be called even if you have privilege on
> the table.

I think we already learned refactoring DAC checks need widespread code
changes and pushes a burden to reviewers.

In this case, I think the point just after invocation of ExecCheckRTEPerms()
in ExecCheckRTPerms() is the best point to put SE-PgSQL's checks.
Needless to say, its specification should be clearly documented.

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

After the long trial-and-errors, we learned a few approaches which
use common entry points for both of DAC and MAC were rocky-path more
than what we initially imagined.
So, we came back to the original design. It deploys MAC hooks on
the strategic points of core routines. On the other hand, people
complained about this approach without clear documentation, because
most of people are not familiar to both of SELinux and PgSQL.
Heikki suggested that a clear developer documentation should be
provided to understand pgsql-hackers this new concept.
(And, Peter has also suggested before a developer documentation will
be a good source of user documentations.)

I plan to submit SE-PgSQL/lite patch with developer documentations
on the next commit-fest.
I can understand what I want to develop and the purpose of codes.
However, it may not match with what you want to know.

So, I'd like to ask what should be included within the developer
documentation at first prior to making a documentation.

I plans the developer documentation should be put as a REAME file,
not a SGML documentation or a certain wiki page.
And I think it should contain the following items.

* overview
- general overview of SE-PgSQL
- introduction of SELinux specific terms (such as "security context")

* internal architecture
- the purpose of sub-components (such as management of security
context, caches of access control decision and so on)
- differences from similar permissions in DAC

* object classes and permissions defined in SELinux model
- list of them and when/where they should be checked.

* specification of SE-PgSQL hooks
(It should be put on the source code comments for easy maintenance.)
- what this hook does, what arguments are required, what result will
be returned.

* code examples
- a few examples to add MAC checks within 3rd party modules.

Do you have any comments? What should be added to? or removed from?

Thanks,

KaiGai Kohei wrote:
> Heikki Linnakangas wrote:
>> KaiGai Kohei wrote:
>>> When we create a new object, we can provide an explicit security context
>>> to be assigned on the new object, instead of the default one.
>> To get started, do we really need that feature? It would make for a
>> significantly smaller patch if there was no explicit security labels on
>> objects.
>
> The importance of the feature is relatively minor than MAC itself.
> So, I can agree to omit code corresponding to statement support
> from the first patch. (IIRC, about 300-400 lines can be reduced.)
> But it will be necessary feature at the next step, because DBA cannot
> create a special purpose table without statement support.
>
> For example, if security policy allows DBA to create read-writable
> table (in default) and read-only table. He cannot set up read-only
> table without explicit security label support.
>
>>>>> 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.
>>> I'm not clear why we need to rework the permission checks here.
>>> DAC and MAC perform orthogonally and independently.
>>> DAC allows to override column-level privileges by table-level privileges
>>> according to the default PG's model. It seems to me fine.
>>> On the other hand, MAC checks both of permissions. It is also fine.
>> I meant we need to refactor the code doing the permission checks. The
>> existing checks are doing the right thing for DAC, but as you point out,
>> if the MAC checks are within pg_*_aclcheck() functions,
>> pg_attribute_aclcheck() needs to be called even if you have privilege on
>> the table.
>
> I think we already learned refactoring DAC checks need widespread code
> changes and pushes a burden to reviewers.
>
> In this case, I think the point just after invocation of ExecCheckRTEPerms()
> in ExecCheckRTPerms() is the best point to put SE-PgSQL's checks.
> Needless to say, its specification should be clearly documented.
>
> Thanks,

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

KaiGai Kohei wrote:
> After the long trial-and-errors, we learned a few approaches which
> use common entry points for both of DAC and MAC were rocky-path more
> than what we initially imagined.
> So, we came back to the original design. It deploys MAC hooks on
> the strategic points of core routines. On the other hand, people
> complained about this approach without clear documentation, because
> most of people are not familiar to both of SELinux and PgSQL.
> Heikki suggested that a clear developer documentation should be
> provided to understand pgsql-hackers this new concept.
> (And, Peter has also suggested before a developer documentation will
> be a good source of user documentations.)
>
> I plan to submit SE-PgSQL/lite patch with developer documentations
> on the next commit-fest.
> I can understand what I want to develop and the purpose of codes.
> However, it may not match with what you want to know.
>
> So, I'd like to ask what should be included within the developer
> documentation at first prior to making a documentation.
>
> I plans the developer documentation should be put as a REAME file,
> not a SGML documentation or a certain wiki page.
> And I think it should contain the following items.
>
> * overview
> - general overview of SE-PgSQL
> - introduction of SELinux specific terms (such as "security context")
>
> * internal architecture
> - the purpose of sub-components (such as management of security
> context, caches of access control decision and so on)
> - differences from similar permissions in DAC
>
> * object classes and permissions defined in SELinux model
> - list of them and when/where they should be checked.
>
> * specification of SE-PgSQL hooks
> (It should be put on the source code comments for easy maintenance.)
> - what this hook does, what arguments are required, what result will
> be returned.
>
> * code examples
> - a few examples to add MAC checks within 3rd party modules.
>
> Do you have any comments? What should be added to? or removed from?

I guess it was too abstract to suggest anything.
Anyway, I'll begin to describe the developer documentation base on the chapters.
If necessary, we can fix it up later.

In addition, I determined the initial patch only covers access controls on
the four object classes (database, schema, table and column) to reduce burdens
of reviewing.
We also can add support for other object classes (such as procedure) later.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Now I'm working on writing a documentation from the viewpoint of
developers as follows (It is a work in progress):

http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/sepgsql/README

Is there any differences between what I want to describe and what you
want to know? If so, I'll add or modify the documentation.

As I noted before, I decided to slim up my patch on the next commit fest
to reduce the burden of reviewing.

The following functionalities will be separated from the prior version.
- no access controls on procedures.
-> so, the slim version only support checks on databases, schemas,
tables and columns.
- no statement support to specify security context.
(It makes impossible to add support in pg_dump. Is it really OK?)
- no userspace caches for access control decision
- no system catalog changes to store its security context
-> I'll adopt an approach that Stephen Frost suggested before.
It manages a pair of OID and security context in separated system catalog.

Is there any comment? Or, more detailed introductions are necessary?

Thanks,

KaiGai Kohei wrote:
> KaiGai Kohei wrote:
>> After the long trial-and-errors, we learned a few approaches which
>> use common entry points for both of DAC and MAC were rocky-path more
>> than what we initially imagined.
>> So, we came back to the original design. It deploys MAC hooks on
>> the strategic points of core routines. On the other hand, people
>> complained about this approach without clear documentation, because
>> most of people are not familiar to both of SELinux and PgSQL.
>> Heikki suggested that a clear developer documentation should be
>> provided to understand pgsql-hackers this new concept.
>> (And, Peter has also suggested before a developer documentation will
>> be a good source of user documentations.)
>>
>> I plan to submit SE-PgSQL/lite patch with developer documentations
>> on the next commit-fest.
>> I can understand what I want to develop and the purpose of codes.
>> However, it may not match with what you want to know.
>>
>> So, I'd like to ask what should be included within the developer
>> documentation at first prior to making a documentation.
>>
>> I plans the developer documentation should be put as a REAME file,
>> not a SGML documentation or a certain wiki page.
>> And I think it should contain the following items.
>>
>> * overview
>> - general overview of SE-PgSQL
>> - introduction of SELinux specific terms (such as "security context")
>>
>> * internal architecture
>> - the purpose of sub-components (such as management of security
>> context, caches of access control decision and so on)
>> - differences from similar permissions in DAC
>>
>> * object classes and permissions defined in SELinux model
>> - list of them and when/where they should be checked.
>>
>> * specification of SE-PgSQL hooks
>> (It should be put on the source code comments for easy maintenance.)
>> - what this hook does, what arguments are required, what result will
>> be returned.
>>
>> * code examples
>> - a few examples to add MAC checks within 3rd party modules.
>>
>> Do you have any comments? What should be added to? or removed from?
>
> I guess it was too abstract to suggest anything.
> Anyway, I'll begin to describe the developer documentation base on the chapters.
> If necessary, we can fix it up later.
>
> In addition, I determined the initial patch only covers access controls on
> the four object classes (database, schema, table and column) to reduce burdens
> of reviewing.
> We also can add support for other object classes (such as procedure) later.
>
> Thanks,

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

2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> - no statement support to specify security context.
>  (It makes impossible to add support in pg_dump. Is it really OK?)

I doubt that anything without pg_dump support would be even vaguely OK...

...Robert

Robert Haas wrote:
> 2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> - no statement support to specify security context.
>> (It makes impossible to add support in pg_dump. Is it really OK?)
>
> I doubt that anything without pg_dump support would be even vaguely OK...

In my previous experience, it enabled to reduce 300-400 lines of the patch.
But here is no more sense than the 300-400 lines.

In my honest, I like to include a feature to specify an explicit security
context in the patch from the begining.
(It also allows to attach test cases with more variations.)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Robert Haas wrote:
>> 2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> - no statement support to specify security context.
>>> (It makes impossible to add support in pg_dump. Is it really OK?)
>> I doubt that anything without pg_dump support would be even vaguely OK...
>
> In my previous experience, it enabled to reduce 300-400 lines of the patch.
> But here is no more sense than the 300-400 lines.
>
> In my honest, I like to include a feature to specify an explicit security
> context in the patch from the begining.
> (It also allows to attach test cases with more variations.)

Can you explain why that's required for pg_dump support? I was thinking
that there would be no explicit security labels on objects, and
permissions would be checked based on other inherent properties of the
object, like owner, name, schema etc.

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

Heikki Linnakangas wrote:
> KaiGai Kohei wrote:
>> Robert Haas wrote:
>>> 2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>> - no statement support to specify security context.
>>>> (It makes impossible to add support in pg_dump. Is it really OK?)
>>> I doubt that anything without pg_dump support would be even vaguely OK...
>> In my previous experience, it enabled to reduce 300-400 lines of the patch.
>> But here is no more sense than the 300-400 lines.
>>
>> In my honest, I like to include a feature to specify an explicit security
>> context in the patch from the begining.
>> (It also allows to attach test cases with more variations.)
>
> Can you explain why that's required for pg_dump support? I was thinking
> that there would be no explicit security labels on objects, and
> permissions would be checked based on other inherent properties of the
> object, like owner, name, schema etc.

In SELinux model, security context is the only property which can be
used to decision making based on the security policy.
It never uses any other properties, like owner, name, ...

There are two cases when we create a new object.

1) create a new object without any explicit security context.
If we don't have statement support, it is the only case.
In this case, SELinux computes a default security context to be assigned
on the new object. It depends on the client's security context.
Then, it checks "create" permission on a pair of the client's security
context and the default security context. If not allowed, an error will
be raised.

2) create a new object with an explicit security context.
In this case, the given explicit security context will be assigned.
SELinux checks "create" permission on a pair of the client's security
context and the given explicit security context. If not allowed, an error
will be raised.

Please note that SELinux assigns a security context on the managed object
in either cases.

If we don't have any statement support, there are no way to specify
an explicit security context on the new object in creation.
It also means we cannot recover the security context of objects correctly.

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

KaiGai Kohei wrote:
> Heikki Linnakangas wrote:
>> KaiGai Kohei wrote:
>>> Robert Haas wrote:
>>>> 2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>> - no statement support to specify security context.
>>>>> (It makes impossible to add support in pg_dump. Is it really OK?)
>>>> I doubt that anything without pg_dump support would be even vaguely OK...
>>> In my previous experience, it enabled to reduce 300-400 lines of the patch.
>>> But here is no more sense than the 300-400 lines.
>>>
>>> In my honest, I like to include a feature to specify an explicit security
>>> context in the patch from the begining.
>>> (It also allows to attach test cases with more variations.)
>> Can you explain why that's required for pg_dump support? I was thinking
>> that there would be no explicit security labels on objects, and
>> permissions would be checked based on other inherent properties of the
>> object, like owner, name, schema etc.
>
> In SELinux model, security context is the only property which can be
> used to decision making based on the security policy.
> It never uses any other properties, like owner, name, ...

The security context doesn't necessary need to be given explicitly.
Things like network ports, files in filesystems that don't support
security labels are assigned a security context based on some external
policy.

Hmm, I guess the whole feature becomes completely pointless if all
objects always have their default labels, and can't be changed. So I
guess we need that.

I think this discussion started when I wondered why we can't put the
SE-pgsql check for creating an object (e.g table) into
pg_namespace_aclcheck() without changing the signature. The reason you
gave is that we need the security context of the new table being created
to decide if creating such a table is allowed. But assuming that the new
table inherits the security context of the schema it's created in,
pg_namespace_aclcheck() *does* have all the necessary information: it
knows the namespace which determines the new object's security context.
As long as we don't provide syntax to define the security context in the
CREATE command, we're fine, even if there's an ALTER command to change
the security context of the object after the creation.

I'm not sure how much of a difference that detail makes in the big
scheme of things, I'm just trying to find ways to make the patch
minimally invasive..

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

Heikki Linnakangas wrote:
> KaiGai Kohei wrote:
>> Heikki Linnakangas wrote:
>>> KaiGai Kohei wrote:
>>>> Robert Haas wrote:
>>>>> 2009/10/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>>> - no statement support to specify security context.
>>>>>> (It makes impossible to add support in pg_dump. Is it really OK?)
>>>>> I doubt that anything without pg_dump support would be even vaguely OK...
>>>> In my previous experience, it enabled to reduce 300-400 lines of the patch.
>>>> But here is no more sense than the 300-400 lines.
>>>>
>>>> In my honest, I like to include a feature to specify an explicit security
>>>> context in the patch from the begining.
>>>> (It also allows to attach test cases with more variations.)
>>> Can you explain why that's required for pg_dump support? I was thinking
>>> that there would be no explicit security labels on objects, and
>>> permissions would be checked based on other inherent properties of the
>>> object, like owner, name, schema etc.
>> In SELinux model, security context is the only property which can be
>> used to decision making based on the security policy.
>> It never uses any other properties, like owner, name, ...
>
> The security context doesn't necessary need to be given explicitly.
> Things like network ports, files in filesystems that don't support
> security labels are assigned a security context based on some external
> policy.
>
> Hmm, I guess the whole feature becomes completely pointless if all
> objects always have their default labels, and can't be changed. So I
> guess we need that.
>
> I think this discussion started when I wondered why we can't put the
> SE-pgsql check for creating an object (e.g table) into
> pg_namespace_aclcheck() without changing the signature. The reason you
> gave is that we need the security context of the new table being created
> to decide if creating such a table is allowed. But assuming that the new
> table inherits the security context of the schema it's created in,
> pg_namespace_aclcheck() *does* have all the necessary information: it
> knows the namespace which determines the new object's security context.
> As long as we don't provide syntax to define the security context in the
> CREATE command, we're fine, even if there's an ALTER command to change
> the security context of the object after the creation.

What I pointed out is just a part of matters if we try to deploy SE-PgSQL
hooks within aclchk.c routines.

For example, pg_namespace_aclcheck() with ACL_CREATE is not only invoked
just before creation of a new table. It is also called when we create
a new function, type, conversion and so on.

For example, pg_namespace_aclcheck() does not take an argument to deliver
the column definitions of new table. When columns are inherited from the
parent table, we have to copy the security context of the parent column,
but we can know the column's definition inside of the pg_namespace_aclcheck().
(It needs to be called after MergeAttributes(), but pg_namespace_aclcheck()
is called before that.)

For example, SE-PgSQL model distinguish "setattr" permission from "drop".
But pg_class_ownercheck() is used for both ALTER and DROP statement.
So, we cannot know which permission should be applied inside from the
pg_class_ownercheck().

For example, ...

At the first commit fest, I was suggested to change definitions of the default
PG access control routines to deliver needed information for both DAC and MAC,
if pg_xxx_aclcheck() is not suitable for SELinux model.
Then, I developed a junk in the result. :(

> I'm not sure how much of a difference that detail makes in the big
> scheme of things, I'm just trying to find ways to make the patch
> minimally invasive..

Basically, I don't think we should change something pg_xxx_aclcheck() and
pg_xxx_ownercheck() routines, because it well implements the default PG model.
If we try to call DAC and MAC from the common entry points, it requires us
many of pain, as we could learn from our hard experience.

What I would like to suggest is to put MAC hook on the strategic points.
The hooks are just invocations of sepgsql_*() functions, so does not need
much of reworks on the core routines.
I believe this is the minimally invasive way.

Linux kernel is one of the best practice. It deploys hooks to call MAC checks
with needed information (such as inode, task_struct, ...) on the strategic
points of the kernel. Basically, DAC and MAC works orthogonally, so it is quite
natural design.

Its specifications are documented in the source code clearly, so folks without
special attentions for security also can know what information should be given
and what result will be returned.
What I would like to suggest is a similar approach. So, now I'm working to write
a documentation from the viewpoint of developer, and coding SE-PgSQL routines
with comments about its specifications.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei escribió:

> There are two cases when we create a new object.
>
> 1) create a new object without any explicit security context.
> If we don't have statement support, it is the only case.
> In this case, SELinux computes a default security context to be assigned
> on the new object. It depends on the client's security context.
> Then, it checks "create" permission on a pair of the client's security
> context and the default security context. If not allowed, an error will
> be raised.

So, following this path, it is possible to write pg_dump support without
a explicit security contexts: you have to make pg_dump write out the
different tuples under different users. So you'd have more than one
data object in the dump output for each table, one for every existing
security context. This seems extremely difficult and intrusive however.

It seems that having explicit security contexts in statements is
necessary for this area to be reasonably simple.

Now, let's assume that COPY data includes the security context for each
tuple in the output. How is that data restored? Would you need to
grant super-SEPostgres privileges to the user restoring the data?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> KaiGai Kohei escribió:
>
>> There are two cases when we create a new object.
>>
>> 1) create a new object without any explicit security context.
>> If we don't have statement support, it is the only case.
>> In this case, SELinux computes a default security context to be assigned
>> on the new object. It depends on the client's security context.
>> Then, it checks "create" permission on a pair of the client's security
>> context and the default security context. If not allowed, an error will
>> be raised.
>
> So, following this path, it is possible to write pg_dump support without
> a explicit security contexts: you have to make pg_dump write out the
> different tuples under different users. So you'd have more than one
> data object in the dump output for each table, one for every existing
> security context. This seems extremely difficult and intrusive however.
>
> It seems that having explicit security contexts in statements is
> necessary for this area to be reasonably simple.

Yes, it may be possible to restore the tables without statement, if we switch
OS-user's privilege for each tables, but unreasonable and unrealistic.

> Now, let's assume that COPY data includes the security context for each
> tuple in the output.

When we support row-level security, it will be necessary to backup and
restore the security context for each tuples.
What I'm talking about is how we specify the security context of the new
tables. If we can have statement support, it will be specified as follows:

CREATE TABLE t ( a int primary key, b text)
SECURITY_CONTEXT = 'system_u:object_r:sepgsql_ro_table_t:unclassified';

> How is that data restored? Would you need to
> grant super-SEPostgres privileges to the user restoring the data?

We need to restore the backup by the user who has privileges to create
database objects dumped at least. But no needs to have super-privilege.

For example, if all the dumped table are labeled as either "unclassified"
or "classified" but not "secret", all the needed privilege is to create
"unclassified" and "classified" tables, not "secret" table.

However, I expect that "unconfined" domain does the backup/restore works
expect for especially secure system. I don't think the default security
policy (such as ones bundled with Fedora) should restrict DBA's privileges
connected from the shell process.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei escribió:
> Alvaro Herrera wrote:

> >Now, let's assume that COPY data includes the security context for each
> >tuple in the output.
>
> When we support row-level security, it will be necessary to backup and
> restore the security context for each tuples.

Oh, right, that part is being left out. Sorry.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support