Re: [PATCH] Reworks for Access Control facilities (r2311)

Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Stephen Frost wrote:
>>> The scenario you outline could happen without SE-PG, couldn't it?
>>> Specifically, if a user makes a connection, creates a temporary table,
>>> and then their rights to create temporary tables are revoked? What
>>> should happen in that instance though? Should they continue to have
>>> access to the tables they've created? Should they be dropped
>>> immediately?
>> The permission to be checked here is ACL_USAGE, not ACL_CREATE_TEMP.
>> So, the default PG model does not prevent to access his temporary
>> tables, even if ACL_CREATE_TEMP is revoked after the creation.
>> In this case, he cannot create new temporay tables any more due to
>> the lack of ACL_CREATE_TEMP. But it does not prevent accesses to the
>> temporary objects because these are already created.
>
> What I'm failing to understand is why SE-PG would be changing this then.
> In general, the pg_temp stuff is a bit of a 'wart', as I understand it,
> and is there mainly to be a place to put temporary tables. The fact
> that it's a schema, which we use in other cases to implement access
> controls, feels more like a side-effect of it being a schema than
> something really necessary.
>
>>> I'm not convinced this case is handled sufficiently.. We at least need
>>> to think about what we want to happen and document it accordingly.
>> It is a special case when pg_namespace_aclmask() is called towards
>> a temporary namespace. It always returns ACL_USAGE bit at least
>> independently from its acl setting. (ACL_CREATE bit depends on the
>> ACL_CREATE_TEMP privilege on the current database.)
>>
>> Please see the pg_namespace_aclmask(). Its source code comment
>> describes it and it always makes its decision to allow to search
>> temporary namespace.
>> I guess it is the reason why pg_namespace_aclcheck() is not called
>> towards the temporary namespace, and it's right for the default PG
>> model.
>>
>> But SE-PG model concerns the assumption.
>
> The temporary namespace is just there to hold temporary tables which
> have been created. It's not intended to be a point of access control.
> I understand that there may be some cases where SE-PG wants to control
> access when the default PG model doesn't, but this feels like a case
> where the PG model doesn't because it's an implementation detail that's
> really intended to be hidden from the user.

It is a good point.
PostgreSQL implements temporary database object feature using a special
schema (pg_temp_*), but it is an implementation details indeed.

As you mentioned, it is a schema in the fact. It may be harmful for
simplicity of the security model to use exception cases.
But it is a perspective from developers, not users.

For example, how many people pay mention that network layer is implemented
as a pseudo filesystem in Linux? You are saying such a thing, correct?

At least, I don't think it is an issue corresponding to security risk.
The reason why SE-PG want to check permission to search a certain schema
including temporary one is a simplicity of access control model.

Yes, it is reasonable both of MAC/DAC to handle temporary schema as
an exception of access controls on schemas.

>>>> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
>>>> because it is equivalent to allow everyone to execute the conversion
>>>> function. It seems to me ownership is more appropriate check.
>>> This is, again, something which should be discussed and dealt with
>>> separately from the ac_* model implementation.
>> Ahn, it is just my opinion when I saw the code.
>> I have no intention to change the behavior in this patch.
>> Is the source code comment also unconfortable?
>
> I could probably go either way on this. In any case, it should be a
> separate discussion in a separate thread, if we really want to discuss
> it.

OK,

>>> My concern is just that there might now be an error message seen be the
>>> user first regarding the negator instead of a permissions error that
>>> would have been seen first before, for the same situation.
>>>
>>> I doubt this is a problem, really, but we should at least bring up any
>>> changes in behaviour like this and get agreement that they're
>>> acceptable, even if it's just the particulars of error-messages (since
>>> they could possibly contain information that shouldn't be available...).
>> I don't think it is a problem.
>> The default PG model (also SE-PG model) does not support to hide existence
>> of a certain database objects, even if client does not have access permissions.
>> For example, a client can SELECT from the pg_class which include relations
>> stored within a certain namespace without ACL_USAGE.
>> The default PG model prevent to resolve the relation name in the schema,
>> but it is different from to hide its existent.
>
> I know it doesn't hide existence of major database objects. Depending
> on the situation, there might be other information that could be leaked.
> I realize that's not the case here, but I still want to catch and
> document any behavioral changes, even if it's clear they shouldn't be an
> issue.

