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

Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>> Stephen Frost wrote:
>>>> You might also provide a specific example of where and why this check
>>>> matters. I'm not entirely convinced it's necessary or makes sense, to
>>>> be honest..
>> By the default, it is 100% correct to omit checks here.
>>
>> But it can make an issue, when we port SE-PG.
>> SELinux has its working mode (Enforcing or Permissive). On the permissive
>> mode, it also check security policy but does not prevent anything except
>> for generating access violation logs. We can toggle the working mode at the
>> runtime, so it is necessary to consider the following scenario.
>
> 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.

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

>>>> Should the ac_object_drop() just call the regular drop routines?
>> In this patch, ac_xxx_drop() has dacSkip flag to bypass permission
>> checks on cascaded deletion. Perhaps, it might be necessary to put
>> two different ac_ routine to check object deletion.
>
> I'm don't think that's necessary. Having the flag is appropriate, what
> I'm wondering about is why you say in the comment that calling
> ac_object_drop() should be done, but then you don't call it. Even if
> it doesn't do anything in the default case, I think the call should be
> included. The point is that it should be part of the API and
> implemented and ready to go for when SE-PG is added. I don't like the
> idea of having it commented out now, but then uncommenting it when SE-PG
> is added.

The reason why I commented as ac_object_drop() should be done is that
SE-PG model also need to apply checks on cascaded objects, not only
the original one, as you mentioned. And I expected that code only used
in SE-PG will be suggested to remove from the first patch.

The dacSkip flag is a bit different issue.
In some cases, cascaded deletion is not completely equivalent to the
regular deletion.

For example, the default PG model checks ownership of the relation
when we create/drop a trigger object. The PE-PG model also follow
the manner, so it also checks db_table:{setattr} permission.
When we drop a table, it automatically drops triggers which depends
on the table. In this case, if ac_object_drop() calls ac_trigger_drop()
with dacSkip=true, SE-PG checks db_table:{setattr} first, then it also
checks db_table:{drop} later. It is not incorrect, but redundant.

But, it is not a fundamental differences between approaches.
For example, we can skip SE-PG's check on triggers when dacSkip=true.
In this case, the name of variable might be a bit strange.

It is a matter of preference finally.

>>>> I don't find the comment regarding what happened with FindConversion to
>>>> be nearly descriptive enough. Can you elaborate on why the check wasn't
>>>> necessary and has now been removed? If it really isn't needed, why have
>>>> that function at all?
>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>>
>> I'll add a comment about the reason why this check was simply eliminated.
>
> Could these kind of changes be done as a separate patch? Perhaps one
> which should be applied first? It's alot easier to review if we can
> have:
>
> - patches to fix things in PG (such as the above)
> - patch to add in ac_* model

I think we can apply these kind of eliminations earlier or later.
These checks might be redundant or unnecessary, but harmless.
As far as the reworks patch does not touch them, it does not affect
to our discussion.

> This would allow us to much more easily prove to ourselves that the ac_*
> model doesn't break anything. As this is security related code, we
> really need to be comfortable that we're not doing anything to break the
> security of the system.

OK, I'll separate this part.

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

>>>> Regarding OperatorCreate, it looks like there may be some functional
>>>> changes. Specifically, get_other_operator() could now be called even if
>>>> a given user doesn't own the operator shell he's trying to fill out. It
>>>> could then error-out with the "operator cannot be its own negator or
>>>> sort operator" error. I'm not sure that's a big deal at all, but it's a
>>>> difference which we might want to document as expected. This assumes,
>>>> of course, that such a situation could really happen. If I'm missing
>>>> something and that's not possible, let me know.
>> When the get_other_operator() returns valid OID of the negator and
>> commutator, these OIDs are delivered to ac_operator_create(), then
>> ac_operator_create() calls pg_oper_ownercheck() to check ownership
>> on the negator and commutator.
>> I think it keeps compatibility in behavior.
>
> 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.

>>>> The new 'permission' argument to ProcedureCreate should be documented in
>>>> the function definition, or at least where it's used, as to what it's
>>>> for and why it's needed and why ac_proc_create is conditional on it.
>> I'll add the following comment. Is it natural?
>> ----
>> /*
>> * The 'permission' argument should be false, when an internal stuff
>> * tries to define a new obviously safe function. It allows to bypass
>> * security checks to create a new function.
>> * Otherwise, it should be true.
>> */
>> ----
>
> I don't really like referring to 'internal stuff'.. How about:
>
> ----------------
> The 'permission' argument specifies if permissions checking will be
> done. This allows bypassing security checks when they're not necessary,
> such as being called from internal routines where the checks have
> already been done or they're clearly not required.
> In general, this argument should be 'true' to ensure that appropriate
> permissions checks are done.
> ----------------
>
> Something like that.. Of course, it needs to be correct, and you're
> more familiar with that code, so if that comment isn't correct, please
> change it.

I would like to use the revised comment.

