Re: [v9.2] Object access hooks with arguments support (v1)

On Wed, Oct 19, 2011 at 6:18 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/10/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> In the example table creation, heap_create_with_catalog() is invoked
>>> by 5 routines, however, 3 of them are just internal usages, so it is not
>>> preferable to apply permission checks on table creation....
>>
>> Some wit once made the remark that if a function has 10 arguments, it
>> has 11 arguments, meaning that functions with very large numbers of
>> arguments typically indicate a poor choice of abstraction boundary.
>> That's pretty much what I think is going on with
>> heap_create_with_catalog().  I think maybe some refactoring is in
>> order there, though I am not sure quite what.
>>
> Sorry, are you complained about the number of arguments in
> heap_create_with_catalog? or hooks of security checks?

I'm just saying that heap_create_with_catalog() is big and complex and
does a lot of different things. The fact it does security checks even
though it's also sometimes called as an internal function strikes me
as a modularity violation. Normally I would expect to have a
high-level routine (which is invoked more or less directly from SQL)
that does security checks and then calls a low-level routine (that
actually does the work) if they pass. Other parts of the code that
want to perform the same operation without the security checks can
call the low-level function directly. But that's not the way it's set
up here.

> Yes, I never say SELECT INTO without DAC checks cause actual
> security hole, because owner can change its access permissions by
> himself, unlike MAC.
> Please suppose that there are two security labels: confidential table
> (TC) and public table (PT). Typical MAC policy (including selinux)
> does not allow users who can read from tables with TC to write out
> data into tables with PT, because PT is readable for public as literal,
> so confidential data may be leaked to public domain in the result.
>
> It is a significant characteristic of MAC; called as data-flow-control.
> So, it damages significant part of its worth, if MAC could not distinct
> CREATE TABLE from SELECT INTO in PostgreSQL, and it is the
> reason why I strongly requires a flag of contextual information here.
>
> Although you say it is not possible to maintain, doesn't the above
> introduction help us to understand nothing why MAC needs to
> distinct SELECT INTO from CREATE TABLE?, and why it needs
> a flag to distinct two cases?

Sure. But what is going to happen when someone needs to modify that
code in a year or two? In order to understand what to do with the
object access hook, they're going to need to understand all those
details you just mentioned. And those details do not exist in the
patch as submitted. Worse, let's suppose we'd committed a patch like
the one you have here before foreign tables went in. Then, whoever
added foreign tables would have needed to know to add the foreign
table server name to the object access hook invocation, because
apparently sepgsql needs that. How would they know they needed to do
that? I've committed practically all of the sepgsql-related patches,
and I would not have known that, so it seems likely to me that other
people adding new functionality to the server wouldn't know it either.

When someone comes along in another year or two and adds materialized
views, will they need to pass some additional data to the object
access hook? Probably, but I bet you're the only one who can quickly
figure out what it is. That's no good. We're not going to make
changes to PostgreSQL core that only you can maintain, and that are
likely to be broken by future commits. At this point I feel pretty
good that someone can look at the stuff that we've done so far with
SECURITY LABEL and the object access hooks, and if they add a new
object type, they can make those things apply to the new object type
too by copying what's already there, without making any reference to
the sepgsql code. There's a clear abstraction boundary, and people
who are working on one side of the boundary do not need to understand
the details of what happens on the other side of the boundary.

In this particular case, I think it might be reasonable to change the
DAC behavior, so that a CREATE TABLE AS SELECT or SELECT INTO requires
insert privileges on the new table as well as permission to create it
in the first place. I don't particularly see any reason to require
different privileges for CREATE TABLE followed by INSERT .. SELECT
than what we require when the two commands are rolled into one. Prior
to 9.0, this case couldn't arise, because we didn't have default
privileges, so I'm inclined to think that the way it works now is a
historical accident rather than a deliberate design decision.

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