I agree that it should be documented.
Where should I document them on? I guess the purpose of the description
is to inform these behavior changes for users, not only developers.
The official documentation sgml? wiki.postgresql.org? or, source code
comments are enough?

>>> which is really the only
>>> case we want to skip permissions checking in the default model (is this
>>> correct?).
>> If you saying the case is only MAC should be applied but no DAC,
>> it is correct.
>>
>> I think four other cases that both of MAC/DAC should be bypassed.
>> - when temporary database objects are cleaned up on session closing.
>> - when ATRewriteTables() cleans up a temporary relation.
>> - when CLUSTER command cleans up a temporary relation.
>> - when autovacuum found an orphan temp table, and drop it.
>>
>> In this case, ac_object_drop() should not be called anyway,
>> as if ac_proc_create() is not called when caller does not want.
>
> Ok.

Sorry, I missed a point.
The shdepDropOwned() launched using DROP OWNED BY statement is a case that
we have no DAC for each database objects but MAC should be applied on them.
We can consider it as a variety of cascaded deletion, so ac_object_drop()
should be also put here.

>>>> In the current implementation, these permissions are checked on the
>>>> separated timing just after obtaining OID of namespace and procedure.
>>> Checking immediately after resolving OIDs seems fine to me. Clearly,
>>> that has to be done and it's really closer to 'parsing' than actually
>>> doing anything.
>> Please note that what factors are used depends on the security model
>> for the same required action, such as CREATE CONVERSION.
>> If we put ac_*() functions after the name->OID resolution, it breaks
>> the purpose of security abstraction layer.
>>
>> In this example, it makes access control decision to create a new
>> conversion, and raises an error if violated.
>> But the way to make the decision is encapsulated from the caller.
>> A configuration may make decision with the default PG model.
>> An other configuration may make decision with the default PG and SE-PG
>> model. The caller has to provide information to the ac_*() functions,
>> but it should not depend on a certain security model.
>
> I agree that what the caller provides shouldn't depend on the security
> model. I wasn't suggesting that it would. The name->OID resolution I
> was referring to was for things like getting the namespace OID, not
> getting the OID for the new conversion being created. Does that clear
> up the confusion here?

Sorry, confusable description.

What I would like to say is something like:

CreateXXXX()
{
namespaceId = LookupCreationNamespace();
ac_xxx_create_namespace(); --> only one use it, but other doesn't use?
:
tablespaceId = get_tablespace_oid();
ac_xxx_create_tablespace(); --> only DAC use it?
:
ac_xxx_create(); --> only MAC use it?
:
values[ ... ] = ObjectIdGetDatum(namespaceId);
values[ ... ] = ObjectIdGetDatum(tablespaceId);
simple_heap_insert();
:
}

When we create a new object X which needs OID of namespace and tablespace,
these names have to be resolved prior to updating system catalogs.
If we put ac_*() routines for each name resolution, it implicitly assumes
a certain security model and the ac_*() routine getting nonsense.

>>>> It is only necessary to provide enough information to make decision for
>>>> the ac_*() routines.
>>> Right.. Can we be consistant about having the permissions check done as
>>> soon as we have enough information to call the ac_*() routines then? I
>>> believe that's true in most cases, but not all, today. Any of those
>>> changes which change behaviour should be discussed in some way (such as
>>> an email to -hackers as you did for FindConversion()).
>> At least, the current implementation deploys ac_*() routines as soon as
>> the caller have enough information to provide it.
>
> Good. We should document where that changed behaviour though, but also
> document that this is a policy that we're going to try and stick with in
> the future (running ac_*() as soon as there is enough information to).

This policy focuses on developers, so it is enough to be source code comments?

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