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

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(at)ak(dot)jp(dot)nec(dot)com>

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(at)ak(dot)jp(dot)nec(dot)com>