Updates of SE-PostgreSQL 8.4devel patches (r1704) - Mailing list pgsql-hackers

As I promised last week, SE-PostgreSQL patches are revised here:

[1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch
[2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch
[3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch
[4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch
[5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch

* List of updates:- It is rebased to the latest CVS HEAD.
- The two reader permission ("select" and "use") are integrated into  a single one ("select") as the original design
didtwo year's ago.  (It also enables to pick up read columns from rte->selectedCols.)
 
- The 'walker' code in sepgsql/checker.c is removed.  In the previous revision, SE-PostgreSQL walked on the given query
tree to pick up all the appeared tables/columns. The reason why  we needed separated walker phase is SE-PostgreSQL
wantedto apply  two kind of "reader" permissions, but these are integrated into one.  (In addition, column-level
privilegesare not available when I   started to develop SE-PostgreSQL. :-))  In the currect version, SE-PostgreSQL
knowswhat tables/columns  are appeared in the given query, from relid, selectedCols and  modifiedCols in RangeTblEntry.
Then,it makes access controls  decision communicating with in-kernel SELinux.  After the existing DAC are checked,
SE-PostgreSQLalso checks  client's privileges on the appeared tables/columns as DAC doing.  Required privilges are
followsthese rules:   * If ACL_SELECT is set on rte->requiredPerms, client need to have      db_table:{select} and
db_column:{select}for the tables/columns.   * If ACL_INSERT is set on rte->requiredPerms, client need to have
db_table:{insert}and db_column:{insert} for the tables/columns.   * If ACL_UPDATE is set on rte->requiredPerms, client
needto have      db_table:{update} and db_column:{update} for the tables/columns.   * If ACL_DELETE is set on
rte->requiredPerms,client need to have      db_table:{delete} for the tables.  This design change gives us a great
benefitin code maintenance.  A trade-off is hardwired rules to modify "pg_rewrite" system catalog.  The correctness
accesscontrols depends on this catalog is protected  from unexpected manipulation by hand, because it stores a parsed
querytree of views.
 
- T_SelinuxItem is removed from include/node/nodes.h, and the codes  related to the node type is eliminated from
copyfuncs.cant others.  It was used to store all appeared tables/columns in the walker phase,  but now it is
unnecessary.
- Several functions are moved to appropriate files:  - The codes related to permission bits are consolidated to
'sepgsql/perms.c'as its filename means.    (It was placed at 'avc.c' due to historical reason.)  - A few hooks (such as
sepgsqlHeapTupleInsert)are moved to 'checker.c',    and rest of simple hooks are kept in 'hooks.c'.
 
- The scale of patches become a bit slim more.                               <rev.1668>          <rev.1704>
security/Makefile           |   11          ->  |   11 security/sepgsql/Makefile    |   16          ->  |   16
security/sepgsql/avc.c      | 1157 +++++++  ->  |  837 +++++ security/sepgsql/checker.c   |  902 +++++    ->  |  538
+++security/sepgsql/core.c      |  235 +        ->  |  247 + security/sepgsql/dummy.c     |   37          ->  |   43
security/sepgsql/hooks.c    |  748 ++++     ->  |  621 +++ security/sepgsql/label.c     |  360 ++       ->  |  360 ++
security/sepgsql/perms.c    |  295 +        ->  |  400 ++
 
 [kaigai@saba ~]$ diffstat sepgsql-core-8.4devel-r1668.patch        :  64 files changed, 4770 insertions(+), 11
deletions(-),4947 modifications(!)
 
 [kaigai@saba ~]$ diffstat sepgsql-core-8.4devel-r1704.patch        :  60 files changed, 4048 insertions(+), 11
deletions(-),4944 modifications(!)
 
 About 700 lines can be reduced in total!

I believe this revision can reduce the burden of reviewers.
Please any comments!


* An issue:I found a new issue in this revision.
- ACL_SELECT_FOR_UPDATE and ACL_UPDATE have identical value.
 In this version, SE-PostgreSQL knows what permission should be checked via RangeTblEntry::requiredPerms, and it
appliesits access control policy with translating them into SELinux's permissions.
 
 But we have a trouble in the following query. ---------------- [kaigai@saba ~]$ psql postgres psql (8.4devel) Type
"help"for help.
 
 postgres=# CREATE TABLE t1 (a int, b text) postgres-#     SECURITY_LABEL = 'system_u:object_r:sepgsql_ro_table_t:s0';
CREATETABLE   -- NOTE: "sepgsql_ro_table_t" means read-only table from unpriv clients. postgres=# INSERT INTO t1 VALUES
(1,'aaa'), (2, 'bbb'); INSERT 0 2 postgres=# \q [kaigai@saba ~]$ runcon -t sepgsql_test_t -- psql postgres psql
(8.4devel)Type "help" for help.   -- NOTE: "sepgsql_test_t" means unpriv client. postgres=# SELECT * FROM t1;  a |  b
---+----- 1 | aaa  2 | bbb (2 rows)
 
 postgres=# SELECT * FROM t1 FOR SHARE OF t1; ERROR:  SELinux: denied { update }
scontext=unconfined_u:unconfined_r:sepgsql_test_t:s0-s0:c0.c63tcontext=system_u:object_r:sepgsql_ro_table_t:s0
tclass=db_tablename=t1 ----------------
 
 It raises an error with the client does not have db_table:{update} permission on the table labeled as
"sepgsql_ro_table_t",although this query does not modify the table: "t1".
 
 Our desirable behavior is SE-PostgreSQL checks db_table:{select lock} and db_column:{select} permissions for the
tables/columns.(*) db_table:{lock} means permission to lock a table explicitly.
 
 The reason why the ACL_UPDATE is set on RangeTblEntry::requiredPerms is ACL_SELECT_FOR_UPDATE is defined as same value
withACL_UPDATE, so we cannot distinguish which permission is required, or both of them.
 
 I want these permissions have different values without any impacts for user interfaces. One possible idea is
ACL_UPDATE_CHR('w') to be extracted into (ACL_UPDATE | ACL_SELECT_FOR_UPDATE) bits, not a single bit for each
character,and it is granted by the token of "update" in GRANT/REVOKE statement.
 
 Basically, I don't want go back to the "walker" approach. The ACL_SELECT_FOR_UPDATE without identical value is much
desirablefor me. What is your opinion?
 

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