SE-PgSQL patch review

Started by ITAGAKI Takahiroover 16 years ago162 messageshackers
Jump to latest
#1ITAGAKI Takahiro
itagaki.takahiro@oss.ntt.co.jp

I'm reviewing SE-PgSQL patch and have some comments.
https://commitfest.postgresql.org/action/patch_view?id=212

Forgive me, but I'm a novice of SELinux. Some of the comments
and question might be nonsense.

==== Patch overview ====
The patch itself is large (>13K), but more than half of it are
well-written regression test, comments and README.

It adds object-level and column-level security checks after normal
permission checks. Row-level checks are not included in the patch.

==== Syntax and identifier names ====
* Can we use "security context" only for securty checking but also
for general "context" handling? If so, we can call it just "context".
It might be useful if the context is designed for more general purpose.
For example, we could develop context-based logging or contex-aware
setting parameters. I think "Application Name" patch is one of the
implementations of context-based logging.
https://commitfest.postgresql.org/action/patch_view?id=195

* CREATE TABLE tbl (...) SECURITY_CONTEXT = '...'
IMHO, '_' and '=' don't suit for SQL.
If there are no conflicts in gram.y, how about removing them?
CREATE TABLE tbl (...) SECURITY CONTEXT '...'

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

* Since SE-PgSQL will be integrated into the core, we can use pg_*
names for the feature. For example, we can rename sepgsql_getcon() to
pg_get_security_context(). Also, we should not use 'con' as an
abbreviated form of 'context' because we often use it for 'connection'.
The same can be said for GUC parameter names.
(ex: "sepostgresql" to be "security_policy")

==== Error messages and error codes ====
* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).
/* Class SE - SE-PostgreSQL Error (SE-PgSQL-specific error class) */
#define ERRCODE_SELINUX_INTERNAL_ERROR MAKE_SQLSTATE('S','E', '0','0','1')
#define ERRCODE_INVALID_SECURITY_CONTEXT MAKE_SQLSTATE('S','E', '0','0','2')
#define ERRCODE_SELINUX_AUDIT_LOG MAKE_SQLSTATE('S','E', '0','0','3')

* Can we use error messages that looks like existing privilege errors?
ERRCODE_INSUFFICIENT_PRIVILEGE:
=> permission denied to rename database
Error messages from SE-PgSQL
=> SE-PgSQL prevents to modify "pg_class" by hand
looks very different. I'd suggest something like
=> security context denied to rename database

I'll suggest we avoid using the term "SE-PgSQL" in the user visible
message and documentation because because the feature will be integrated
into the core deeply. Of course, we can use "SE-PgSQL" in *promotion*.

==== Internal structures ====
* Are the security labels enough stable?
What will we need when SELinux configuration is modified?
We store security labels as text for each object and column.

* Each security checks are called after pg_*_aclcheck(). Is it possible to
merge SE-PgSQL checks into ACL functions in aclchk.c ?

* What is difference between sepgsql_database_getcon(oid) and
pg_database.datsecon? Why do we need those getcon functions?

That's all comments for now. I'll test the patch in actual machines next.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

#2KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: ITAGAKI Takahiro (#1)
Re: SE-PgSQL patch review

Itagaki Takahiro wrote:

I'm reviewing SE-PgSQL patch and have some comments.
https://commitfest.postgresql.org/action/patch_view?id=212

Forgive me, but I'm a novice of SELinux. Some of the comments
and question might be nonsense.

Itagaki-san, thanks for your volunteering so much!

==== Patch overview ====
The patch itself is large (>13K), but more than half of it are
well-written regression test, comments and README.

It adds object-level and column-level security checks after normal
permission checks. Row-level checks are not included in the patch.

Yes, I separated most of features to reduce code size.
The developer documentation and source code comments were added instead.

==== Syntax and identifier names ====
* Can we use "security context" only for securty checking but also
for general "context" handling? If so, we can call it just "context".
It might be useful if the context is designed for more general purpose.
For example, we could develop context-based logging or contex-aware
setting parameters. I think "Application Name" patch is one of the
implementations of context-based logging.
https://commitfest.postgresql.org/action/patch_view?id=195

The "security context" is a proper-noun in SELinux.
I don't think it is a good idea to call it just a "context".

If we have a set of properties of the client called as a context,
security context is one of the property in a context, such as user
identifier, application name and so on.

If Log_line_prefix supports the security context of the client,
it seems me a good idea. (However, we can implement it in the future.)

BTW, SELinux also provides indications about what should be logged
on access violation events. It is also an duty of userspace object
manager. See the sepgsql_audit_log() in selinux.c.

* CREATE TABLE tbl (...) SECURITY_CONTEXT = '...'
IMHO, '_' and '=' don't suit for SQL.
If there are no conflicts in gram.y, how about removing them?
CREATE TABLE tbl (...) SECURITY CONTEXT '...'

I tried it, and it seems to me fine.

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

* Since SE-PgSQL will be integrated into the core, we can use pg_*
names for the feature. For example, we can rename sepgsql_getcon() to
pg_get_security_context(). Also, we should not use 'con' as an
abbreviated form of 'context' because we often use it for 'connection'.
The same can be said for GUC parameter names.

What is your opinion for '*_secon()', instead of '*_security_context'?
If OK, these functions will be renamed as follows:

- sepgsql_template1_getcon -> pg_get_template1_secon
- sepgsql_default_getcon -> pg_get_default_secon
- sepgsql_getcon -> pg_get_client_secon
- sepgsql_database_getcon -> pg_get_database_secon
- sepgsql_schema_getcon -> pg_get_schema_secon
- sepgsql_relation_getcon -> pg_get_relation_secon
- sepgsql_attribute_getcon -> pg_get_attribute_secon

(ex: "sepostgresql" to be "security_policy")

I prefer "selinux_support" more than "security_policy", because
it is a noun to mean a set of access control rules in the kernel.

==== Error messages and error codes ====
* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).
/* Class SE - SE-PostgreSQL Error (SE-PgSQL-specific error class) */
#define ERRCODE_SELINUX_INTERNAL_ERROR MAKE_SQLSTATE('S','E', '0','0','1')
#define ERRCODE_INVALID_SECURITY_CONTEXT MAKE_SQLSTATE('S','E', '0','0','2')
#define ERRCODE_SELINUX_AUDIT_LOG MAKE_SQLSTATE('S','E', '0','0','3')

I already uses predefined error code, if exist.
For example, sepgsql_compute_perms() is the routine to make access control
decision in SELinux. It uses ERRCODE_INSUFFICIENT_PRIVILEGE, when it needs
to raise an error.

extern bool
sepgsql_compute_perms(char *scontext, char *tcontext,
uint16 tclass, uint32 required,
const char *audit_name, bool abort)
{
:
/*
* It asks SELinux whether the given access should be allowed,
* or not. It set the given avd structure correctly, then returns.
*/
compute_perms_internal(scontext, tcontext, tclass, &avd);
:
/*
* If here is no policy violations, or SE-PgSQL performs in permissive
* mode, or the client process peforms in permissive domain, it returns
* normally with 'true'.
*/
if (!denied ||
!sepgsql_get_enforce() ||
(avd.flags & SELINUX_AVD_FLAGS_PERMISSIVE) != 0)
return true;
:
/*
* Otherwise, it raises an error or returns 'false', depending on the
* caller's indication by 'abort'.
*/
if (abort)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("SELinux: security policy violation")));

