Thread

1. [v9.1] access control reworks in ALTER TABLE

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-17T07:26:49Z

   The attached patch reworks access control logics corresponding
   to ALTER TABLE statement, as a groundwork for the upcoming
   enhanced security features.
   
   As we discussed in the last commit fest, it shall be a preferable
   way to wrap up a unit of access control logics into some functions
   which are categorized by the actions to be checked. These functions
   will be able to perform as entrypoints of existing permission checks
   and the upcoming security features, like SELinux.
   
   The first step of this efforts is to consolidate existing access
   control codes into a unit for each operations. Once we reworks them,
   it is obvious to replace these unit by access control functions.
   ALTER TABLE is the most functional command in PostgreSQL, so it may
   be a good stater for this efforts.
   --------
   
   In this patch, I put access control codes in the execution stage
   after all needed information getting gathered. Then, privileges
   are checked at once.
   
   The ALTER TABLE implementation, exceptionally, has a few stages.
   The preparation stage applies sanity and permission checks, and
   expand the given command to child relations. Then, the execution
   stage updates system catalogs according to the given command.
   However, we don't have multiple stages in most of ddl statements.
   So, even if we adopt a basis to apply permission checks in the
   preparation stage, we cannot reuse this basis for others.
   
   Most of AT commands checks ownerships of the relation using
   ATSimplePermissions() in the preparation stage, but now a few number
   of commands also needs to check privileges in the execution stage
   dues to some reasons.
   
   (1) commands with self recursion
   ATExecDropColumn(), ATAddCheckConstraint() and ATExecDropConstraint()
   implements its recursive calls by itself. It means we cannot know
   child relations to be altered in the preparation stage, so it has
   the following code typically.
   
   |   /* At top level, permission check was done in ATPrepCmd, else do it */
   |   if (recursing)
   |       ATSimplePermissions(rel, false);
   
   (2) commands directly called from alter.c
   RenameRelation(), renameatt() and AlterTableNamespace() are directly
   called from alter.c, so they have no preparation stage. Instead of
   the ATSimplePermissions(), it calls CheckRelationOwnership() in the
   caller. Also, renameatt() checks ownership of the relation during
   its recursive calls.
   
   (3) commands need privilges to other objects
   - AlterTableNamespace() checks ACL_CREATE on the specified namespace
     in the LookupCreationNamespace().
   - ATPrepSetTableSpace() checks ACL_CREATE on the specified tablespace.
   - ATExecAddIndex() checks ACL_CREATE on the namespace and ACL_CREATE
     on the specified tablespace (if exist) in DefineIndex().
   - ATExecAddInherit() checks ownership of the specified parent relation.
   - ATAddForeignKeyConstraint() checks ACL_REFERENCES on the both of
     specified table/columns.
   
   (4) commands needs its own sanity checks
   ATSimplePermissions() also ensures the relation is table (or view),
   and not a system catalog, not only checks its ownership.
   However, some of commands need to apply an exceptional sanity checks.
   ATSimplePermissionsRelationOrIndex() is used for sanity checks of the
   AT commands which is allowed on indexes. ATPrepSetStatistics() skips
   checks to ensure the relation is not system catalogs.
   
   
   At first, this patch broke down the ATSimplePermissions() into sanity
   checks and permission checks.
   
   This patch break down the ATSimplePermissions() into sanity checks
   and permissions checks, then later part is moved to the execution
   stage.
    - ATCheckSanity()
    - ATCheckOwnership()
   
   It enables to eliminate (1) special case handling on the commands with
   self recursion, and (4) exceptional sanity checks because ATCheckSanity()
   takes three arguments to control sanity checks behavior.
   
   It also means we don't need to consider (2) functions are exceptional,
   because all the commands apply checks in the execution stage.
   
   For the (3) functions, it moved ownership checks nearby the permission
   checks to other object, according to the basis: access control codes in
   the execution stage after all needed information getting gathered, and
   these are checked at once.
   
   ATExecAddIndex() is a wrapper function of the DefineIndex(). It requires
   caller checks ownership of the relation to be indexed, then it checks
   rest of permissions for namespace and tablespace. I moved checks for
   ownership of the relation into DefineIndex(), and also eliminated the
   CheckRelationOwnership() from standard_ProcessUtility() that is another
   caller of DefineIndex().
   
   Any comments?
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@ak.jp.nec.com>
   

