Re: security hook on table creation

On Wed, Sep 29, 2010 at 9:59 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/09/29 22:00), Robert Haas wrote:
>>
>> On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>  wrote:
>>>
>>> I don't think it is an option to move the hook after the pollution
>>> of system catalogs, although we can pull out any information about
>>> the new relation from syscache.
>>
>> Why not?
>>
> All the existing security checks applies before modifying system catalogs.
>
> At least, I cannot find out any constructive reason why we try to apply
> permission checks on object creation time with different manner towards
> the existing privilege mechanism...

The reason would be so that you can apply security labels if you so
desire. If you choose to throw an error instead, transaction abort
will clean everything up. We could have two hooks - one earlier when
we're checking DAC permisisons, and a second one later to apply
labels, but I don't see that there's enough of a gain from that to be
worth the additional complexity. It's still simpler than your
proposed design, though.

>> So what?  The patch you submitted doesn't provide the OID of the new
>> schema when someone does ALTER TABLE SET SCHEMA, either.  I proposed a
>> design which was much more general than what you submitted, and you're
>> now complaining that it's not general enough.  It's unrealistic to
>> think you're going to solve every problem with one patch.
>
> Sorry, I never said one patch with enough generic hook solves everything.
>
> By contraries, I think the proposed prototype of the hook cannot inform
> the plugins anything except for OID and event type, even if necessary.

That is true. But ISTM that it will handle a remarkably large number
of cases very well. We could of course do more later, either by
adding additional hooks or by adding capabilities to this one.
However, you'd first need to make a convincing argument that those
capabilities are important.

> Some of permission checks needs its specific prototype to inform extra
> information rather than OIDs; such as new label in SECURITY LABEL hook,
> new schema in upcoming ALTER TABLE SET SCHEMA, and so on...
>
> Of course, we can implement some of permission checks with OID of the
> target object and event type collectly. E,g. I cannot image any extra
> information to check permission on COMMENT. I never deny it.

Why not? If you're going to prohibit another plugin from relabeling
an object based on the provider and label, why not allow or disallow
comments based on the content of the comment? A general problem with
your designs from the very beginning is that they involve the enhanced
security provider needing to know about absolutely everything that
goes on in core, and visca versa. That's unmaintainable and we're not
doing it.

Incidentally, wanting to know the label that some other security
provider might try to assign to an object is a crystal-clear example
of moving the goal-posts. You had a hook for that (which I removed)
in the security label patch, and it didn't provide the label anyway.
How can it be a requirement now if it wasn't two weeks ago? You need
to stay focused on coming up with simple, easy-to-understand hooks
that ideally have use case cases that are broader than security, but
at least that are broadly applicable to security rather than very
narrowly tailored to extremely specific things which you want to do.

I think that the remit of this patch should be to add hooks for CREATE
and DROP to every single object type in the system that are generic
and can be used for any purpose, whether security related or
otherwise, with room for extension to additional operations in future
patches.

>> Moreover,
>> it's far from obvious that you actually do need the details that
>> you're proposing anyway.  Are you really going to write an SE-Linux
>> policy that allows people to change the schema of table A to schema B
>> but not schema C?  Or that allows a hypothetical smack plugin to label
>> a given object with one label but not another?  And if so, are those
>> absolutely must-have features for the first version or are those
>> things that would be nice to have in version 3 or 4?
>
> In your proposition, prototype of the security hook has four arguments:
> ObjectType, oid, subid and ObjectAccessType, doesn't it?

Yes.

> When user tries to change the schema of table from A to B, we can know
> the current schema of the table using syscache, but we need to inform
> the plugin that B is the new schema, because we have no way to pull out
> what schema was provided by the user.
>
> As LookupCreationNamespace() checks CREATE permission on the new schema,
> SELinux also want to check permission on the new schema, not only old one.
> So, I concerned about the prototype does not inform about new schema that
> user provided using ALTER TABLE SET SCHEMA statement.

You're not answering my question. Are you going to write an SE-Linux
policy that allows table A to be moved to schema B but not to schema
C? And if so, is that an essential feature for the first version or
something that can be added later?

My understanding from the conversation at BWPUG is that this is not
something that Josh Brindle and David Quigley are concerned about.
Hooks on object creation are important for type transitions, so that
you can automatically assign labels rather than forcing users to apply
them by hand; the fact that we can also the entire CREATE operation to
get bounced from the same hook is a bonus. But with that exception,
they seemed to think that coarse-grained permissions would be fine for
a basic implementation: perhaps even just install something in
ProcessUtility_hook and bounce DDL across the board, so long as it's
controlled by reference to the security policy rather than by DAC. I
think we can do better than that in a pretty short period of time if
we avoid getting side-tracked, but the key is that we have to avoid
getting side-tracked.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company