Skip site navigation (1) Skip section navigation (2)

Updates of SE-PostgreSQL 8.4devel patches (r1704)

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>, Bruce Momjian <bruce(at)momjian(dot)us>, Joshua Brindle <method(at)manicmethod(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Josh Berkus <josh(at)agliodbs(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>, Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>
Subject: Updates of SE-PostgreSQL 8.4devel patches (r1704)
Date: 2009-03-09 06:52:40
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
As I promised last week, SE-PostgreSQL patches are revised here:


* 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 did two 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 wanted to apply
   two kind of "reader" permissions, but these are integrated into one.
   (In addition, column-level privileges are not available when I
    started to develop SE-PostgreSQL. :-))
   In the currect version, SE-PostgreSQL knows what 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-PostgreSQL also checks
   client's privileges on the appeared tables/columns as DAC doing.
   Required privilges are follows these 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 need to 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 benefit in code maintenance.
   A trade-off is hardwired rules to modify "pg_rewrite" system catalog.
   The correctness access controls depends on this catalog is protected
   from unexpected manipulation by hand, because it stores a parsed
   query tree of views.

 - T_SelinuxItem is removed from include/node/nodes.h, and the codes
   related to the node type is eliminated from copyfuncs.c ant 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(at)saba ~]$ diffstat sepgsql-core-8.4devel-r1668.patch
   64 files changed, 4770 insertions(+), 11 deletions(-), 4947 modifications(!)

  [kaigai(at)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 applies its access control
  policy with translating them into SELinux's permissions.

  But we have a trouble in the following query.
  [kaigai(at)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';
    -- 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(at)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.c63 tcontext=system_u:object_r:sepgsql_ro_table_t:s0 tclass=db_table name=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 with ACL_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 desirable
  for me. What is your opinion?

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

In response to


pgsql-hackers by date

Next:From: Pavel StehuleDate: 2009-03-09 07:46:09
Subject: Re: Out parameters handling
Previous:From: Ryan BradetichDate: 2009-03-09 05:01:21
Subject: Re: Out parameters handling

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group