ExecutorCheckPerms() hook
in [PgSql]
|
From: Tom Lane on 20 May 2010 12:32 Robert Haas <robertmhaas(a)gmail.com> writes: > In yesterday's development meeting, we talked about the possibility of > a basic SE-PostgreSQL implementation that checks permissions only for > DML. Greg Smith offered the opinion that this could provide much of > the benefit of SE-PostgreSQL for many users, while being much simpler. > In fact, SE-PostgreSQL would need to get control in just one place: > ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick > patch showing how we could add a hook here to let a hypothetical > SE-PostgreSQL module get control in the relevant place. The attached > patch also includes a toy contrib module showing how it could be used > to enforce arbitrary security policy. Hm, I think you need to ignore RT entries that have no requiredPerms bits set. (Not that it matters too much, unless you were proposing to actually commit this contrib module.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 20 May 2010 12:37 On Thu, May 20, 2010 at 12:32 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas(a)gmail.com> writes: >> In yesterday's development meeting, we talked about the possibility of >> a basic SE-PostgreSQL implementation that checks permissions only for >> DML. Greg Smith offered the opinion that this could provide much of >> the benefit of SE-PostgreSQL for many users, while being much simpler. >> In fact, SE-PostgreSQL would need to get control in just one place: >> ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick >> patch showing how we could add a hook here to let a hypothetical >> SE-PostgreSQL module get control in the relevant place. The attached >> patch also includes a toy contrib module showing how it could be used >> to enforce arbitrary security policy. > > Hm, I think you need to ignore RT entries that have no requiredPerms > bits set. (Not that it matters too much, unless you were proposing to > actually commit this contrib module.) Well, that's an easy change - just out of curiosity, how do we end up with RT entries with no requiredPerm bits set? As for committing it, I would definitely like to commit the actual hook. If we want the hook without the contrib module that's OK with me, although I generally feel it's useful to have examples of how hooks can be used, which is why I took the time to produce a working example. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 20 May 2010 13:33 Robert Haas <robertmhaas(a)gmail.com> writes: > On Thu, May 20, 2010 at 12:32 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: >> Hm, I think you need to ignore RT entries that have no requiredPerms >> bits set. �(Not that it matters too much, unless you were proposing to >> actually commit this contrib module.) > Well, that's an easy change - just out of curiosity, how do we end up > with RT entries with no requiredPerm bits set? Inheritance child tables look like that now, per the discussion awhile back that a SELECT on the parent shouldn't require any particular permission on the individual child tables. IIRC there are some other cases involving views too, but those are probably just optimizations (ie not do duplicate permissions checks) rather than something that would result in a user-visible behavioral issue. > As for committing it, I would definitely like to commit the actual > hook. If we want the hook without the contrib module that's OK with > me, although I generally feel it's useful to have examples of how > hooks can be used, which is why I took the time to produce a working > example. +1 on committing the hook. As for the contrib module, it doesn't strike me that there's much of a use-case for it as is. I think it's enough that it's available in the -hackers archives. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 24 May 2010 02:48 (2010/05/21 1:14), Robert Haas wrote: > In yesterday's development meeting, we talked about the possibility of > a basic SE-PostgreSQL implementation that checks permissions only for > DML. Greg Smith offered the opinion that this could provide much of > the benefit of SE-PostgreSQL for many users, while being much simpler. > In fact, SE-PostgreSQL would need to get control in just one place: > ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick > patch showing how we could add a hook here to let a hypothetical > SE-PostgreSQL module get control in the relevant place. The attached > patch also includes a toy contrib module showing how it could be used > to enforce arbitrary security policy. > > I don't think that this by itself would be quite enough framework for > a minimal SE-PostgreSQL implementation - for that, you'd probably need > an object-labeling facility in core which SE-PostgreSQL could leverage > - or else some other way to determine which the label associated with > a given object - but I think that plus this would be enough. I'd like to point out two more points are necessary to be considered for DML permission checks in addition to ExecCheckRTPerms(). * DoCopy() Although DoCopy() is called from standard_ProcessUtility(), it performs as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on the copied table or attributes, similar to what ExecCheckRTEPerms() doing. * RI_Initial_Check() RI_Initial_Check() is a function called on ALTER TABLE command to add FK constraints between two relations. The permission to execute this ALTER TABLE command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(), so it does not affect anything on the DML permission reworks. When we add a new FK constraint, both of the existing FK and PK relations have to satify the new constraint. So, RI_Initial_Check() tries to check whether the PK relation has corresponding tuples to FK relation, or not. Then, it tries to execute a secondary query using SPI_*() functions, if no access violations are expected. Otherwise, it scans the FK relation with per tuple checks sequentionally (see, validateForeignKeyConstraint()), but slow. If we have an external security provider which will deny accesses on the FK/PK relation, but the default PG checks allows it, the RI_Initial_Check() tries to execute secondary SELECT statement, then it raises an access violation error, although we are already allowed to execute ALTER TABLE statement. Therefore, we also need to check DML permissions at RI_Initial_Check() to avoid unexpected access violation error, prior to the secondary query. BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. The attached patch provides a common checker function of DML, and modifies ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker function instead of individual ACL checks. The most part of the checker function is cut & paste from ExecCheckRTEPerms(), but its arguments are modified for easy invocation from other functions. extern bool check_dml_permissions(Oid relOid, Oid userId, AclMode requiredPerms, Bitmapset *selCols, Bitmapset *modCols, bool abort); Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Robert Haas on 24 May 2010 09:18
2010/5/24 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > BTW, I guess the reason why permissions on attributes are not checked here is > that we missed it at v8.4 development. That's a little worrying. Can you construct and post a test case where this results in a user-visible failure in CVS HEAD? > The attached patch provides a common checker function of DML, and modifies > ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker > function instead of individual ACL checks. This looks pretty sane to me, although I have not done a full review. I am disinclined to create a whole new directory for it. I think the new function should go in src/backend/catalog/aclchk.c and be declared in src/include/utils/acl.h. If that sounds reasonable to you, please revise and post an updated patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |