Re: Reworks for Access Control facilities (r2363)

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>