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

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Could you post any review comments, even if it is not comprehensive yet?

In general, you don't need to preface your comments with 'MEMO:'. I
would encourage removing that. You might use 'FIXME:' instead, if it is
something which needs to be corrected in the future. Additionally,
please think about PG as a whole project. Talking about 'native
privilege mechanism' implies there are 'native' and 'foreign' ones. I
would recommend using the term 'default' instead. Also, rather than
saying it is "not a correct way" when referring to decisions made about
the permissions checks for the 'default' mechanism, just say it's the
default way and that other modules might not implement it the same.
Saying it's "not a correct way" implies a problem with the existing
code. If that's true, then it should be addressed separatly from this
patch.

Specifically, in LookupExplicitNamespace, you added:

---------------------
MEMO: The native privilege mechanism always allows everyone to apply
ACL_USAGE permission on the temporary namespaces implicitly, so it was
omitted to check the obvious one here. But it is a heuristic, not a
correct way.
---------------------

My recommendation for how to reword this, if it's accurate, is:
---------------------
By default, everyone is permitted ACL_USAGE on temporary namespaces
implicitly, so a check for it was omitted here. Other security models
may wish to implement a check, so call ac_schema_search() to check.
---------------------

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

Also, does it make sense to still have LookupCreationNamespace? Given
its charter and the changes, is it still different from
LookupExplicitNamespace?

I would recommend adding back the comments above the calls to
ac_*_grant() which explicitly say:

---------------------
If we found no grant options, consider whether to issue a hard error.
Per spec, having any privilege at all on the object will get you by
here.
---------------------

The issue here is to make it clear to any callers that ac_*_grant() does
not check if everything being attempted will succeed but rather if
any one thing will. Same thing with the comments above the ac_*_grant()
functions themselves.

I'm not sure that the comment in restrict_grant() regarding this is
really necessary, considering that post-change there shouldn't be a
pg_aclmask() anymore, so referring to it in a comment seems odd.

In general, I like what you've done with splitting
restrict_and_check_grant() up.

Regarding your comment in dependency.c about objects which are dropped
due to dependencies:

Have you already developed the code to resolve this issue? Is there
additional information required to check if the object can be dropped?
Should the ac_object_drop() just call the regular drop routines? But
they'd have to have a flag added which indicates if it's a dependency
drop or not, right? If the regular drop routine isn't called, then what
would ac_object_drop() use to determine if the drop is permitted or not?

My concern here is that we don't want to develop a new API and then
immediately discover that it doesn't cover the cases we need it for.
If you have already developed an ac_object_drop() which would work for
existing PG and would be easily changed to support SE, I would recommend
you include it in this patch.

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?

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.

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.

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.

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.

I'm also concerned about the inconsistancy regarding if the roleOid is
passed into the function of if GetUserId() is used. I would recommend
being consistant with this. Either GetUserId() is always 'good enough',
or it's not, and we should require the roleOid to be passed into all of
the ac_* functions. I'm tending towards the latter, since it appears to
be necessary in some cases. If there is some division of the ac_*
functions where it's consistant within a division, that might be
alright, but it should be discussed somewhere.

I've glanced through the rest and in general I feel like it's starting
to look good. Thanks for your efforts towards this. I'd really like to
see this get committed eventually.

Thanks!

Stephen

Stephen, thanks for your comments.

Stephen Frost wrote:
>> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> >> Could you post any review comments, even if it is not comprehensive yet?
>>
>> In general, you don't need to preface your comments with 'MEMO:'. I
>> would encourage removing that. You might use 'FIXME:' instead, if it is
>> something which needs to be corrected in the future. Additionally,
>> please think about PG as a whole project. Talking about 'native
>> privilege mechanism' implies there are 'native' and 'foreign' ones. I
>> would recommend using the term 'default' instead.

OK, indeed, we don't have such a manner something like 'MEMO:' and 'FIXME:'.

I could not find an appropriate name for the current database acl mechanism.
OK, I'll revise the source code comment using the "default".

>> Also, rather than
>> saying it is "not a correct way" when referring to decisions made about
>> the permissions checks for the 'default' mechanism, just say it's the
>> default way and that other modules might not implement it the same.
>> Saying it's "not a correct way" implies a problem with the existing
>> code. If that's true, then it should be addressed separatly from this
>> patch.
>>
>> Specifically, in LookupExplicitNamespace, you added:
>>
>> ---------------------
>> MEMO: The native privilege mechanism always allows everyone to apply
>> ACL_USAGE permission on the temporary namespaces implicitly, so it was
>> omitted to check the obvious one here. But it is a heuristic, not a
>> correct way.
>> ---------------------
>>
>> My recommendation for how to reword this, if it's accurate, is:
>> ---------------------
>> By default, everyone is permitted ACL_USAGE on temporary namespaces
>> implicitly, so a check for it was omitted here. Other security models
>> may wish to implement a check, so call ac_schema_search() to check.
>> ---------------------

