Thread: [PATCH] unalias of ACL_SELECT_FOR_UPDATE

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.
So, I would like to define the bit as (1<<12) which is not in use.

I think we have two options to solve the matter.

The one is to check ACL_SELECT_FOR_UPDATE by UPDATE privilege, as
the attached patch doing. It implicitly expands the users privilege
when he has ACL_UPDATE, so it enables GRANT UPDATE to cover both of
ACL_UPDATE and ACL_SELECT_FOR_UPDATE as the current implementation
doing.

The other is to add a new privilege for explicit table locks, such
as something like LOCK privilege. It is a straightforward approach,
but we need to have a user visible changes.

Which is more preferable design?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
*** base/src/include/nodes/parsenodes.h    2009-04-09 00:13:21.000000000 +0900
--- base_ex/src/include/nodes/parsenodes.h    2009-04-09 00:41:07.000000000 +0900
*************** typedef uint32 AclMode;            /* a bitmask o
*** 71,80 ****
  #define ACL_CREATE        (1<<9)    /* for namespaces and databases */
  #define ACL_CREATE_TEMP (1<<10) /* for databases */
  #define ACL_CONNECT        (1<<11) /* for databases */
- #define N_ACL_RIGHTS    12        /* 1 plus the last 1<<x */
- #define ACL_NO_RIGHTS    0
  /* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
! #define ACL_SELECT_FOR_UPDATE    ACL_UPDATE


  /*****************************************************************************
--- 71,80 ----
  #define ACL_CREATE        (1<<9)    /* for namespaces and databases */
  #define ACL_CREATE_TEMP (1<<10) /* for databases */
  #define ACL_CONNECT        (1<<11) /* for databases */
  /* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
! #define ACL_SELECT_FOR_UPDATE    (1<<12)
! #define N_ACL_RIGHTS    13        /* 1 plus the last 1<<x */
! #define ACL_NO_RIGHTS    0


  /*****************************************************************************
*** base/src/backend/utils/adt/acl.c    2009-02-09 13:08:34.000000000 +0900
--- base_ex/src/backend/utils/adt/acl.c    2009-04-06 11:49:32.000000000 +0900
*************** aclmask(const Acl *acl, Oid roleid, Oid
*** 1141,1151 ****
      for (i = 0; i < num; i++)
      {
          AclItem    *aidata = &aidat[i];

          if (aidata->ai_grantee == ACL_ID_PUBLIC ||
              aidata->ai_grantee == roleid)
          {
!             result |= aidata->ai_privs & mask;
              if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
                  return result;
          }
--- 1141,1156 ----
      for (i = 0; i < num; i++)
      {
          AclItem    *aidata = &aidat[i];
+         AclMode        aiprivs = aidata->ai_privs;
+
+         /* fixup ACL_SELECT_FOR_UPDATE */
+         if (aiprivs & ACL_UPDATE)
+             aiprivs |= ACL_SELECT_FOR_UPDATE;

          if (aidata->ai_grantee == ACL_ID_PUBLIC ||
              aidata->ai_grantee == roleid)
          {
!             result |= aiprivs & mask;
              if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
                  return result;
          }