2. Re: [v9.1] access control reworks in ALTER TABLE

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-24T06:52:08Z

   At Ottawa, I had a talk with Robert Haas about this reworks.
   
   Because a part of ALTER TABLE options need information which can
   be gathered at execution stage, not preparation stage, the patch
   tried to move all the access control stuff into execution stage.
   
   However, he pointed out it has a matter the tables tried to be
   altered may acquire locks on the table without permissions.
   It can allow DoS attacks, so we agreed we should not move these
   checks into execution stage.
   Fortunately, only three ALTER TABLE options need information
   gathered at execution stage (add inheritance; add FK constraints;
   add index); So, we can rework them using separated checker functions,
   so it is not a matter.
   
   I marked it as returned with feedback.
   At the 1st CF, we focus on the reworks of DML permission checks.
   So, the rest of DDL works should be done on the 2nd.
   
   Thanks,
   
   (2010/03/17 16:26), KaiGai Kohei wrote:
   > The attached patch reworks access control logics corresponding
   > to ALTER TABLE statement, as a groundwork for the upcoming
   > enhanced security features.
   > 
   > As we discussed in the last commit fest, it shall be a preferable
   > way to wrap up a unit of access control logics into some functions
   > which are categorized by the actions to be checked. These functions
   > will be able to perform as entrypoints of existing permission checks
   > and the upcoming security features, like SELinux.
   > 
   > The first step of this efforts is to consolidate existing access
   > control codes into a unit for each operations. Once we reworks them,
   > it is obvious to replace these unit by access control functions.
   > ALTER TABLE is the most functional command in PostgreSQL, so it may
   > be a good stater for this efforts.
   > --------
   > 
   > In this patch, I put access control codes in the execution stage
   > after all needed information getting gathered. Then, privileges
   > are checked at once.
   > 
   > The ALTER TABLE implementation, exceptionally, has a few stages.
   > The preparation stage applies sanity and permission checks, and
   > expand the given command to child relations. Then, the execution
   > stage updates system catalogs according to the given command.
   > However, we don't have multiple stages in most of ddl statements.
   > So, even if we adopt a basis to apply permission checks in the
   > preparation stage, we cannot reuse this basis for others.
   > 
   > Most of AT commands checks ownerships of the relation using
   > ATSimplePermissions() in the preparation stage, but now a few number
   > of commands also needs to check privileges in the execution stage
   > dues to some reasons.
   > 
   > (1) commands with self recursion
   > ATExecDropColumn(), ATAddCheckConstraint() and ATExecDropConstraint()
   > implements its recursive calls by itself. It means we cannot know
   > child relations to be altered in the preparation stage, so it has
   > the following code typically.
   > 
   > |   /* At top level, permission check was done in ATPrepCmd, else do it */
   > |   if (recursing)
   > |       ATSimplePermissions(rel, false);
   > 
   > (2) commands directly called from alter.c
   > RenameRelation(), renameatt() and AlterTableNamespace() are directly
   > called from alter.c, so they have no preparation stage. Instead of
   > the ATSimplePermissions(), it calls CheckRelationOwnership() in the
   > caller. Also, renameatt() checks ownership of the relation during
   > its recursive calls.
   > 
   > (3) commands need privilges to other objects
   > - AlterTableNamespace() checks ACL_CREATE on the specified namespace
   >    in the LookupCreationNamespace().
   > - ATPrepSetTableSpace() checks ACL_CREATE on the specified tablespace.
   > - ATExecAddIndex() checks ACL_CREATE on the namespace and ACL_CREATE
   >    on the specified tablespace (if exist) in DefineIndex().
   > - ATExecAddInherit() checks ownership of the specified parent relation.
   > - ATAddForeignKeyConstraint() checks ACL_REFERENCES on the both of
   >    specified table/columns.
   > 
   > (4) commands needs its own sanity checks
   > ATSimplePermissions() also ensures the relation is table (or view),
   > and not a system catalog, not only checks its ownership.
   > However, some of commands need to apply an exceptional sanity checks.
   > ATSimplePermissionsRelationOrIndex() is used for sanity checks of the
   > AT commands which is allowed on indexes. ATPrepSetStatistics() skips
   > checks to ensure the relation is not system catalogs.
   > 
   > 
   > At first, this patch broke down the ATSimplePermissions() into sanity
   > checks and permission checks.
   > 
   > This patch break down the ATSimplePermissions() into sanity checks
   > and permissions checks, then later part is moved to the execution
   > stage.
   >   - ATCheckSanity()
   >   - ATCheckOwnership()
   > 
   > It enables to eliminate (1) special case handling on the commands with
   > self recursion, and (4) exceptional sanity checks because ATCheckSanity()
   > takes three arguments to control sanity checks behavior.
   > 
   > It also means we don't need to consider (2) functions are exceptional,
   > because all the commands apply checks in the execution stage.
   > 
   > For the (3) functions, it moved ownership checks nearby the permission
   > checks to other object, according to the basis: access control codes in
   > the execution stage after all needed information getting gathered, and
   > these are checked at once.
   > 
   > ATExecAddIndex() is a wrapper function of the DefineIndex(). It requires
   > caller checks ownership of the relation to be indexed, then it checks
   > rest of permissions for namespace and tablespace. I moved checks for
   > ownership of the relation into DefineIndex(), and also eliminated the
   > CheckRelationOwnership() from standard_ProcessUtility() that is another
   > caller of DefineIndex().
   > 
   > Any comments?
   > 
   > Thanks,
   > 
   > 
   > 
   > 
   
   
   -- 
   KaiGai Kohei <kaigai@ak.jp.nec.com>