Re: [PATCH] SE-PgSQL/lite rev.2163

Robert,

The sepgsql/avc.c provides a facility to cache access control decisions
in userspace, and enables to reduce time of kernel invocations.
However, its size is the largest one in the SE-PgSQL patch.

[kaigai(at)saba gram]$ wc -l src/backend/security/sepgsql/avc.c
829 src/backend/security/sepgsql/avc.c

I think separation of the avc facility contributes to keep code
size smaller, rather than pg_security mechanism.

[kaigai(at)saba gram]$ wc -l src/backend/catalog/pg_security.c
381 src/backend/catalog/pg_security.c

I guess it enables to reduce 20% of patch scale. (800L/4KL)

I can also add more source code comments, such as when the security
hooks to be invoked, what is the purpose and so on, instead of ruduced
lines. It will make clear where the hooks to be placed.

Thanks,

KaiGai Kohei wrote:
> Robert Haas wrote:
>> 2009/7/12 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> Robert, thanks for your comments.
>>>
>>> Robert Haas wrote:
>>>> 2009/7/10 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>> The SE-PostgreSQL patches are updated as follows:
>>>>>
>>>>> [1/5] http://sepgsql.googlecode.com/files/sepgsql-01-sysatt-8.5devel-r2163.patch
>>>>> [2/5] http://sepgsql.googlecode.com/files/sepgsql-02-core-8.5devel-r2163.patch
>>>>> [3/5] http://sepgsql.googlecode.com/files/sepgsql-03-gram-8.5devel-r2163.patch
>>>>> [4/5] http://sepgsql.googlecode.com/files/sepgsql-04-tests-8.5devel-r2163.patch
>>>>> [5/5] http://sepgsql.googlecode.com/files/sepgsql-05-docs-8.5devel-r2163.patch
>>>>>
>>>>> List of updates:
>>>>> * Patch set was organized to a few ones which provides only core features.
>>>>> * Code base was upgraded to the latest CVS HEAD.
>>>>> * Some of features in the fullset edition were separated, to focus on
>>>>> the core feature of SE-PostgreSQL at the first commit fest.
>>>> I took a look at this a little bit. It looks as if you are still
>>>> treating the Security Label as a special attribute of the tuple, which
>>>> seems completely unnecessary given that this patch set is not
>>>> attempting to implement row-level security. It seems to me that all
>>>> you should need is regular old columns pg_class.relseclabel,
>>>> pg_attribute.attseclabel, etc; it also seems to me that would simplify
>>>> the code.
>>> Are you saying that whole of the pg_security mechanism also should be
>>> postponed to the second commit fest, not only system column's definitions?
>> I'm not sure what you mean by "the whole of the pg_security
>> mechanism". But I believe there was negative feedback previously
>> about the idea of sandwhiching the security label into the tuple
>> header instead of making it an ordinary (non-system) column in a
>> system catalog table.
>
> It means the mechanism to translate a security identifier and its
> text representation each other.
> The reason why I separated the pg_security mechanism was to reduce
> scale of the patch, because we agreed to postpone row-level stuffs
> in the v8.4 development cycle. Please note that only four relations
> needed to have a capability to assign security labels (pg_database,
> pg_class, pg_attribute and pg_proc).
> However, when row-level security is available, consumption of the
> storage by security labels in raw-text cannot be ignored, although
> massive number of database objects shares limited number of security
> labels.
>
>>>> As a general comment, I don't have much confidence that your original
>>>> design for row-level security is a good one. I think that needs to be
>>>> thought about very carefully before anything is implemented. I note
>>>> that your original design called for a system catalog lookup on each
>>>> row returned, which is basically saying that all security-label
>>>> filtering will be implemented as a nested loop with inner index scan.
>>>> It seems to me that if we ever implement row-level security (which is
>>>> far from a sure thing) we're certainly going to want to allow the
>>>> planner to make other decisions, such as sorting the tuples by
>>>> security label and performing a merge join against the pg_security
>>>> catalog, or hashing the pg_security catalog and performing a hash
>>>> join, or using an index on the security label column to perform a
>>>> bitmap index scan, or any other technique that the planner already
>>>> knows how to consider. I say all this not to encourage you to spend
>>>> more time on row-level security now (because I think that would be
>>>> premature) but to encourage you to abandon all the design decisions in
>>>> this patch that are presuming a particular design for row-level
>>>> security and focus on making the features that are actually in this
>>>> patch set as lean and robust as possible.
>>> I'm confusing a bit for the comments.
>>> The row-level access control stuff (which is managed in my repository
>>> now) does not need to look up the pg_security system catalog every time.
>>> I guess you believe SE-PgSQL looks up the system catalog to fetch security
>>> label in text form, and calls in-kernel SELinux to make its decision for
>>> every tuples. However, it is incorrect.
>>>
>>> The userspace avc (security/sepgsql/avc.c) routines enable to cache access
>>> control decisions recently used, and return its result for the required
>>> pair of security identifier (not a text form) and type of actions (like
>>> db_table:{select}), if it hits a cache entries.
>>> It means SE-PgSQL can return its decisions using only security identifier
>>> in most cases.
>> Perhaps you're not making a kernel call for each row (good!), but I
>> think you are still assuming that you're going to fetch the rows
>> first, without any sepgsql-specific decision making, and then perform
>> an operation of some kind on each row to see whether to filter it. If
>> so, what I'm saying is that's bad.
>>
>> Suppose we have two security contexts A and B, and two users Alice and
>> Bob. Alice can see only tuples in security context A, and Bob can see
>> only tuples in security context B. If I create an index on the
>> security ID of a table with row-level security, (a) then will it work?
>> and (b) if one of the users issues a command like "select * for
>> table", will the planner consider a bitmap-index-scan using that index
>> rather than a sequential scan of the entire table?
>
> In the current implementation, the index should be used, if user refers
> the "security_label" system column as a part of query, such as:
>
> SELECT * FROM t1 WHERE security_label = 'A';
>
> In this case, tuples are fetched with index scan based on text comparison
> at first, then SE-PgSQL checks the security label of the fetched tuple
> to determine whether it should be filtered, or not.
> It is hooked at the ExecScan(), and performs as if a volatile function
> is appended at WHERE condition.
>
> However, the (current) planner does not apply indexes for the (b) case.
> If the SeqScan is selected, SE-PgSQL also applies its controls on the
> fetched tuples.
>
>>> I think what you suggested is useful, if SE-PgSQL needs security label
>>> in text form on making its decision. However, it seems to me your comments
>>> bases on incorrect assumption. Could you point to me, if I incorrectly
>>> understood the intentions of your comments.
>>>
>>>> Another problem that I have with this patch set is that it STILL
>>>> doesn't have parity with the DAC permissions scheme (despite previous
>>>> requests to make it have parity). For example, you're checking
>>>> privileges like db_column:{drop}, which have no analogue in our
>>>> current permissions scheme. I think this has been discussed several
>>>> times before, and it seems that you still haven't chosen to fully take
>>>> that advice, which I suspect is going to be an absolute bar to getting
>>>> this committed. (I am not a committer, of course, so take whatever I
>>>> say with a grain of salt, but that's my opinion for what it's worth.)
>>>> It seems to me that if you have REAL parity with the existing
>>>> permissions scheme, it shouldn't be necessary to add additional,
>>>> separate sepgsql checks in every place currently has a standard
>>>> permissions check. Instead, you should be able to put all of the
>>>> logic into functions like pg_class_aclmask(). This will be:
>>> I moved several security hooks to the pg_xxx_aclmask() because these
>>> permissions to be checked in same places.
>>> However, both of security models also have different definitions.
>>> For example, when we create a new table, dac checks ACL_CREATE on
>>> the namespace (it may be equivalent to db_schema:{add_object}),
>>> but MAC checks db_table:{create} permission on the new table itself
>>> labeled with default or user given one.
>> So why do it differently? I don't see why you have to invent a whole
>> new paradigm here.
>
> The paradigm is not my inventment. It just follows the security model
> in SELinux. It assigns its security label on the managed object (such
> as files, sockets, processes, ipc objects and so on) on their creation
> time, and checks user's privileges to create it labeled.
>
>>From viewpoint of the system-wide consistency in access controls,
> we should not ignore SELinux's manner which provides security policy.
>
> The filesystem DAC and MAC mechanism is a good analogy.
> When we create a file under the directoly, the kernel checks "w" permission
> of the directoly. In addition, SELinux checks dir:{add_name} permission on
> the directoly and file:{create} permission on the newly created file with
> a certain security label.
> A part of security hooks are called from the DAC check routines, but rest
> of them are called from other strategic points, due to the differences
> in security models.
>
> I never oppose to move security hooks into pg_xxx_aclcheck() routines
> as far as we can define one-to-oen mapping between DAC and MAC.
> However, in some cases, we cannot implement the security model correctly.
>
> Thanks,
>
>>> For the security hooks we can move to pg_xxx_aclmask(), I can agree
>>> to move them as possible as we can, such as sepgsqlCheclProcedureExecute()
>>> invoked from pg_proc_aclmask(). But, I concluded rest of security hooks
>>> are hard to integrate with pg_xxxx_aclmask() mechanism.
>> ...Robert
>

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