OK, indeed, it might be an inappropriate representation.

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

1. A client tries to create a temporary table, the security policy does not
allow him to search his temporary namespace but it can be bypassed due to
the permissive mode.
2. Security administrator toggles it to enforcing mode.
3. The client tries to use the temporary table, but the security policy does
not allow him to search the temporary namespace, and it shall be enforced
in this time.

>> Also, does it make sense to still have LookupCreationNamespace? Given
>> its charter and the changes, is it still different from
>> LookupExplicitNamespace?

These have a bit of difference. The LookupCreationNamespace() can set up
a new temporary namespace, if "pg_temp" is given but the temporary namespace
it not still set up. LookupExplicitNamespace() does not set it up.

>> I would recommend adding back the comments above the calls to
>> ac_*_grant() which explicitly say:
>>
>> ---------------------
>> If we found no grant options, consider whether to issue a hard error.
>> Per spec, having any privilege at all on the object will get you by
>> here.
>> ---------------------
>>
>> The issue here is to make it clear to any callers that ac_*_grant() does
>> not check if everything being attempted will succeed but rather if
>> any one thing will. Same thing with the comments above the ac_*_grant()
>> functions themselves.

OK, I'll add this comments.

>> I'm not sure that the comment in restrict_grant() regarding this is
>> really necessary, considering that post-change there shouldn't be a
>> pg_aclmask() anymore, so referring to it in a comment seems odd.

Hmm, it may not be necessary after the code reviewing. I'll remove it.

>> In general, I like what you've done with splitting
>> restrict_and_check_grant() up.

Thanks,

>> Regarding your comment in dependency.c about objects which are dropped
>> due to dependencies:
>>
>> Have you already developed the code to resolve this issue? Is there
>> additional information required to check if the object can be dropped?
>> Should the ac_object_drop() just call the regular drop routines? But
>> they'd have to have a flag added which indicates if it's a dependency
>> drop or not, right? If the regular drop routine isn't called, then what
>> would ac_object_drop() use to determine if the drop is permitted or not?
>>
>> My concern here is that we don't want to develop a new API and then
>> immediately discover that it doesn't cover the cases we need it for.
>> If you have already developed an ac_object_drop() which would work for
>> existing PG and would be easily changed to support SE, I would recommend
>> you include it in this patch.

At the SE-PgSQL v8.4.x series, I already added checks on deleteOneObject().
http://code.google.com/p/sepgsql/source/browse/branches/pgsql-8.4.x/sepgsql/src/backend/catalog/dependency.c#959

I added a new boolean argument for the function to inform whether the
deletion should be checked, or not. In some cases, it is necessary to
bypass permission checks on cascaded deletion, such as cleaning up
database objects within temporary namespace.

It is not difficult the caller to distinguish whether the given deletion
is cascaded or not.
The findDependentObjects() returns a list of dependency objects.
Any entries to be removed in cascade deletion does not have DEPFLAG_ORIGINAL
flag in the ObjectAddressExtra structure. If the object has the flag, it is
already checked, so we don't need to check permission at the deleteOneObject()
twice.

>> 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.
The one is deployed on regular drop routines, such as ac_relation_drop().
The other is deloped on dependency drop routine, such as ac_object_drop().
If SE-PG feature checks all the drop permission at the ac_object_drop()
(except for objects which does not use dependency deletion), it is not
necessary to distinguish whether the given object is original or cascaded.

>> 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.
It already checks permission when we create a new conversion, and it
is enough for the purpose.

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.

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

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

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

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.

However, either of them are possible.

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

>> 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.
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.
It is only necessary to provide enough information to make decision for
the ac_*() routines.

>> I'm also concerned about the inconsistancy regarding if the roleOid is
>> passed into the function of if GetUserId() is used. I would recommend
>> being consistant with this. Either GetUserId() is always 'good enough',
>> or it's not, and we should require the roleOid to be passed into all of
>> the ac_* functions. I'm tending towards the latter, since it appears to
>> be necessary in some cases. If there is some division of the ac_*
>> functions where it's consistant within a division, that might be
>> alright, but it should be discussed somewhere.

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?

>> I've glanced through the rest and in general I feel like it's starting
>> to look good. Thanks for your efforts towards this. I'd really like to
>> see this get committed eventually.