return false;
}

These error codes has its own meanings.
* ERRCODE_SELINUX_INTERNAL_ERROR
It is an error due to the communication between PostgreSQL and SELinux.
It seems similar to ERRCODE_INTERNAL_ERROR, but it is not "can't-happen"
conditions, so I defined it individually.

* ERRCODE_INVALID_SECURITY_CONTEXT
It is used when client gives an invalid security context on CREATE or
ALTER statement. I could not find any similar predefined error code.

* ERRCODE_SELINUX_AUDIT_LOG
It is used when SE-PgSQL generates access violation logs, not an error.
I also could not find any similar predefined error code.

* Can we use error messages that looks like existing privilege errors?
ERRCODE_INSUFFICIENT_PRIVILEGE:
=> permission denied to rename database

Here is a point that we should provide users a hint which enables
to make clear the reason of access violations. So, I think we should
contains a mark to show it come from SE-PgSQL.

Currently, it raises an error on access violation in sepgsql_compute_perms().
It always prints out "SELinux: security policy violation".

Error messages from SE-PgSQL
=> SE-PgSQL prevents to modify "pg_class" by hand
looks very different. I'd suggest something like
=> security context denied to rename database

If we follow the existing manner, it may be:
"SELinux: permission denied: \"%s\" is a system catalog"

I'll suggest we avoid using the term "SE-PgSQL" in the user visible
message and documentation because because the feature will be integrated
into the core deeply. Of course, we can use "SE-PgSQL" in *promotion*.

If we use another name for the feature, "SELinux support" may be preferable?

==== Internal structures ====
* Are the security labels enough stable?
What will we need when SELinux configuration is modified?
We store security labels as text for each object and column.

If the security labels get invalid due to the modification of SELinux
configuration or other reasons, it considers the database objects are
unlabeled.

Whenever we fetch a text representation of security label from system
catalog, it invokes security_check_context_raw() to check whether the
security label is valid on the current configuration, or not.

If invalid, it obtains an "unlabeled" security context from the system.

For example,

