[PATCH] unalias of ACL_SELECT_FOR_UPDATE

KaiGai Kohei wrote:

Currently, the ACL_SELECT_FOR_UPDATE privilege is defined as an alias
of ACL_UPDATE as follows:

at src/include/nodes/parsenodes.h:
:
/* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
#define ACL_SELECT_FOR_UPDATE ACL_UPDATE
:

It is unconfortable for us because SE-PostgreSQL have two individual
permissions for updates (db_table:{update}) and explicit table locks
(db_table:{lock}), but it unables to discriminate whether the given
relation is actually used for UPDATE or SELECT FOR UPDATE.

What's the point of doing SELECT FOR UPDATE if you're not actually going
to UPDATE the row? Having separate permissions for SELECT FOR UPDATE and
UPDATE seems useless.

A separate permission for SELECT FOR SHARE makes more sense, though.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

KaiGai Kohei wrote:

Currently, the ACL_SELECT_FOR_UPDATE privilege is defined as an alias
of ACL_UPDATE as follows:

at src/include/nodes/parsenodes.h:
:
/* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
#define ACL_SELECT_FOR_UPDATE ACL_UPDATE
:

It is unconfortable for us because SE-PostgreSQL have two individual
permissions for updates (db_table:{update}) and explicit table locks
(db_table:{lock}), but it unables to discriminate whether the given
relation is actually used for UPDATE or SELECT FOR UPDATE.

What's the point of doing SELECT FOR UPDATE if you're not actually going
to UPDATE the row? Having separate permissions for SELECT FOR UPDATE and
UPDATE seems useless.

I wonder why SELECT FOR UPDATE need ACL_UPDATE, although the statement
itself does not modify any of the given relation.
Indeed, it normally leads UPDATE statements, but I think ACL_UPDATE
should be checked on the later phase.

A separate permission for SELECT FOR SHARE makes more sense, though.

It is my major concern rather than exclusive locks.
The SELECT FOR SHARE statement also requires ACL_SELECT_FOR_UPDATE,
although it is a read only operation. It makes us hard to set up
a table with foreign-key which refers a primary-key on read-only
table, for example.

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

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

Heikki Linnakangas wrote:

What's the point of doing SELECT FOR UPDATE if you're not actually going
to UPDATE the row? Having separate permissions for SELECT FOR UPDATE and
UPDATE seems useless.

I wonder why SELECT FOR UPDATE need ACL_UPDATE, although the statement
itself does not modify any of the given relation.

Because it blocks competing transactions in exactly the same way as an
UPDATE does. I agree with Heikki --- there is no apparent value in
having a separate permission bit for this. Given that AclMode is 3/4ths
full already, I'm not for inventing new privilege types without a very
strong use-case.

A separate bit for SELECT FOR SHARE might possibly make sense given the
strength-of-locking argument. But doing both would eat half of the
available bits, and bring nearer the day that we need a different
representation for AclMode.

regards, tom lane

Tom Lane wrote:

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

Heikki Linnakangas wrote:

What's the point of doing SELECT FOR UPDATE if you're not actually going
to UPDATE the row? Having separate permissions for SELECT FOR UPDATE and
UPDATE seems useless.

I wonder why SELECT FOR UPDATE need ACL_UPDATE, although the statement
itself does not modify any of the given relation.

Because it blocks competing transactions in exactly the same way as an
UPDATE does. I agree with Heikki --- there is no apparent value in
having a separate permission bit for this. Given that AclMode is 3/4ths
full already, I'm not for inventing new privilege types without a very
strong use-case.

A separate bit for SELECT FOR SHARE might possibly make sense given the
strength-of-locking argument. But doing both would eat half of the
available bits, and bring nearer the day that we need a different
representation for AclMode.

I would not like to have a discussion how many permission bits will be
necessary in the future, because nobody can say something ensured.

We have an another approach that defines ACL_SELECT_FOR_SHARE as
an alias of ACL_SELECT, and applies it on SELECT FOR SHARE statement.
(Needless to say, the targets are already listed, so it might not necessary
to put a ACL_SELECT_FOR_SHARE bit explicitly.)

In the LOCK statement, it checks ACL_SELECT privilege for shared locks and
discriminate between shared and exclusive locks. It seems to me quite natural.

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

KaiGai Kohei wrote:

We have an another approach that defines ACL_SELECT_FOR_SHARE as
an alias of ACL_SELECT, and applies it on SELECT FOR SHARE statement.
(Needless to say, the targets are already listed, so it might not necessary
to put a ACL_SELECT_FOR_SHARE bit explicitly.)

That's even more useless, since you need ACL_SELECT for SELECT FOR SHARE
anyway.

In the LOCK statement, it checks ACL_SELECT privilege for shared locks and
discriminate between shared and exclusive locks. It seems to me quite natural.

It checks ACL_SELECT for *Access*ShareLock. SELECT FOR SHARE acquires a
RowShareLock. So the equivalent of "SELECT * FROM foo FOR SHARE" using
LOCK is "LOCK TABLE foo RowShareLock", which checks
ACL_UPDATE|ACL_DELETE|ACL_TRUNCATE.

IMHO the only sane change would be to introduce a new
ACL_SELECT_FOR_SHARE permission for SELECT FOR SHARE. That way you could
grant SELECT_FOR_SHARE permission on a table to let people insert rows
into other tables that have a foreign key reference to it, without
having to grant UPDATE permission.

Does the SQL spec have anything to say about this, BTW?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

IMHO the only sane change would be to introduce a new
ACL_SELECT_FOR_SHARE permission for SELECT FOR SHARE.

This might be worth doing ...

That way you could
grant SELECT_FOR_SHARE permission on a table to let people insert rows
into other tables that have a foreign key reference to it, without
having to grant UPDATE permission.

... but this argument for it is utter nonsense. FKs are not a
permissions problem, because the triggers run as the table's owner.
The only permission you need is REFERENCES:

regression=# create user m;
CREATE ROLE
regression=# create user s;
CREATE ROLE
regression=# \c - m
psql (8.4beta1)
You are now connected to database "regression" as user "m".
regression=> create table m(f1 int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "m_pkey" for table "m"
CREATE TABLE
regression=> insert into m values(1);
INSERT 0 1
regression=> \c - s
psql (8.4beta1)
You are now connected to database "regression" as user "s".
regression=> create table s (f1 int references m);
ERROR: permission denied for relation m
regression=> \c - m
psql (8.4beta1)
You are now connected to database "regression" as user "m".
regression=> grant references on m to s;
GRANT
regression=> \c - s
psql (8.4beta1)
You are now connected to database "regression" as user "s".
regression=> create table s (f1 int references m);
CREATE TABLE
regression=> insert into s values(1);
INSERT 0 1
regression=> insert into s values(2);
ERROR: insert or update on table "s" violates foreign key constraint "s_f1_fkey"
DETAIL: Key (f1)=(2) is not present in table "m".
regression=>

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

That way you could
grant SELECT_FOR_SHARE permission on a table to let people insert rows
into other tables that have a foreign key reference to it, without
having to grant UPDATE permission.

... but this argument for it is utter nonsense. FKs are not a
permissions problem, because the triggers run as the table's owner.
The only permission you need is REFERENCES:

Then I think we should leave it as it is. I don't see any plausible use
case for a separate SELECT FOR SHARE or UPDATE permission.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

That way you could
grant SELECT_FOR_SHARE permission on a table to let people insert rows
into other tables that have a foreign key reference to it, without
having to grant UPDATE permission.

... but this argument for it is utter nonsense. FKs are not a
permissions problem, because the triggers run as the table's owner.
The only permission you need is REFERENCES:

Then I think we should leave it as it is. I don't see any plausible use
case for a separate SELECT FOR SHARE or UPDATE permission.

My concern is not on the vanilla PostgreSQL's access controls.

In the discussion for v8.4 development cycle, I got a suggestion that
the query-tree-walker routine is a duplication of the code so we should
utilize the information collected by the core analyzer. I agreed this
design changes and it enabled to simplify the implementation.
However, ACL_UPDATE and ACL_SELECT_FOR_UPDATE internally shares same bit
so SE-PostgreSQL cannot discriminate between UPDATE and SELECT FOR UPDATE
or SHARE. It has been a headach for me.

Last night, I got an another idea without any changes ACL_xxx definitions.
If the given rte->requiredPerms has ACL_UPDATE (or ACL_SELECT_FOR_UPDATE),
we might be able to discriminate them using rte->modifiedCols, because
UPDATE statement set a bit on rte->modifiedCols at least.

if (rte->requiredPerms & ACL_UPDATE) {
if (bms_is_empty(rte->modifiedCols))
// consider it as SELECT FOR UPDATE/SHARE
else
// consider it as UPDATE
}

If we don't have a (internally) separate bit on ACL_SELECT_FOR_UPDATE,
it can be a solution for SE-PostgreSQL.

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

KaiGai Kohei wrote:

However, ACL_UPDATE and ACL_SELECT_FOR_UPDATE internally shares same bit
so SE-PostgreSQL cannot discriminate between UPDATE and SELECT FOR UPDATE
or SHARE.

Why should it discriminate between them?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

KaiGai Kohei wrote:

However, ACL_UPDATE and ACL_SELECT_FOR_UPDATE internally shares same bit
so SE-PostgreSQL cannot discriminate between UPDATE and SELECT FOR UPDATE
or SHARE.

Why should it discriminate between them?

Typically, we cannot set up a foreign-key which refers a primary-key within
read-only table from SELinux's viewpoint.
The vanilla access control mechanism switches the current userid, and it enables
to run SELECT FOR SHARE without ACL_UPDATE, but SELinux's security model does not
have a concept of ownership.

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

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

Heikki Linnakangas wrote:

Why should it discriminate between them?

Typically, we cannot set up a foreign-key which refers a primary-key within
read-only table from SELinux's viewpoint.
The vanilla access control mechanism switches the current userid, and it enables
to run SELECT FOR SHARE without ACL_UPDATE, but SELinux's security model does not
have a concept of ownership.

Should I not read that as "SELinux's security model is so impoverished
that it cannot be useful for monitoring SQL behavior"? If you don't
understand current user and ownership, it's hopeless. Trying to
distinguish SELECT FOR UPDATE instead of that is a workaround that is
only going to fix one symptom (if it even works for this, which I doubt).
There will be many more.

regards, tom lane

Tom Lane wrote:

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

Heikki Linnakangas wrote:

Why should it discriminate between them?

Typically, we cannot set up a foreign-key which refers a primary-key within
read-only table from SELinux's viewpoint.
The vanilla access control mechanism switches the current userid, and it enables
to run SELECT FOR SHARE without ACL_UPDATE, but SELinux's security model does not
have a concept of ownership.

Should I not read that as "SELinux's security model is so impoverished
that it cannot be useful for monitoring SQL behavior"? If you don't
understand current user and ownership, it's hopeless. Trying to
distinguish SELECT FOR UPDATE instead of that is a workaround that is
only going to fix one symptom (if it even works for this, which I doubt).
There will be many more.

It is a difference between two security designs, characteristics and
philosophies, not a competitive merit and demerit.
SELinux makes its decision based on the security policy and the security
context of client and objects accessed. Here, user identifier and object
ownership don't appear.
Meanwhile, the vanilla PostgreSQL makes its decision based on the user
identifier and database ACLs of objects accessed. It does not use the
security context, needless to say.

At the begining, I considered that SE-PostgreSQL want ACL_SELECT_FOR_UPDATE
to have individual bit other than ACL_UPDATE, but I also considered it should
not add user visible changes in the vanilla access controls, so the first
proposition changed only internal representation without new user visible
permissions (it was still checked as UPDATE privilege).

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

KaiGai Kohei wrote:

Tom Lane wrote:

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

The vanilla access control mechanism switches the current userid, and it enables
to run SELECT FOR SHARE without ACL_UPDATE, but SELinux's security model does not
have a concept of ownership.

Should I not read that as "SELinux's security model is so impoverished
that it cannot be useful for monitoring SQL behavior"? If you don't
understand current user and ownership, it's hopeless. Trying to
distinguish SELECT FOR UPDATE instead of that is a workaround that is
only going to fix one symptom (if it even works for this, which I doubt).
There will be many more.

It is a difference between two security designs, characteristics and
philosophies, not a competitive merit and demerit.
SELinux makes its decision based on the security policy and the security
context of client and objects accessed. Here, user identifier and object
ownership don't appear.
Meanwhile, the vanilla PostgreSQL makes its decision based on the user
identifier and database ACLs of objects accessed. It does not use the
security context, needless to say.

Can't you have a SE-PostgreSQL policy like "disallow ACL_UPDATE on table
X for user Y, except when current user is owner of X"?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

KaiGai Kohei wrote:

Tom Lane wrote:

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

The vanilla access control mechanism switches the current userid, and it enables
to run SELECT FOR SHARE without ACL_UPDATE, but SELinux's security model does not
have a concept of ownership.

Should I not read that as "SELinux's security model is so impoverished
that it cannot be useful for monitoring SQL behavior"? If you don't
understand current user and ownership, it's hopeless. Trying to
distinguish SELECT FOR UPDATE instead of that is a workaround that is
only going to fix one symptom (if it even works for this, which I doubt).
There will be many more.

It is a difference between two security designs, characteristics and
philosophies, not a competitive merit and demerit.
SELinux makes its decision based on the security policy and the security
context of client and objects accessed. Here, user identifier and object
ownership don't appear.
Meanwhile, the vanilla PostgreSQL makes its decision based on the user
identifier and database ACLs of objects accessed. It does not use the
security context, needless to say.

Can't you have a SE-PostgreSQL policy like "disallow ACL_UPDATE on table
X for user Y, except when current user is owner of X"?

It seems to me a quite ad-hoc idea.
At least, it can prevents several users (with individual security contexts)
to share a common read-writable table, even if both of the kernel security
policy and vanilla database ACL allow it.
Now we can discriminate them using rte->modifiedCols. I don't find out any
problem in this approach. It can solve my headach without any changes in
the vanilla database ACLs.

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

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

Heikki Linnakangas wrote:

Can't you have a SE-PostgreSQL policy like "disallow ACL_UPDATE on table
X for user Y, except when current user is owner of X"?

It seems to me a quite ad-hoc idea.

That's rather a silly charge to be leveling when your own proposal is
such a horrid kluge as this one. As near as I can tell, you intend
that SELinux will be unable to prohibit SELECT FOR UPDATE because it
cannot tell the difference between that and a foreign key reference.
If that isn't a hack, I don't know what is.

regards, tom lane

On Mon, Apr 20, 2009 at 3:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

It seems to me a quite ad-hoc idea.

That's rather a silly charge to be leveling when your own proposal is
such a horrid kluge as this one.  As near as I can tell, you intend
that SELinux will be unable to prohibit SELECT FOR UPDATE because it
cannot tell the difference between that and a foreign key reference.
If that isn't a hack, I don't know what is.

I think we're talking at cross purposes here. I think Kai Gai's
descriptions make sense if you start with a different set of
assumptions. The idea behind SELinux is that each individual object is
access controlled and each user has credentials which grant access to
specific operations on specific objects. As I understand it part of
the goal is to eliminate situations where "setuid" or other forms of
privilege escalation is required.

So in this situation -- I suspect, if any SELinux people want to pipe
up to tell me whether I'm on the right track -- the idea is that you
should be able to examine a user superficially and know for certain
whether he has the ability to lock a record or whether that privilege
has been denied him. It shouldn't be possible for him to gain the
privilege by going through a view or trigger which runs as another
user.

If that's right then the previous suggestion that we take only the
part of SELinux which maps to the existing POstgres model really falls
down. SEPostgres won't map to the Postgres model just as SELinux
doesn't map directly to the traditional Unix security model. It'll be
a whole new security model which may be internally consistent but
won't make sense to think about together with the Postgres model. That
would be a hard sell because as demonstrated in this thread, we don't
really understand the SE security model and it would probably be quite
invasive to support.

If on the other hand I'm wrong and this isn't a fundamental feature
but just an implementation question then I think the right solution is
to fix the problems that make it hard to implement the Postgres
security model in SELinux. The consensus earlier was that the first
version of the patch to land would just be a minimal patch which
implements the existing security model using SELinux without making
any changes to the model. Playing around with new privileges and how
we distinguish referential integrity checks wouldn't be part of that.

--
greg

Greg Stark <stark@enterprisedb.com> writes:

I think we're talking at cross purposes here. I think Kai Gai's
descriptions make sense if you start with a different set of
assumptions. The idea behind SELinux is that each individual object is
access controlled and each user has credentials which grant access to
specific operations on specific objects. As I understand it part of
the goal is to eliminate situations where "setuid" or other forms of
privilege escalation is required.

Well, if so, the idea is a miserable failure. SELinux has just as many
setuid programs as any other Unix, and absolutely zero hope of removing
them. I am not going to take the idea of "remove setuid" seriously when
they haven't been able to accomplish it anywhere else.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Greg Stark <stark@enterprisedb.com> writes:

I think we're talking at cross purposes here. I think Kai Gai's
descriptions make sense if you start with a different set of
assumptions. The idea behind SELinux is that each individual object is
access controlled and each user has credentials which grant access to
specific operations on specific objects. As I understand it part of
the goal is to eliminate situations where "setuid" or other forms of
privilege escalation is required.

Well, if so, the idea is a miserable failure. SELinux has just as many
setuid programs as any other Unix, and absolutely zero hope of removing
them. I am not going to take the idea of "remove setuid" seriously when
they haven't been able to accomplish it anywhere else.

But can you remove privileges from users to make these programs ineffective?
So even if you obtain root privileges you're missing the SE privilege which
the program expects to use?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

On Mon, Apr 20, 2009 at 03:48:11PM +0100, Greg Stark wrote:

So in this situation -- I suspect, if any SELinux people want to pipe
up to tell me whether I'm on the right track -- the idea is that you
should be able to examine a user superficially and know for certain
whether he has the ability to lock a record or whether that privilege
has been denied him. It shouldn't be possible for him to gain the
privilege by going through a view or trigger which runs as another
user.

My (admittedly superficial) research into the topic suggests to me that
it's because SELinux is entirely into protecting the data. It doesn't
really care whether you're accessing it via a view or function or what.
If you don't have permissions you can't get it and no-one within
postgresql can grant you access either (that's why it's MAC).

The way I understood the specific problem here is that SELECT FOR
UPDATE doesn't semantically change any data so you don't really need
UPDATE permissions to do it. That's just an artifact of the Postgres
implementation.

If on the other hand I'm wrong and this isn't a fundamental feature
but just an implementation question then I think the right solution is
to fix the problems that make it hard to implement the Postgres
security model in SELinux. The consensus earlier was that the first
version of the patch to land would just be a minimal patch which
implements the existing security model using SELinux without making
any changes to the model. Playing around with new privileges and how
we distinguish referential integrity checks wouldn't be part of that.

ISTM that limiting the patch to doing what can already be done with
standard postgresql is silly. SE-Postgres is not trying to supplant the
Pg model, it's trying to do things that the Pg model can't do. Namely,
label stuff secret and be sure no-one without clearence can read it,
even if someone makes a setuid function for it.

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