*************** aclmask(const Acl *acl, Oid roleid, Oid
*** 1162,1167 ****
--- 1167,1177 ----
      for (i = 0; i < num; i++)
      {
          AclItem    *aidata = &aidat[i];
+         AclMode        aiprivs = aidata->ai_privs;
+
+         /* fixup ACL_SELECT_FOR_UPDATE */
+         if (aiprivs & ACL_UPDATE)
+             aiprivs |= ACL_SELECT_FOR_UPDATE;

          if (aidata->ai_grantee == ACL_ID_PUBLIC ||
              aidata->ai_grantee == roleid)
*************** aclmask(const Acl *acl, Oid roleid, Oid
*** 1170,1176 ****
          if ((aidata->ai_privs & remaining) &&
              has_privs_of_role(roleid, aidata->ai_grantee))
          {
!             result |= aidata->ai_privs & mask;
              if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
                  return result;
              remaining = mask & ~result;
--- 1180,1186 ----
          if ((aidata->ai_privs & remaining) &&
              has_privs_of_role(roleid, aidata->ai_grantee))
          {
!             result |= aiprivs & mask;
              if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
                  return result;
              remaining = mask & ~result;

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
FORUPDATE/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>

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 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

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>

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 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

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/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

On Mon, Apr 20, 2009 at 12:48 PM, Martijn van Oosterhout
<kleptog@svana.org> wrote:
> 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.

Not really, because SE-PostgreSQL introduces its own analogue of
setuid/security definer, which happens to be called "trusted
procedure", and you can do the same darn thing.

The issue at hand is foreign key constraints.  Standard PostgreSQL
checks those constraints as the table owner using the table owner's
credentials.  The question is whether there's some reason why
SE-PostgreSQL shouldn't do the same.

...Robert

Gregory Stark wrote:
> 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?

It is also too radical goal for SELinux. :-)

SELinux intends to prevent "unexpected" privilege escalation, but it does
not mean to eliminate setuids. The "unexpected" means the actions are not
explicitly allowed in the security policy.

The SELinux privileges mechanism works orthogonally with DAC mechanism.
If a user runs a root-setuid program, he can get full-controllable privileges
in the DAC rules, but SELinux checks his privileges from different aspect to
mask unallowed privilges due to the MAC rules.
Thus, SELinux makes its decision based on only security contexts, independent
from user identifier and other factors.

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

Robert Haas wrote:
> On Mon, Apr 20, 2009 at 12:48 PM, Martijn van Oosterhout
> <kleptog@svana.org> wrote:
>> 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.
> 
> Not really, because SE-PostgreSQL introduces its own analogue of
> setuid/security definer, which happens to be called "trusted
> procedure", and you can do the same darn thing.
> 
> The issue at hand is foreign key constraints.  Standard PostgreSQL
> checks those constraints as the table owner using the table owner's
> credentials.  The question is whether there's some reason why
> SE-PostgreSQL shouldn't do the same.

It is an idea to be worth considering, I think.

The current foreign-key implementation internally invokes secondary
queries to check whether the given tuples satisfies the constraints,
or not. SE-PostgreSQL checks any given queries and permissions on
the required database objects, so the secondary queries are also
checked.

However, the way to achive foreign-key feature is purely depending
on the implementation of DBMS, so we might be able to consider it
as a part of system internal stuff.
For example, if PostgreSQL implemented the foreign-key feature using
hard-wired functions, we don't need to apply checks here because of
it is not a query.
One possible compromise is to skip SE- checks during foreign-key
checks. I'll consider the idea a bit more.

BTW, as we have discussed many times, SE-PostgreSQL does not intend
to prevent unpriv users to infer existence of invisible tuples.
So, this design changes will not be a headach for me.

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

KaiGai Kohei wrote:
> Robert Haas wrote:
>> On Mon, Apr 20, 2009 at 12:48 PM, Martijn van Oosterhout
>> <kleptog@svana.org> wrote:
>>> 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.
>> Not really, because SE-PostgreSQL introduces its own analogue of
>> setuid/security definer, which happens to be called "trusted
>> procedure", and you can do the same darn thing.
>>
>> The issue at hand is foreign key constraints.  Standard PostgreSQL
>> checks those constraints as the table owner using the table owner's
>> credentials.  The question is whether there's some reason why
>> SE-PostgreSQL shouldn't do the same.
> 
> It is an idea to be worth considering, I think.

Robert, currently I could not find semantics breaks in your suggestion.
I plan to update SE- implementation to skip checks during foreign-key
constraints and add a new SE- permission: "reference" which allows
to set up fereign-keys.

Thanks,

> The current foreign-key implementation internally invokes secondary
> queries to check whether the given tuples satisfies the constraints,
> or not. SE-PostgreSQL checks any given queries and permissions on
> the required database objects, so the secondary queries are also
> checked.
> 
> However, the way to achive foreign-key feature is purely depending
> on the implementation of DBMS, so we might be able to consider it
> as a part of system internal stuff.
> For example, if PostgreSQL implemented the foreign-key feature using
> hard-wired functions, we don't need to apply checks here because of
> it is not a query.
> One possible compromise is to skip SE- checks during foreign-key
> checks. I'll consider the idea a bit more.
> 
> BTW, as we have discussed many times, SE-PostgreSQL does not intend
> to prevent unpriv users to infer existence of invisible tuples.
> So, this design changes will not be a headach for me.
> 
> Thanks,
> --
> OSS Platform Development Division, NEC
> KaiGai Kohei <kaigai@ak.jp.nec.com>

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

2009/4/21 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Robert, currently I could not find semantics breaks in your suggestion.
> I plan to update SE- implementation to skip checks during foreign-key
> constraints and add a new SE- permission: "reference" which allows
> to set up fereign-keys.

Sounds good!  I hope that works out for you!

...Robert

Robert Haas wrote:
> 2009/4/21 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> Robert, currently I could not find semantics breaks in your suggestion.
>> I plan to update SE- implementation to skip checks during foreign-key
>> constraints and add a new SE- permission: "reference" which allows
>> to set up fereign-keys.
> 
> Sounds good!  I hope that works out for you!

Robert,
I found a concern for the approach apart from the original matter.

When a FK constraint has ON UPDATE CASCADE rule and the security
policy allows someone to update the PK table, it can allow them
to update read-only FK table.
It might or not be a matter depending on the point of view.
If we consider setting up of FK constraint is a very sensitive
operation as much as loaing C-libraries, it can be fair enough.
(Because we assume SE-PostgreSQL does not checks actions from
internal features which are installed by limited number of DBAs.)

However, I don't think CREATE TABLE with FK constraint should be
restricted to the limited number of DBAs. It will give demerits
from the aspects of usability.
So, I reconsidered that SE-PostgreSQL should checks secondary
queries in FK constraints as the older version doing.

Fortunately, the original matter can be solved in other approach.
This change does not give us any design impact.

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