bool
sepgsql_database_common(Oid datOid, uint32 required, bool abort)
{
:
/*
* (1) It fetches a security context from pg_database.datsecon
* as a text data.
*/
tuple = SearchSysCache(DATABASEOID,
ObjectIdGetDatum(datOid),
0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for database %u", datOid);

datsecon = SysCacheGetAttr(DATABASEOID, tuple,
Anum_pg_database_datsecon, &isnull);
if (!isnull)
context = TextDatumGetCString(datsecon);
/*
* (2) If the fetched security context is not a valid one,
* we obtain system's "unlabeled" security context as a fallback.
* - security_check_context_raw() applies validation checks
* - sepgsql_get_unlabeled_context() returns "unlabeled" context.
* Typically, it is "system_u:object_r:unlabeled_t:s0"
*/
if (!context || security_check_context_raw(context) < 0)
context = sepgsql_get_unlabeled_context();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/*
* (3) We call permission check routine on a couple of security
* contexts (client and the target database).
* It may be a valid security context, or "unlabeld" security
* context. In either case, we can provide a valid security context.
*/
audit_name = NameStr(((Form_pg_database) GETSTRUCT(tuple))->datname);
rc = sepgsql_compute_perms(sepgsql_get_client_context(),
context,
SEPG_CLASS_DB_DATABASE,
required,
audit_name, abort);
ReleaseSysCache(tuple);

return rc;
}

* Each security checks are called after pg_*_aclcheck(). Is it possible to
merge SE-PgSQL checks into ACL functions in aclchk.c ?

It is impossible due to some reasons.

For example, some of entrypoints are enclosed by "if (superuser())".
It implicitly allows any clients with superuser attribute to bypass MAC
checks, but it is quite violates SE-PgSQL's design, if we put the hooks
in the PG default model routines.

For example, SE-PgSQL model distinguish "setattr" permission from "drop".
But pg_class_ownercheck() is used for both ALTER and DROP statement.
So, we cannot know which permission should be applied inside from the
pg_class_ownercheck().

For example, ...

We already tried to provide a common set of entrypoints for both of DAC
and MAC at the last commit fest ("Reworks for Access Controls" patch).
This abstraction layer had to provide enough information to make access
control decisions for both of models, and be deployed on the strategic
points for both models. But it needed massive amount of reworks in the
core routines, so we decided this approach is not a right direction.

* What is difference between sepgsql_database_getcon(oid) and
pg_database.datsecon? Why do we need those getcon functions?

SELinux allows to translate the last field of security context between
a human readable form and a raw form.

For example, if a table is labeled as follows:
system_u:object_r:sepgsql_table_t:s0:c1
^^^^^
We call this field as "range" or "mcs label". In the raw-format, it is
a complex of symbols, but we can assign human meaningfull alias on them.
If "s0:c1" has its alias ("1st_sales_division"), above security context
shall be printed as:
system_u:object_r:sepgsql_table_t:1st_sales_division
^^^^^^^^^^^^^^^^^^
This feature is called "mcstrans".
It is necessary to port a definition of security context to other hosts
which may not have same mapping between a human-readable and raw format.

Thus, I used sepgsql_xxxx_getcon() function in the pg_dump to follows
the manner in SELinux when we export/import security context of objects
from/to the object manager (PostgreSQL).

I'll revise the following things at first:
- Replace SE-PgSQL from user visible error messages and documentations.
- SECURITY_CONTEXT [=] '<label>' to be SECURITY CONTEXT '<label>'

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#3KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: KaiGai Kohei (#2)
Re: SE-PgSQL patch review

* Can we use error messages that looks like existing privilege errors?
ERRCODE_INSUFFICIENT_PRIVILEGE:
=> permission denied to rename database

Here is a point that we should provide users a hint which enables
to make clear the reason of access violations. So, I think we should
contains a mark to show it come from SE-PgSQL.

Currently, it raises an error on access violation in sepgsql_compute_perms().
It always prints out "SELinux: security policy violation".

It is just for your information.

SELinux allows end-users to confirm what accesses are violated using audit logs.
In operating system, it is written on the /var/log/audit/audit.log.

See the result of:
% grep ^type=AVC /var/log/audit/audit.log
:
type=AVC msg=audit(1258412043.107:81): avc: denied { search } for pid=1750 comm="httpd" name="root" dev=sda5 ino=1062881 scontext=system_u:system_r:httpd_t:s0 tcontext=system_u:object_r:admin_home_t:s0 tclass=dir
:

In SE-PgSQL, it writes out detailed information about access violations
on log files. Then, it can be used to revise security policy.

For example:
postgres=# UPDATE t1 SET a = 1;
ERROR: SELinux: security policy violation

It shall be logged as follows:
LOG: SELinux: denied { update } scontext=unconfined_u:unconfined_r:sepgsql_test_t:Unclassified tcontext=system_u:object_r:sepgsql_ro_table_t:Classified tclass=db_table name=t1
STATEMENT: UPDATE t1 SET a = 1;
ERROR: SELinux: security policy violation
STATEMENT: UPDATE t1 SET a = 1;

We can also provide these logs to analyzer programs to pop-up a hint for
trouble-shooting (setroubleshoot), to generate policy module automatically
from access violation logs (audit2allow), and so on.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#4Ross J. Reedstrom
reedstrm@rice.edu
In reply to: KaiGai Kohei (#2)
Re: SE-PgSQL patch review

On Tue, Nov 24, 2009 at 03:12:43PM +0900, KaiGai Kohei wrote:

Itagaki Takahiro wrote:

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

Does "WITH" work? Seems to read better to me:

CREATE TABLE tbl (col integer WITH SECURITY CONTEXT [...])

--
Ross Reedstrom, Ph.D. reedstrm@rice.edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
The Connexions Project http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE

#5KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Ross J. Reedstrom (#4)
Re: SE-PgSQL patch review

Ross J. Reedstrom wrote:

On Tue, Nov 24, 2009 at 03:12:43PM +0900, KaiGai Kohei wrote:

Itagaki Takahiro wrote:

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

Does "WITH" work? Seems to read better to me:

CREATE TABLE tbl (col integer WITH SECURITY CONTEXT [...])

It was conflicted. :(

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

#6KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: KaiGai Kohei (#5)
Re: SE-PgSQL patch review

KaiGai Kohei wrote:

Ross J. Reedstrom wrote:

On Tue, Nov 24, 2009 at 03:12:43PM +0900, KaiGai Kohei wrote:

Itagaki Takahiro wrote:

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

We need to put a reserved token, such as "AS", prior to the
SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

Does "WITH" work? Seems to read better to me:

CREATE TABLE tbl (col integer WITH SECURITY CONTEXT [...])

It was conflicted. :(

BTW, we have two options, if we don't use AS token here.

1. It moves "SECURITY" to reserved keyword.
We can represent SECURITY CONTEXT option for each columns quite
natural, but it also has a pain. It disallow to use "security"
as a column name.

2. Another syntax to support SECURITY CONTEXT
For example:
CREATE TABLE tbl_name (
col_X_name int primary key,
col_Y_name text default 'aaa'
) SECURITY CONTEXT ( 'label of the table',
col_Y_name IS 'label of the column Y', ... );

I don't have any preference between the current syntax and the new one.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#7ITAGAKI Takahiro
itagaki.takahiro@oss.ntt.co.jp
In reply to: KaiGai Kohei (#2)
Re: SE-PgSQL patch review

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

CREATE TABLE tbl (...) SECURITY CONTEXT '...'
* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

There might be another idea to put security context in WITH options:
1. CREATE TABLE tbl (...) WITH (security_context = '...')
2. CREATE TABLE tbl (col integer WITH (security_context = '...') PRIMARY KEY)
If we use the syntax, '_' and '=' is reasonable.

BTW, I like to reverse the order of constraints and WITH options in
column definitions (2), but I could not solve shift/reduce errors
-- it might conflict with "PRIMARY KEY WITH (index-parameters)".

- sepgsql_template1_getcon -> pg_get_template1_secon
- sepgsql_database_getcon -> pg_get_database_secon

Why do we need two version of functions for template1 and database?
The template1 database is the default template for CREATE DATABASE,
but we can also choose another one. Do we need to distinguish them?

* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).

I already uses predefined error code, if exist.

What I meant was: there are no problem to add new error codes for SE-PgSQL,
but I think the values of the codes should be '42xxx' because those errors
are still "Class 42 - Access Rule Violation" from the view of users.

==== Internal structures ====
* Are the security labels enough stable?
We store security labels as text for each object and column.

If the security labels get invalid due to the modification of SELinux
configuration or other reasons, it considers the database objects are
unlabeled.

I believe you have a plan to add row-level security checking in the future
version. Do you have some documentation about how to implement security
context for each row? I'm worrying about the on-disk representation.
Security labels stored in text format takes 20-40 bytes per row. It is not
negligibly-small, and might be hard to be treated because of variable-length.

We store OIDs for each row at the end of tuple header. If we also
store securty labels in the same way, will we need some kinds of
"securty label to OID" converter in the future?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

#8KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: ITAGAKI Takahiro (#7)
Re: SE-PgSQL patch review

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

CREATE TABLE tbl (...) SECURITY CONTEXT '...'
* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

There might be another idea to put security context in WITH options:
1. CREATE TABLE tbl (...) WITH (security_context = '...')
2. CREATE TABLE tbl (col integer WITH (security_context = '...') PRIMARY KEY)
If we use the syntax, '_' and '=' is reasonable.

BTW, I like to reverse the order of constraints and WITH options in
column definitions (2), but I could not solve shift/reduce errors
-- it might conflict with "PRIMARY KEY WITH (index-parameters)".

If we put "SECURITY CONTEXT" clause prior to the column constraints,
there are no syntax conflicts. However, it seems to me not intuitive.

like, CREATE TABLE tbl (col int SECURITY CONTEXT '...' NOT NULL);

In addition, if we inject "security_context" in the relation options,
the way to fetch it is far different from other database objects.

Instead, what is your opinion for the syntax?

CREATE TABLE tbl (...) SECURITY CONTEXT ('label', col='label', ...);

When "col=" is omitted, it means an explicit security context of the
new table. Otherwise, it means an explicit one of the given column.

And, for consistency,

CREATE DATABASE dbname SECURITY CONTEXT ('label');
CREATE SCHEMA scname SECURITY CONTEXT ('label');

- sepgsql_template1_getcon -> pg_get_template1_secon
- sepgsql_database_getcon -> pg_get_database_secon

Why do we need two version of functions for template1 and database?
The template1 database is the default template for CREATE DATABASE,
but we can also choose another one. Do we need to distinguish them?

They have different purposes.

The sepgsql_database_getcon() prints out a security context of the
database for the given OID in human-readable form.

The sepgsql_template1_getcon() returns a security context to be
assigned on the initial database from SELinux configuration.
Typically, it is configured at /etc/selinux/targeted/contexts/sepgsql_contexts.
If not exist, it asks SELinux a default security context as an initial
database.
Then, initdb uses the result to assign initial security context of the
managed database objects.

* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).

I already uses predefined error code, if exist.

What I meant was: there are no problem to add new error codes for SE-PgSQL,
but I think the values of the codes should be '42xxx' because those errors
are still "Class 42 - Access Rule Violation" from the view of users.

Ahh, OK. I'll fix it.

==== Internal structures ====
* Are the security labels enough stable?
We store security labels as text for each object and column.

If the security labels get invalid due to the modification of SELinux
configuration or other reasons, it considers the database objects are
unlabeled.

I believe you have a plan to add row-level security checking in the future
version. Do you have some documentation about how to implement security
context for each row? I'm worrying about the on-disk representation.
Security labels stored in text format takes 20-40 bytes per row. It is not
negligibly-small, and might be hard to be treated because of variable-length.

We store OIDs for each row at the end of tuple header. If we also
store securty labels in the same way, will we need some kinds of
"securty label to OID" converter in the future?

Yes, it was contained in the earlier proposition with full-set functionalities.

http://wiki.postgresql.org/wiki/SEPostgreSQL_Architecture#Interaction_between_pg_security_system_catalog

In SELinux model, massive number of objects shares a limited number of
security context (e.g more than 100 tables may have a same one), this
design (it stores "security label OID" within the tuple header) is well
suitable for database objects.

BTW, I plan the following steps for the row-level security.
| * A facility to put "security label OID" within the tuple header.
| * System column support to print out the security context.
| (This system column shall be writable to relabel)
| * Pure-SQL row-level security checks, something like Oracle Private
| Database which allows user defined access control decision function.
| * SELinux aware row-level checks on the virtual private database stuff.
V It can be implemented as one of the decision making functions.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#9KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: KaiGai Kohei (#8)
Re: SE-PgSQL patch review

* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).

I already uses predefined error code, if exist.

What I meant was: there are no problem to add new error codes for SE-PgSQL,
but I think the values of the codes should be '42xxx' because those errors
are still "Class 42 - Access Rule Violation" from the view of users.

Ahh, OK. I'll fix it.

I also think ERRCODE_INVALID_SECURITY_CONTEXT is suitable for the Access
Rule Violation class ('44xxx').

However, it seems to me ERRCODE_SELINUX_INTERNAL_ERROR should be moved
to the System Error class ('58xxx'), because it will be raised due to
the problem on communicating with SELinux, not access violations.

And, we may be able to remove ERRCODE_SELINUX_AUDIT_LOG, because audit
logs are generated on access violation events (in most case, if security
policy is right), so ERRCODE_INSUFFICIENT_PRIVILEGE might be suitable
to call ereport(LOG, ...) with an audit log message.

Isn't it strange in manner?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#10ITAGAKI Takahiro
itagaki.takahiro@oss.ntt.co.jp
In reply to: KaiGai Kohei (#8)
Re: SE-PgSQL patch review

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

==== Internal structures ====

http://wiki.postgresql.org/wiki/SEPostgreSQL_Architecture#Interaction_between_pg_security_system_catalog

In SELinux model, massive number of objects shares a limited number of
security context (e.g more than 100 tables may have a same one), this
design (it stores "security label OID" within the tuple header) is well
suitable for database objects.

What plan do you have for system columns added by the patch
(datsecon, nspsecon, relsecon, etc) after we have securty_id
system column? Will we have duplicated features then?

Even if system tables don't use security_id columns, should the data type
of *secon be oid instead of text? I think pg_security described in the wiki
page is useful even if we only have object-level security.
How about adding pg_security and changing the type of *secon to oid?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

#11KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: ITAGAKI Takahiro (#10)
Re: SE-PgSQL patch review

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

==== Internal structures ====

http://wiki.postgresql.org/wiki/SEPostgreSQL_Architecture#Interaction_between_pg_security_system_catalog

In SELinux model, massive number of objects shares a limited number of
security context (e.g more than 100 tables may have a same one), this
design (it stores "security label OID" within the tuple header) is well
suitable for database objects.

What plan do you have for system columns added by the patch
(datsecon, nspsecon, relsecon, etc) after we have securty_id
system column? Will we have duplicated features then?

In my plan, these fields will be removed when we add security system
column support.

Even if system tables don't use security_id columns, should the data type
of *secon be oid instead of text? I think pg_security described in the wiki
page is useful even if we only have object-level security.
How about adding pg_security and changing the type of *secon to oid?

The reason why the current version stores security context in plain
text is to minimize the scale of changeset as I have been pointed out
many times for a long time. :(
The pg_security catalog support requires about additional 1KL to the
current patch. It seems to me it goes against to the previous suggestions.

-- keep it smaller, and step-by-step enhancement

BTW, If you don't have any complaints about new syntax in CREATE TABLE
statement, I'll revise the patch soon.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

#12Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#8)
Re: SE-PgSQL patch review

2009/11/24 KaiGai Kohei <kaigai@ak.jp.nec.com>:

BTW, I plan the following steps for the row-level security.
| * A facility to put "security label OID" within the tuple header.
| * System column support to print out the security context.
|   (This system column shall be writable to relabel)
| * Pure-SQL row-level security checks, something like Oracle Private
|   Database which allows user defined access control decision function.
| * SELinux aware row-level checks on the virtual private database stuff.
V   It can be implemented as one of the decision making functions.

I think we're getting ahead of ourselves talking about row-level
security at this point, but FWIW I have a lot of concerns about how
the previous version of this feature was designed. In particular, I
think we should set up row-level security in a way that (1) allows it
to be used for purposes other than SE-Linux and (2) allows
row-filtering to take advantage of indices. If I have a table with a
million rows, but only rights to see 100 of them, the system
administrator should be able to define an index that will allow the
100 I can see to be fetched via a bitmap-index scan rather than a
seq-scan with a probe for every row.

...Robert

#13ITAGAKI Takahiro
itagaki.takahiro@oss.ntt.co.jp
In reply to: KaiGai Kohei (#11)
Re: SE-PgSQL patch review

KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:

-- keep it smaller, and step-by-step enhancement

I'd prefer "smaller concept" rather than "smaller patch".

I think the philosophy of SE-PgSQL itself is ok,
but there are some issues in the design and implementation:

==== No interaction with existing features ====
* SE-PgSQL injects security-context-based access control, but there are
no interaction between it and the existing role-based access control.

* SE-PgSQL introduces concept of "security context", but there are
no interaction between it and the existing context-related features.
(ex. pg_hba.conf and Application name patch)

This is just an idea, but how about implementing context-based access
control based on role-based ACL? We will not use security context directly
for access control, but it forbid to use ROLEs in some conditions.
An example of implementation image is:

=# ALTER ROLE role VALID ON SECURITY CONTEXT '...'

For example, this could allow us to modify rows only with a particular
application, only from particular machine, and only in particular hour.
Since we've already supported object- and column-level ACL, I think
we can the same capability of the patch using security-context-to-role
mapper. Or, is it not ideal in the policy of SELinux?

==== Postgres is not prepared to receive SE-PgSQL ====
We depend on superuser too heavily. As KaiGai-san mentioned, we use
"if (superuser())" instead of ACL in some places. It is a bad manner.
We should centralize access control in one module (maybe aclcheck.c),
and SE-PgSQL should be implemented only in the module.

If possible, it might be good for SE-PgSQL to replace all of the
role-based access control layer in postgres because it is hard for
users to maintain both Postgres ROLEs and SELinux settings consistently.
Do we need pluggable ACL layer before accepting SE-PgSQL?

==== Need to reconsider row-level security control ====
Row-level security control is excluded from the patch, but we'd better
also considering it at first. You mentioned as:

In SELinux model, massive number of objects shares a limited number of
security context (e.g more than 100 tables may have a same one)

but I'm not sure what application do you suppose. For example,
if we protect web application from SQL injection attacks, the
password column for each row can be read only from the end user
represented by the row. The number of security labels will be same
as the number of end users (= rows).

==== Actual benefits of SE-PgSQL ====
SE-PgSQL will be committed step-by-step -- but could you explain which step
can solve which problem in the real world? Imagine that SQL injections,
measure for SOX Act, divulgation of personal information, .... They are
security holes in terms of a whole application, but not a hole in terms of
database, because database cannot always distinguish between legal and
illegal data access (ex. correction of wrong data vs. rigging of benefits).

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

#14KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: ITAGAKI Takahiro (#13)
Re: SE-PgSQL patch review

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:

-- keep it smaller, and step-by-step enhancement

I'd prefer "smaller concept" rather than "smaller patch".

Its difference is unclear for me.

In this CF, I separated most of separatable concepts to reduce
size of the patch, as follows:
- No access controls except for databases, schemas, tables and columns
- No row-level access control granurality
- No security OID and text translation
- No access control decision cache

Needless to say, these feature can be added later, step-by-step.
But the core of SE-PgSQL is access control based on the SELinux policy.
It is an atomic feature, so unseparatable.

==== No interaction with existing features ====
* SE-PgSQL injects security-context-based access control, but there are
no interaction between it and the existing role-based access control.

If the interaction means restriction of available pairs between
a certain database role and a certain security context of the
client, it may be available as I followed on the next section.

However, SELinux accept only security context as an identifiers
of processes, files, tables and so on. If you mention to consider
database role identifier in access control decision, it is not
possible.

* SE-PgSQL introduces concept of "security context", but there are
no interaction between it and the existing context-related features.
(ex. pg_hba.conf and Application name patch)

This is just an idea, but how about implementing context-based access
control based on role-based ACL? We will not use security context directly
for access control, but it forbid to use ROLEs in some conditions.
An example of implementation image is:

=# ALTER ROLE role VALID ON SECURITY CONTEXT '...'

For example, this could allow us to modify rows only with a particular
application, only from particular machine, and only in particular hour.
Since we've already supported object- and column-level ACL, I think
we can the same capability of the patch using security-context-to-role
mapper. Or, is it not ideal in the policy of SELinux?

Basically, it is not bad idea to restrict available database roles by
the security context of the client.
However, we need to revise the concept a bit.

Please remind its principle. SE-PgSQL applies all the its access controls
according to the security policy of SELinux.
SELinux defines all the access control rules as a relationship between
a couple of security contexts.

So, this idea can be rewritten as follows:

1. add a security context to pg_authid system column
2. add db_role object class in the security policy
(It needs discussion in the SELinux community)
3. It checks db_role:{loginas} permission between the client and
the security context of the db_authid entry.

For example, if the security policy allows "system_u:system_r:httpd_t:s0"
(a typical web server) domain to login as a database role labeled as
"system_u:object_r:web_role_t:s0", we can assign this context on a certain
database role to be performs as web users.

ALTER ROLE role SECURITY CONTEXT ( 'system_u:object_r:web_role_t:s0' );

Please note that I basically agree to implement this relationshipt,
but it can be implemented as an incremental patch, as long as the
core feature is merged first.

==== Postgres is not prepared to receive SE-PgSQL ====
We depend on superuser too heavily. As KaiGai-san mentioned, we use
"if (superuser())" instead of ACL in some places. It is a bad manner.
We should centralize access control in one module (maybe aclcheck.c),
and SE-PgSQL should be implemented only in the module.

If possible, it might be good for SE-PgSQL to replace all of the
role-based access control layer in postgres because it is hard for
users to maintain both Postgres ROLEs and SELinux settings consistently.
Do we need pluggable ACL layer before accepting SE-PgSQL?

I already tried this approach in the commit fest#2.
But it was a big failure. :(

I don't think it is a right direction.

In addition, former cases (SELinux, X-window, ...) does not integrate
its default access control stuff and the optional access controls.

==== Need to reconsider row-level security control ====
Row-level security control is excluded from the patch, but we'd better
also considering it at first. You mentioned as:

In SELinux model, massive number of objects shares a limited number of
security context (e.g more than 100 tables may have a same one)

but I'm not sure what application do you suppose. For example,
if we protect web application from SQL injection attacks, the
password column for each row can be read only from the end user
represented by the row. The number of security labels will be same
as the number of end users (= rows).

I need to admit the current proposal is a "lite" version, so some of
functionality may be lack.

For example, see the page.24 of this slids:
http://sepgsql.googlecode.com/files/JLS2009-KaiGai-LAPP_SELinux.pdf

It assigns a certain security context on web-applications for each
virtual host. In this example, a web-application once labeled as "blue"
can never access both of filesystem objects and database objects (such
as tables) labeled as other colors. SELinux performs as a logical wall
to separate individual domains.
Nowadays, we can often see such a multi-tenant system.

I'd like you to understand that I separated most of separatable features
as a result of the previous discussions, such as row-level granurality,
access control decision cache, translater between security OID and text
and so on.

==== Actual benefits of SE-PgSQL ====
SE-PgSQL will be committed step-by-step -- but could you explain which step
can solve which problem in the real world? Imagine that SQL injections,
measure for SOX Act, divulgation of personal information, .... They are
security holes in terms of a whole application, but not a hole in terms of
database, because database cannot always distinguish between legal and
illegal data access (ex. correction of wrong data vs. rigging of benefits).

Good question. The hardness to fix all the security holes in application
is the original motivation of reference monitor model (it is a conceptual
design in 80's security reserch. SELinux and other MAC system is on the
genealogy).

Please imagine the reason why SELinux applies its access control policy
on the accesses to system resources via system-calls.

If we have to fix all the application bugs, it looks like a endless-war.
The number of strategic points to be checked are infinite in fact.
However, all the application has to use system-calls to access system
resources such as files, networks, etc. It means we can aquire any
accesses without exceptions with hooks in operating system, even if
violated accesses are not bugs in operating system.

The operating system also cannot distinguish whether the given access
is actually a licit one, or not. But it prevent violated access in the
outline.
For example, when a user tries to write a file, SELinux cannot know
whether the contents is right or not. But if a "unclassified" user
tries to write something on a "classified" file, SELinux will prevent
this request independent from the correctness of the contents.

Please note that SELinux/SE-PgSQL never deny existing access control
model. They have their own purpose and whorth, we never deny it.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#15Martijn van Oosterhout
kleptog@svana.org
In reply to: ITAGAKI Takahiro (#13)
Re: SE-PgSQL patch review

On Thu, Nov 26, 2009 at 11:15:46AM +0900, Itagaki Takahiro wrote:

==== No interaction with existing features ====
* SE-PgSQL injects security-context-based access control, but there are
no interaction between it and the existing role-based access control.

And there shouldn't be, I think. SE-PgSQL is MAC which means that what
someone can access is configured elsewhere. This example is
non-sensical:

=# ALTER ROLE role VALID ON SECURITY CONTEXT '...'

When someone logs in they have a security context and what they can
access is then decided. You can't reconfigure what someone has access
to with anything within the DB (other than label changes), the SE-PgSQL
rules are elsewhere. That's what "Mandatory" refers to.

but I'm not sure what application do you suppose. For example,
if we protect web application from SQL injection attacks, the
password column for each row can be read only from the end user
represented by the row. The number of security labels will be same
as the number of end users (= rows).

This example is also strange: the program that needs to read the
password need to be able to see all rows because by definition the user
cannot be logged in yet. After you login there is no need to be able to
read your own password. So column-access control is fine here.

And even if there are lots of contexts, so what? Security is not free,
but given SE-PgSQL is in use in the real world, clearly people think
the tradeoffs are worth it.

Finally, this is not about protection against SQL injection, it's protection
against people in Sales reading data belonging to Finance.

==== Actual benefits of SE-PgSQL ====
SE-PgSQL will be committed step-by-step -- but could you explain which step
can solve which problem in the real world? Imagine that SQL injections,
measure for SOX Act, divulgation of personal information, .... They are
security holes in terms of a whole application, but not a hole in terms of
database, because database cannot always distinguish between legal and
illegal data access (ex. correction of wrong data vs. rigging of benefits).

As far as I can tell, just about all the interesting cases are for
row-level security. While MAC on tables and columns will be
interesting, my guess the real uptake will be when row-level control is
in.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Show quoted text

Please line up in a tree and maintain the heap invariant while
boarding. Thank you for flying nlogn airlines.

#16KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: ITAGAKI Takahiro (#13)
Re: SE-PgSQL patch review

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:

-- keep it smaller, and step-by-step enhancement

I'd prefer "smaller concept" rather than "smaller patch".

For the last a few days, I've talked with Itagaki-san off list to make clear
where is the point of his suggestion.

In summary, it was similar approach with what I already proposed in the CF#2,
but rejected.

During the first commit-fest of v8.5 development cycle, Stephen Frost suggested
to rework the default PG's access controls to host other optional security
features, not only the default one.
Then, I submitted a large patch titled as "Reworks for Access Controls",
but it contained 3.5KL of changeset on the core routines, and 4KL of new codes
into "src/backend/security/*" except for documentations and testcases.
Then, this approach was rejected (not "returned with feedback") due to the scale
and complexity.

After the fest, we discussed the direction to implement SE-PgSQL.
Basically, it needs to keep the changeset small, and the rest of features (such
as row-level granurality, access control decision cache, ...) shoule be added
step-by-step consistently, according to the suggestion in the v8.4 development
cycle. Heikki Linnakangas also suggested we need developer documentation which
introduces SE-PgSQL compliant permission checks and specification of security
hooks, after the reworks are rejected.

So, I boldly removed most of the features from SE-PgSQL except for its core
functionalities, and added developer documentation (README) and widespread
source code comments to introduce the implementations instead.
In the result, the current proposal is near to naked one.
- No access controls except for database, schema, table and column.
- No row-level granularity in access controls.
- No access control decision chache.
- No security OID within HeapTupleHeader.

I believe the current patch is designed according to the past suggestions.
However, his suggestion seems to me backing to the rejected approach again.

I've been torn between the past suggestions and his suggestion.
So, I asked him to get off reviewing the patch, because of the deadlock in
the development process. At the current moment, I'd like to respect
suggestions in the past discussions more.

Thanks for paying your efforts in spite of differences in opinions.
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#17Bruce Momjian
bruce@momjian.us
In reply to: KaiGai Kohei (#16)
Re: SE-PgSQL patch review

KaiGai Kohei wrote:

In summary, it was similar approach with what I already proposed in the CF#2,
but rejected.

During the first commit-fest of v8.5 development cycle, Stephen Frost suggested
to rework the default PG's access controls to host other optional security
features, not only the default one.
Then, I submitted a large patch titled as "Reworks for Access Controls",
but it contained 3.5KL of changeset on the core routines, and 4KL of new codes
into "src/backend/security/*" except for documentations and testcases.
Then, this approach was rejected (not "returned with feedback") due to the scale
and complexity.

After the fest, we discussed the direction to implement SE-PgSQL.
Basically, it needs to keep the changeset small, and the rest of features (such
as row-level granurality, access control decision cache, ...) shoule be added
step-by-step consistently, according to the suggestion in the v8.4 development
cycle. Heikki Linnakangas also suggested we need developer documentation which
introduces SE-PgSQL compliant permission checks and specification of security
hooks, after the reworks are rejected.

So, I boldly removed most of the features from SE-PgSQL except for its core
functionalities, and added developer documentation (README) and widespread
source code comments to introduce the implementations instead.
In the result, the current proposal is near to naked one.
- No access controls except for database, schema, table and column.
- No row-level granularity in access controls.
- No access control decision chache.
- No security OID within HeapTupleHeader.

I believe the current patch is designed according to the past suggestions.

Agreed. The patch is exactly what I was hoping to see:

o only covers existing Postgres ACLs
o has both user and developer documentation
o includes regression tests
o main code impact is minimal

Now, if this is applied, we might then move forward with implementing
SE-Linux specific features like mandatory access control (MAC) and
row-level security.

In terms of review, the patch is 13k lines, but most of that is
documentation, se-linux-specific files, system catalog adjustments, and
regression tests.

Also, I attended KaiGai's talk in Tokyo where he explained that managing
permission at the operating system level, the web server level (via
.htaccess and htpasswd), and at the database level is confusing, and
having a single permission system has benefits.

The number of revisions and adjustments KaiGai has done since the
original SE-PostgreSQL patch is amazing and certainly gives me
confidence that he will be around to help in case there are any problems
in the future.

So, one big problem is that no one has agreed to review it, partly or
probably because few developers understand the SE-Linux API, and many
people who have used SE-Linux have been confused by it.

I think I could review this if I could team up with someone to help me,
ideally someone on instant message (IM) and perhaps using SE-Linux.

I think the big question is whether this feature (mappming SE-Linux
permissions to existing Postgres permissions) has an acceptable code
impact. Of course we might be adding things later, but at this stage is
this something we can apply?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

#18David Fetter
david@fetter.org
In reply to: Bruce Momjian (#17)
Re: SE-PgSQL patch review

On Mon, Nov 30, 2009 at 11:03:08PM -0500, Bruce Momjian wrote:

KaiGai Kohei wrote:

In summary, it was similar approach with what I already proposed
in the CF#2, but rejected.

During the first commit-fest of v8.5 development cycle, Stephen
Frost suggested to rework the default PG's access controls to host
other optional security features, not only the default one. Then,
I submitted a large patch titled as "Reworks for Access Controls",
but it contained 3.5KL of changeset on the core routines, and 4KL
of new codes into "src/backend/security/*" except for
documentations and testcases. Then, this approach was rejected
(not "returned with feedback") due to the scale and complexity.

After the fest, we discussed the direction to implement SE-PgSQL.
Basically, it needs to keep the changeset small, and the rest of
features (such as row-level granurality, access control decision
cache, ...) shoule be added step-by-step consistently, according
to the suggestion in the v8.4 development cycle. Heikki
Linnakangas also suggested we need developer documentation which
introduces SE-PgSQL compliant permission checks and specification
of security hooks, after the reworks are rejected.

So, I boldly removed most of the features from SE-PgSQL except for its core
functionalities, and added developer documentation (README) and widespread
source code comments to introduce the implementations instead.
In the result, the current proposal is near to naked one.
- No access controls except for database, schema, table and column.
- No row-level granularity in access controls.
- No access control decision chache.
- No security OID within HeapTupleHeader.

I believe the current patch is designed according to the past suggestions.

Agreed. The patch is exactly what I was hoping to see:

o only covers existing Postgres ACLs
o has both user and developer documentation
o includes regression tests
o main code impact is minimal

This patch addresses points 1-3 of Andrew Sullivan's post here:
http://archives.postgresql.org/pgsql-hackers/2008-10/msg00388.php

Left out is point 4, namely whether it's possible to map metadata
access controls can do this completely, and if so, whether this patch
implements such a mapping.

This is totally separate from the really important question of whether
SE-Linux has a future, and another about whether, if SE-Linux has a
future, PostgreSQL needs to go there.

All that aside, there is an excellent economic reason why a
proprietary product might need to follow a dead-end technology, namely
increasing shareholder value due to one or more large, long-term
deals.

PostgreSQL doesn't have this motive, although some of the proprietary
forks may well.

Can we see about Andrew Sullivan's point 4? Or is it more important
to address the "do we want to" question first?

Whatever order we take them in, we do need to address both.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#19KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Bruce Momjian (#17)
Re: SE-PgSQL patch review

Bruce Momjian wrote:

KaiGai Kohei wrote:

In summary, it was similar approach with what I already proposed in the CF#2,
but rejected.

During the first commit-fest of v8.5 development cycle, Stephen Frost suggested
to rework the default PG's access controls to host other optional security
features, not only the default one.
Then, I submitted a large patch titled as "Reworks for Access Controls",
but it contained 3.5KL of changeset on the core routines, and 4KL of new codes
into "src/backend/security/*" except for documentations and testcases.
Then, this approach was rejected (not "returned with feedback") due to the scale
and complexity.

After the fest, we discussed the direction to implement SE-PgSQL.
Basically, it needs to keep the changeset small, and the rest of features (such
as row-level granurality, access control decision cache, ...) shoule be added
step-by-step consistently, according to the suggestion in the v8.4 development
cycle. Heikki Linnakangas also suggested we need developer documentation which
introduces SE-PgSQL compliant permission checks and specification of security
hooks, after the reworks are rejected.

So, I boldly removed most of the features from SE-PgSQL except for its core
functionalities, and added developer documentation (README) and widespread
source code comments to introduce the implementations instead.
In the result, the current proposal is near to naked one.
- No access controls except for database, schema, table and column.
- No row-level granularity in access controls.
- No access control decision chache.
- No security OID within HeapTupleHeader.

I believe the current patch is designed according to the past suggestions.

Agreed. The patch is exactly what I was hoping to see:

o only covers existing Postgres ACLs
o has both user and developer documentation
o includes regression tests
o main code impact is minimal

Now, if this is applied, we might then move forward with implementing
SE-Linux specific features like mandatory access control (MAC) and
row-level security.

In terms of review, the patch is 13k lines, but most of that is
documentation, se-linux-specific files, system catalog adjustments, and
regression tests.

Also, I attended KaiGai's talk in Tokyo where he explained that managing
permission at the operating system level, the web server level (via
.htaccess and htpasswd), and at the database level is confusing, and
having a single permission system has benefits.

The number of revisions and adjustments KaiGai has done since the
original SE-PostgreSQL patch is amazing and certainly gives me
confidence that he will be around to help in case there are any problems
in the future.

So, one big problem is that no one has agreed to review it, partly or
probably because few developers understand the SE-Linux API, and many
people who have used SE-Linux have been confused by it.

I think I could review this if I could team up with someone to help me,
ideally someone on instant message (IM) and perhaps using SE-Linux.

I think some of SELinux developers (including me) can help you to review
the code. They are maintaining both of the kernel and libraries (APIs).
I'll call for them to help reviewing.

BTW, I can use skype messanger in my home, and IRC may be available in office.

I think the big question is whether this feature (mappming SE-Linux
permissions to existing Postgres permissions) has an acceptable code
impact. Of course we might be adding things later, but at this stage is
this something we can apply?

It needs to deploy a set of hooks on the strategic points of the core
PostgreSQL codes, and rest of steps (such as computing a default security
context and making an access control decision) are done in the SE-PgSQL
specific files.
These hooks are designed not to prevent anything when SE-PgSQL is disabled.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#20KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: David Fetter (#18)
Re: SE-PgSQL patch review

David Fetter wrote:

On Mon, Nov 30, 2009 at 11:03:08PM -0500, Bruce Momjian wrote:

KaiGai Kohei wrote:

In summary, it was similar approach with what I already proposed
in the CF#2, but rejected.

During the first commit-fest of v8.5 development cycle, Stephen
Frost suggested to rework the default PG's access controls to host
other optional security features, not only the default one. Then,
I submitted a large patch titled as "Reworks for Access Controls",
but it contained 3.5KL of changeset on the core routines, and 4KL
of new codes into "src/backend/security/*" except for
documentations and testcases. Then, this approach was rejected
(not "returned with feedback") due to the scale and complexity.

After the fest, we discussed the direction to implement SE-PgSQL.
Basically, it needs to keep the changeset small, and the rest of
features (such as row-level granurality, access control decision
cache, ...) shoule be added step-by-step consistently, according
to the suggestion in the v8.4 development cycle. Heikki
Linnakangas also suggested we need developer documentation which
introduces SE-PgSQL compliant permission checks and specification
of security hooks, after the reworks are rejected.

So, I boldly removed most of the features from SE-PgSQL except for its core
functionalities, and added developer documentation (README) and widespread
source code comments to introduce the implementations instead.
In the result, the current proposal is near to naked one.
- No access controls except for database, schema, table and column.
- No row-level granularity in access controls.
- No access control decision chache.
- No security OID within HeapTupleHeader.

I believe the current patch is designed according to the past suggestions.

Agreed. The patch is exactly what I was hoping to see:

o only covers existing Postgres ACLs
o has both user and developer documentation
o includes regression tests
o main code impact is minimal

This patch addresses points 1-3 of Andrew Sullivan's post here:
http://archives.postgresql.org/pgsql-hackers/2008-10/msg00388.php

Left out is point 4, namely whether it's possible to map metadata
access controls can do this completely, and if so, whether this patch
implements such a mapping.

I'm not clear what means the metadata level access controls here.

If you talk about permissions when we create/alter/drop a certain database
obejct, the existing PG model also checks its permission. These operations
are equivalent to modify metadata of database objects.
And, SE-PgSQL also checks its permissions on modification of metadata.

We can refer the metadata of the database object using SELECT on the system
catalogs typically. In this case, we need row-level granularity because
individual tuples mean metadata of the database object. So, we don't implement
permission checks of references to metadata in this version.

I introduced it on the README file as follows:

| o getattr (not implemented yet)
|
| It is checked when a client read properties of database object.
|
| Typically, it is required on scanning database objects within system catalogs,
| such as "SELECT * FROM pg_class". Because this check requires row-level access
| control facilities, it is not implemented yet in this version.
|
| Note that no need to check this permission, unless fetched properties are
| consumed internally without returning to clients. For example, catcache stuff
| provides system internal stuff an easy way to refer system catalog. It is used
| to implement backend routines, and fetched properties are not returned to the
| client in normal way, so we don't check this permission here.

This is totally separate from the really important question of whether
SE-Linux has a future, and another about whether, if SE-Linux has a
future, PostgreSQL needs to go there.

All that aside, there is an excellent economic reason why a
proprietary product might need to follow a dead-end technology, namely
increasing shareholder value due to one or more large, long-term
deals.

PostgreSQL doesn't have this motive, although some of the proprietary
forks may well.

Can we see about Andrew Sullivan's point 4? Or is it more important
to address the "do we want to" question first?

Whatever order we take them in, we do need to address both.

As Bruce mentioned, our big motivation is improvement of web-system's
security. However, most of security burden tend to concentrate to the
quality of web applications, because it tends to share privileges on
OS/DB for all the user's requests.
In other word, correctness of the system depends on whether every
applications are bug-free, or not. But we know it is difficult to
keep them bug-free. :(

Nowadays, we can often see a web server which host multiple tenants
in a single daemon. For example, one is assigned to the division-X,
and the other is assigned to the division-Y, ...
In this case, it is worthful to prevent a (buggy) web-application to
access resources labeled as an other division independent from the
way to store information assets (either filesystem or database).

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

#21Josh Berkus
josh@agliodbs.com
In reply to: David Fetter (#18)
#22Joshua D. Drake
jd@commandprompt.com
In reply to: David Fetter (#18)
#23Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joshua D. Drake (#22)
#24Joshua D. Drake
jd@commandprompt.com
In reply to: Tom Lane (#23)
#25KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Josh Berkus (#21)
#26KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Tom Lane (#23)
#27KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Joshua D. Drake (#24)
#28Greg Williamson
gwilliamson39@yahoo.com
In reply to: KaiGai Kohei (#27)
#29Bruce Momjian
bruce@momjian.us
In reply to: Greg Williamson (#28)
#30Tom Lane
tgl@sss.pgh.pa.us
In reply to: KaiGai Kohei (#27)
#31KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Tom Lane (#30)
#32Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#30)
#33Josh Berkus
josh@agliodbs.com
In reply to: Bruce Momjian (#32)
#34Tom Lane
tgl@sss.pgh.pa.us
In reply to: Josh Berkus (#33)
#35Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#30)
#36KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Josh Berkus (#33)
#37KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Tom Lane (#34)
#38Andrew Dunstan
andrew@dunslane.net
In reply to: KaiGai Kohei (#37)
#39Ron Mayer
rm_pg@cheapcomplexdevices.com
In reply to: KaiGai Kohei (#37)
#40Ron Mayer
rm_pg@cheapcomplexdevices.com
In reply to: Joshua D. Drake (#24)
#41KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Bruce Momjian (#35)
#42KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Ron Mayer (#39)
#43KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Ron Mayer (#40)
#44Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#38)
#45Josh Berkus
josh@agliodbs.com
In reply to: Tom Lane (#34)
#46Robert Haas
robertmhaas@gmail.com
In reply to: Josh Berkus (#45)
#47Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#46)
#48Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#47)
#49Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#48)
#50Ron Mayer
rm_pg@cheapcomplexdevices.com
In reply to: Robert Haas (#46)
#51Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#49)
#52Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#51)
#53Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#52)
#54Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Robert Haas (#53)
#55Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#53)
#56Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Kevin Grittner (#54)
#57Martijn van Oosterhout
kleptog@svana.org
In reply to: Alvaro Herrera (#56)
#58Chris Browne
cbbrowne@acm.org
In reply to: Robert Haas (#51)
#59Tom Lane
tgl@sss.pgh.pa.us
In reply to: Martijn van Oosterhout (#57)
#60Tom Lane
tgl@sss.pgh.pa.us
In reply to: Chris Browne (#58)
#61Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#53)
#62Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Martijn van Oosterhout (#57)
#63Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#61)
#64Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#63)
#65Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#61)
#66KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Tom Lane (#55)
#67KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Bruce Momjian (#64)
#68KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#65)
#69KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Bruce Momjian (#61)
#70Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: KaiGai Kohei (#69)
#71Greg Smith
gsmith@gregsmith.com
In reply to: KaiGai Kohei (#69)
#72David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Greg Smith (#71)
#73Robert Haas
robertmhaas@gmail.com
In reply to: David P. Quigley (#72)
#74Robert Haas
robertmhaas@gmail.com
In reply to: Robert Haas (#53)
#75David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Robert Haas (#74)
#76Chad Sellers
csellers@tresys.com
In reply to: David P. Quigley (#75)
#77Robert Haas
robertmhaas@gmail.com
In reply to: Chad Sellers (#76)
#78Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#77)
#79Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#78)
#80Chad Sellers
csellers@tresys.com
In reply to: Robert Haas (#77)
#81David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Robert Haas (#79)
#82Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#77)
#83Robert Haas
robertmhaas@gmail.com
In reply to: David P. Quigley (#81)
#84David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Stephen Frost (#82)
#85Peter Eisentraut
peter_e@gmx.net
In reply to: Martijn van Oosterhout (#57)
#86Peter Eisentraut
peter_e@gmx.net
In reply to: Chris Browne (#58)
#87David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Robert Haas (#83)
#88Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#82)
#89Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#85)
#90David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Tom Lane (#89)
#91KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#73)
#92Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#88)
#93KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: David P. Quigley (#75)
#94Greg Smith
gsmith@gregsmith.com
In reply to: David P. Quigley (#84)
#95KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: David P. Quigley (#87)
#96Magnus Hagander
magnus@hagander.net
In reply to: Bruce Momjian (#92)
#97KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Stephen Frost (#82)
#98Robert Haas
robertmhaas@gmail.com
In reply to: Magnus Hagander (#96)
#99Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#98)
#100KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Bruce Momjian (#99)
#101Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#99)
#102Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#101)
#103Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#102)
#104Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#103)
#105David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Tom Lane (#104)
#106Mark Mielke
mark@mark.mielke.cc
In reply to: Tom Lane (#104)
#107Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#104)
#108Greg Smith
gsmith@gregsmith.com
In reply to: Tom Lane (#104)
#109Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#104)
#110KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: David P. Quigley (#105)
#111Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#109)
#112Greg Smith
gsmith@gregsmith.com
In reply to: Tom Lane (#111)
#113Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#111)
#114KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#113)
#115Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#111)
#116Stephen Frost
sfrost@snowman.net
In reply to: Tom Lane (#111)
#117Robert Haas
robertmhaas@gmail.com
In reply to: Magnus Hagander (#115)
#118Robert Haas
robertmhaas@gmail.com
In reply to: KaiGai Kohei (#114)
#119Joshua Brindle
method@manicmethod.com
In reply to: Stephen Frost (#116)
#120Stephen Smalley
sds@tycho.nsa.gov
In reply to: Robert Haas (#117)
#121David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Robert Haas (#118)
#122David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Stephen Frost (#116)
#123Stephen Frost
sfrost@snowman.net
In reply to: Magnus Hagander (#115)
#124Stephen Frost
sfrost@snowman.net
In reply to: David P. Quigley (#122)
#125Stephen Frost
sfrost@snowman.net
In reply to: David P. Quigley (#121)
#126Robert Haas
robertmhaas@gmail.com
In reply to: David P. Quigley (#121)
#127Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#126)
#128David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Stephen Frost (#125)
#129Robert Haas
robertmhaas@gmail.com
In reply to: David P. Quigley (#121)
#130David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Stephen Frost (#124)
#131David P. Quigley
dpquigl@tycho.nsa.gov
In reply to: Robert Haas (#126)
#132Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#129)
#133Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#129)
#134Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#132)
#135Stephen Frost
sfrost@snowman.net
In reply to: David P. Quigley (#130)
#136Stephen Frost
sfrost@snowman.net
In reply to: David P. Quigley (#131)
#137Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#133)
#138Stephen Smalley
sds@tycho.nsa.gov
In reply to: Stephen Frost (#133)
#139Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#134)
#140Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#137)
#141Stephen Frost
sfrost@snowman.net
In reply to: Stephen Smalley (#138)
#142Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#139)
#143Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#140)
#144Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#142)
#145Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#143)
#146Greg Smith
gsmith@gregsmith.com
In reply to: Stephen Frost (#144)
#147Greg Smith
gsmith@gregsmith.com
In reply to: Joshua Brindle (#119)
#148Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#145)
#149Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#148)
#150KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#143)
#151Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#104)
#152Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#151)
#153Ron Mayer
rm_pg@cheapcomplexdevices.com
In reply to: Bruce Momjian (#151)
#154Bruce Momjian
bruce@momjian.us
In reply to: Ron Mayer (#153)
#155Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#148)
#156Stephen Frost
sfrost@snowman.net
In reply to: Tom Lane (#149)
#157Stephen Frost
sfrost@snowman.net
In reply to: Stephen Frost (#156)
#158Stephen Frost
sfrost@snowman.net
In reply to: Bruce Momjian (#151)
#159Bruce Momjian
bruce@momjian.us
In reply to: Stephen Frost (#158)
#160Stephen Frost
sfrost@snowman.net
In reply to: Bruce Momjian (#159)
#161Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#160)
#162KaiGai Kohei
kaigai@ak.jp.nec.com
In reply to: Robert Haas (#161)