Thanks for your reviewing. It is great heplful to improve the project.

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

On Tue, Sep 29, 2009 at 6:54 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Could you post any review comments, even if it is not comprehensive yet?
>
> In general, you don't need to preface your comments with 'MEMO:'.  I
> would encourage removing that.  You might use 'FIXME:' instead, if it is
> something which needs to be corrected in the future.  Additionally,
> please think about PG as a whole project.  Talking about 'native
> privilege mechanism' implies there are 'native' and 'foreign' ones.  I
> would recommend using the term 'default' instead.

Or maybe 'standard'?

...Robert

* 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

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>

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

The attached patch eliminates permission checks in FindConversion()
and EnableDisableRule(), because these are nonsense or redundant.

It is an separated issue from the ac_*() routines.
For now, we decided not to touch these stuffs in the access control
reworks patch. So, we can discuss about these fixes as a different
topic.

See the corresponding messages:
http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com

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

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

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

I can understand that, and I think I even said that myself previously.
Things have changed a bit since then though, we're trying to develop a
generalized API. As I said before, I could go either way on this:
Either drop the comment, or add the function call. I'm leaning towards
adding the function call here since it can be done as part of the
generalized API and doesn't add alot of complexity. Removing the
comment is also an option, but that's not my preference.

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

That was part of the reason I was suggesting changing the name to
'depDrop' for 'dependency Drop', that would clear up the confusion about
it being dac or SE, etc.

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

> > 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 would like to use the revised comment.

Feel free to..

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

That would be fine with me.

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

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

Yes, that's what I'm starting to think too, especially now that you've
explained how sepgsql_object_drop() would work. I'd rather it go
through the ac_* API and the sepgsql_proc_drop() be called from there
with the flag than have a separate path.

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

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

Right, I was just suggesting moving it to immediately after the work
that does not have any side-effect. It must be checked before any
system catalog changes are done. Sorry for the confusion.

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

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

Ok.

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

Right.

Thanks,

Stephen

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> The attached patch eliminates permission checks in FindConversion()
> and EnableDisableRule(), because these are nonsense or redundant.
>
> It is an separated issue from the ac_*() routines.
> For now, we decided not to touch these stuffs in the access control
> reworks patch. So, we can discuss about these fixes as a different
> topic.
>
> See the corresponding messages:
> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
> http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com

Thanks. To make sure it gets picked up, you might respond to Tom's
message above with this same email. Just a thought.

Thanks,

Stephen

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>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Yes, it is reasonable both of MAC/DAC to handle temporary schema as
> an exception of access controls on schemas.

Great.

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

What I would suggest is having a README or similar which accompanies the
patch. This could then be included by reference in the commit message,
or directly in the commit depending on what the committer prefers. Or,
it could just go into the mailing list and commitfest archives. The
point is to make sure the committer understands and isn't suprised when
reviewing the changes and comes across places where the code changes
result in a behaviour change.

If the changes are significant enough (and I don't think they will be,
to be honest..), they should be included by the committer in the commit
message and then picked up by Bruce, et al, when the release notes are
developed. I don't believe it needs to be in the formal PG
documentation, unless there's something documented there today which is
changing (very unlikely..).

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

Right, that makes sense to me.

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

