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

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

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.

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

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

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.

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

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

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

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

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

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.

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

> >> My gut feeling is that we should be doing them first and
> >> doing all of them, if at all possible
>
> The 'doing all of them' will contain resolving database object identified
> by its name. It has to be done prior to the security checks.

Right, that's fine and make sense.

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

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

Thanks,

Stephen