>>>> Again, regarding shdepend, if you have the code already which works with
>>>> the default permissions model for PG, you should include it in this
>>>> patch as part of the API. I realize this contradicts a bit what I said
>>>> earlier, but the main concern here is making sure that it will actually
>>>> work for SEPG. If you vouch that it will, then perhaps we don't need to
>>>> add them now, but I would probably also remove the comments then. One
>>>> or the other.. Let's not add comments which will immediately be
>>>> removed, assuming everything goes well.
>> I guess your requirement is to proof the ac_object_drop() is implementable
>> with reasonable changes.
>> Yes, in the default permission model for PG, it does not check anything
>> here. So, if we add ac_object_drop() here, it should be an empty function
>> or it calls ac_xxx_drop() with dacSkip equals true which means do nothing.
>
> I like the latter of those (call ac_xxx_drop() with dacSkip set to
> true). We might even consider changing the name of 'dacSkip' to be
> 'depDrop' or similar. This would indicate that the function is being
> called to drop an object due to a dependency, which is really the only
> case we want to skip permissions checking in the default model (is this
> correct?). That then makes it clear that other models (such as SE-PG)
> can change the behaviour of permissions checking on objects dropped due
> to dependencies.

As I noted above. It is not a significant design issue.

I prefere 'cascade' more than 'depDrop' as the name of flag variable.

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

>> In my current preference, I would like to remove dacSkip flag from the
>> ac_xxx_drop() routines and we don't call it from the ac_object_drop().
>> And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
>
> We could do this instead. Does it duplicate alot of code from the
> ac_xxx_drop() routines though? If not, then I agree with this approach.
> I still feel we should include calling ac_object_drop() in this patch
> since it's clearly a hook we want to include in the API.

The only difference is where sepgsql_xxx_drop() is called from.
If we have the 'dacSkip' (or 'depDrop' or 'cascade') flag,
the sepgsql_proc_drop() will be called from the ac_proc_drop().
Otherwise, ac_object_drop() calls sepgsql_object_drop(), then
it calls sepgsql_proc_drop().

It needs micro adjustment for several object classes, such as
trigger objects, but it is not a fundamental issue.

At first, I follows your preference. (ac_object_drop() commented out
and it calls ac_xxx_drop() with DAC bypassable flag.)

> Again, it boils down to if there is sufficient information to implement
> the controls we want. How about this- if we wanted (in some future PG)
> to give users the option of "check permissions on objects dropped due to
> dependencies" with regular PG, could we do that in ac_object_drop()
> without alot of extra code? Or would we have to go add the dacSkip (my
> depDrop) flag everywhere then? It seems like we could just have that
> option checked in ac_object_drop() and have it set depDrop accordingly
> for the other functions.

If we expect such kind of future enhancement, it may be more flexible
to deliver a flag value.

>>>> There is a similar change in CreateConversionCommand. Again, I don't
>>>> think it's a big deal, but I wonder if we should make a decision about
>>>> if permission checks should be first or last and then be consistant
>>>> about it. My gut feeling is that we should be doing them first and
>>>> doing all of them, if at all possible.. There are a couple of other
>>>> places like this.
>> The default PG model requires two privilege to create a new conversion.
>> The one is ACL_CREATE on the namespace, the other is ACL_EXECUTE on
>> the conversion procedure. The caller (CreateConversionCommand) has to
>> resolve both of objects identified by human readable name, prior to
>> the ac_conversion_create() invocation.
>>
>> 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.

>> The security checks raises an error on access violations, and it aborts
>> whole of the current transaction. So, I don't think it is a big issue
>> whether it should be put at the first or last.
>
> The issue is that, in my view, it's better to get 'permission denied'
> earlier than some other error. I would be frustrated to work out how to
> create a particular object only to then be given a 'permission denied'
> right at the end. Some things have to be done first to make a decision
> about permissions, such as name->OID resolution, but the real 'work' and
> any issues related to that should be dealt with after permissions
> checking.

It is necessary to categolize the work into two cases.
The one is a work without any side-effect. For example, constructing
a memory object, looking up system caches and so on.
The other is a work with updating system catalogs.

I think the security checks can be reordered within the earlier operations,
but should not move to the later of the updating system catalogs, even if
an accee violation error cancels the updates.

>> 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.
In the result, it had to be moved just before simple_heap_insert() in
some cases.

I'd like to discuss issues related to FindConversion() and EnableDisableRules()
in other patch. And, ac_*() routines don't care about them in this stage.

>> The ac_proc_execute() is the only case which the caller has to give
>> roleOid instead of GetUserId(), because the default PG model requires
>> to check ACL_EXECUTE permission on the pair of owner of the aggregate
>> function and trans/final function of the aggregate.
>> In other case, GetUserId() is good enough.
>>
>> Is it really necessary to add roleOid argument for all the hooks?
>
> Ok.. It's just ac_proc_execute(), and really only in certain
> circumstances, right? Most 'regular' usage of ac_proc_execute() still
> uses GetUserId()? Perhaps we could address this by having the argument
> called 'aggOid' instead, and pass 'InvalidOid' when it's not an
> aggregate, and use GetUserId() in that case inside ac_proc_execute().
> We could also change it to be ac_proc_execute() and
> ac_proc_agg_execute(). That might be cleaner.
>
> What do you think?

Hmm, it may be considerable.

The ac_aggregate_execute(Oid aggOid) may be preferable for the naming
convension.
This check should be put on the ExecInitAgg() and ExecInitWindowAgg().
The current window-func code checks permission on the finalfn/transfn
at the initialize_peragg() called from the ExecInitWindowAgg().

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