No, no. What I was suggesting and what I think we already do in most
places (but not everywhere and it's not really a policy) is this:

CreateXXXX()
{
namespaceId = LookupCreationNamespace();
tablespaceId = get_tablespace_oid();
ac_xxx_create();
:
values[ ... ] = ObjectIdGetDatum(namespaceId);
values[ ... ] = ObjectIdGetDatum(tablespaceId);
simple_heap_insert();
:
}

Which I think is what you're doing with this, it just might be a change
from what was done before when there were multiple permission checks
done.

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

Source code comments would be good for this. The only other place I
could think of it going would be on the developer part of the wiki.

Thanks,

Stephen

Stephen Frost wrote:
>>> 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?
>
> What I would suggest is having a README or similar which accompanies the
> patch. This could then be included by reference in the commit message,
> or directly in the commit depending on what the committer prefers. Or,
> it could just go into the mailing list and commitfest archives. The
> point is to make sure the committer understands and isn't suprised when
> reviewing the changes and comes across places where the code changes
> result in a behaviour change.
>
> If the changes are significant enough (and I don't think they will be,
> to be honest..), they should be included by the committer in the commit
> message and then picked up by Bruce, et al, when the release notes are
> developed. I don't believe it needs to be in the formal PG
> documentation, unless there's something documented there today which is
> changing (very unlikely..).

It may be good idea to put src/backend/security/README.

I'm not clear whether the commit message or release note is appropriate.
(it is unnoticeable if commit message, it is too details for release note.)

> No, no. What I was suggesting and what I think we already do in most
> places (but not everywhere and it's not really a policy) is this:
>
> CreateXXXX()
> {
> namespaceId = LookupCreationNamespace();
> tablespaceId = get_tablespace_oid();
> ac_xxx_create();
> :
> values[ ... ] = ObjectIdGetDatum(namespaceId);
> values[ ... ] = ObjectIdGetDatum(tablespaceId);
> simple_heap_insert();
> :
> }
>
> Which I think is what you're doing with this, it just might be a change
> from what was done before when there were multiple permission checks
> done.

Ahh, it was a communication bug. we talked same thing.

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

Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The attached patch eliminates permission checks in FindConversion()
>> and EnableDisableRule(), because these are nonsense or redundant.
>>
>> It is an separated issue from the ac_*() routines.
>> For now, we decided not to touch these stuffs in the access control
>> reworks patch. So, we can discuss about these fixes as a different
>> topic.
>>
>> See the corresponding messages:
>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>> http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com
>
> Thanks. To make sure it gets picked up, you might respond to Tom's
> message above with this same email. Just a thought.

The following message was my reply.
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php

Now I'm removing something with behavior change such as above patch,
and eliminating comments to be discussed in other thread.
I would like to quote them not to forget them away.

* ACL_CREATE checks on renaming type

When we rename an existing type, it checks ownership of the target
type, but ACL_CREATE on the namespace in which the type is stored,
although it is checked on renaming any other database objects stored
within a certain namespace, such as tables, functions and so on.

Is it an intended behavior?

* Ownership checks on REASSIGN OWNED BY statement

When we use the REASSIGN OWNER BY statement, it tries to change
ownership of the database objects which are owned by the target
role. It internally calls routine to change the ownership such
as AlterFunctionOwner_oid().
The routine depends on the class of objects, and they have a code
to check privilege to change ownership when it is actually changed
as follows:
But the code path corresponding to relations and types does not
have such kind of checks. IMO, similar check should be deployed.

-- at the AlterFunctionOwner_internal()

if (procForm->proowner != newOwnerId)
{
Datum repl_val[Natts_pg_proc];
bool repl_null[Natts_pg_proc];
bool repl_repl[Natts_pg_proc];
Acl *newAcl;
Datum aclDatum;
bool isNull;
HeapTuple newtuple;

/* 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));
}
:

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

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Stephen Frost wrote:
> > Thanks. To make sure it gets picked up, you might respond to Tom's
> > message above with this same email. Just a thought.
>
> The following message was my reply.
> http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php

Right, but now there's actually a patch to go with it.. Just thinking
that Tom might pick up on it more easily if the patch was sent to that
thread, that's all. Of course, he seems to know all anyway, so it's
entirely likely that I'm just being silly.

> Now I'm removing something with behavior change such as above patch,
> and eliminating comments to be discussed in other thread.
> I would like to quote them not to forget them away.

That's fine. You'll start new threads with different subject lines for
them, right? That way other people will see the specific issues and
comment if they want to. I expect few people are actually following
this very long thread at this level. :)

THanks,

Stephen

On Wed, Sep 30, 2009 at 11:17 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Stephen Frost wrote:
>> > Thanks.  To make sure it gets picked up, you might respond to Tom's
>> > message above with this same email.  Just a thought.
>>
>> The following message was my reply.
>>   http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php
>
> Right, but now there's actually a patch to go with it..  Just thinking
> that Tom might pick up on it more easily if the patch was sent to that
> thread, that's all.  Of course, he seems to know all anyway, so it's
> entirely likely that I'm just being silly.
>
>> Now I'm removing something with behavior change such as above patch,
>> and eliminating comments to be discussed in other thread.
>> I would like to quote them not to forget them away.
>
> That's fine.  You'll start new threads with different subject lines for
> them, right?  That way other people will see the specific issues and
> comment if they want to.  I expect few people are actually following
> this very long thread at this level. :)

So what's the status of this patch currently?

...Robert

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> So what's the status of this patch currently?

I'll be reviewing the updates shortly. After that, I'd like a committer
to review it.

Thanks,

Stephen

Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> So what's the status of this patch currently?
>
> I'll be reviewing the updates shortly. After that, I'd like a committer
> to review it.

Do you think this version also should rework an invocation of
pg_namespace_aclcheck() newly added due to the default ACL feature?

IMO, we don't need to hurry-up to catch up these new features just
now, because we have only a week in this commit fest.
It is desirable to improve the patch being commitable earlier.

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