ExecutorCheckPerms() hook
in [PgSql]
|
From: Stephen Frost on 24 May 2010 15:11 * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > 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. Rather than construct a complicated API for this DML activity, why don't we just make ExecCheckRTPerms available for DoCopy to use? It seems like we could move ExecCheckRTPerms() to acl.c without too much trouble. acl.h already includes parsenodes.h and has knowledge of RangeVar's. Once DoCopy is using that, this issue resolves itself with the hook that Robert already wrote up. > * 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. I'm not really thrilled with how RI_Initial_Check() does it's own permissions checking and then calls SPI expecting things to 'just work'. Not sure if there's some way we could handle failure from SPI, or, if it was changed to call ExecCheckRTPerms() instead, how it would handle failure cases from there. One possible solution would be to have an additional option to ExecCheckRTPerms() which asks it to just check and return false if there's a problem, rather than unconditionally calling aclcheck_error() whenever it finds a problem. Using the same function for both the initial check in RI_Initial_Check() and then from SPI would eliminate issues where those two checks disagree for some reason, which would be good in the general case. > BTW, I guess the reason why permissions on attributes are not checked here is > that we missed it at v8.4 development. Indeed, but at the same time, this looks to be a 'fail-safe' situation. Basically, this is checking table-level permissions, which, if you have, gives you sufficient rights to SELECT against the table (any column). What this isn't doing is allowing the option of column-level permissions to be sufficient for this requirement. That's certainly an oversight in the column-level permissions handling (sorry about that), but it's not horrible- there's a workaround if RI_Initial_Check returns false already anyway. Basically, if you are using column-level privs, and you have necessary rights to do this w/ those permissions (but don't have table-level rights), it's not going to be as fast as it could be. > The most part of the checker function is cut & paste from ExecCheckRTEPerms(), > but its arguments are modified for easy invocation from other functions. As mentioned above, it seems like this would be better the other way- have the callers build RangeTbl's and then call ExecCheckRTPerms(). It feels like that approach might be more 'future-proof' as well. Thanks, Stephen
From: KaiGai Kohei on 24 May 2010 20:37 (2010/05/24 22:18), Robert Haas wrote: > 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? Sorry, after more detailed consideration, it seems to me the permission checks in RI_Initial_Check() and its fallback mechanism are nonsense. See the following commands. postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT At that time, the 'ymj' has ownership and REFERENCES permissions on both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return and the fallback-seqscan will run. But, postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: permission denied for relation pk_tbl CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" From more careful observation of the code, the validateForeignKeyConstraint() also calls RI_FKey_check_ins() for each scanned tuples, but it also execute SELECT statement using SPI_*() interface internally. In other words, both of execution paths entirely require SELECT permission to validate new FK constraint. I think we need a new SPI_*() interface which allows to run the given query without any permission checks, because these queries are purely internal stuff, so we can trust the query is harmless. Is there any other idea? >> 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. > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding in the future. If it is ugly to deploy checker functions in separated dir/files, I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms(). It also suggest us where the checker functions should be deployed on the upcoming DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database for example? Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- 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 21:08 (2010/05/25 4:11), Stephen Frost wrote: > * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >> 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. > > Rather than construct a complicated API for this DML activity, why don't > we just make ExecCheckRTPerms available for DoCopy to use? It seems > like we could move ExecCheckRTPerms() to acl.c without too much trouble. > acl.h already includes parsenodes.h and has knowledge of RangeVar's. > Once DoCopy is using that, this issue resolves itself with the hook that > Robert already wrote up. We have two options; If the checker function takes the list of RangeTblEntry, it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, if the checker function takes arguments in my patch, it will be comfortable to DoCopy(), but ExecCheckRTPerms(). In my patch, it takes 6 arguments, but we can reference all of them from the given RangeTblEntry. On the other hand, if DoCopy() has to set up a pseudo RangeTblEntry to call checker function, it entirely needs to set up similar or a bit large number of variables. As I replied in the earlier message, it may be an idea to rename and change the definition of ExecCheckRTEPerms() without moving it anywhere. >> * 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. > > I'm not really thrilled with how RI_Initial_Check() does it's own > permissions checking and then calls SPI expecting things to 'just work'. > Not sure if there's some way we could handle failure from SPI, or, if it > was changed to call ExecCheckRTPerms() instead, how it would handle > failure cases from there. One possible solution would be to have an > additional option to ExecCheckRTPerms() which asks it to just check and > return false if there's a problem, rather than unconditionally calling > aclcheck_error() whenever it finds a problem. > > Using the same function for both the initial check in RI_Initial_Check() > and then from SPI would eliminate issues where those two checks disagree > for some reason, which would be good in the general case. Sorry, I missed the fallback path also needs SELECT permissions because validateForeignKeyConstraint() calls RI_FKey_check_ins() which entirely tries to execute SELECT statement using SPI_*() interface. But, it is a separate issue from the DML permission reworks. It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. What we really want to do here is validation of the new FK constraints. So, the validateForeignKeyConstraint() intends to provide a fall-back code when table-level permission is denied, doesn't it? In this case, we should execute the secondary query without permission checks, because the permissions of ALTER TABLE is already checked, and we can trust the given query is harmless. >> BTW, I guess the reason why permissions on attributes are not checked here is >> that we missed it at v8.4 development. > > Indeed, but at the same time, this looks to be a 'fail-safe' situation. > Basically, this is checking table-level permissions, which, if you have, > gives you sufficient rights to SELECT against the table (any column). > What this isn't doing is allowing the option of column-level permissions > to be sufficient for this requirement. That's certainly an oversight in > the column-level permissions handling (sorry about that), but it's not > horrible- there's a workaround if RI_Initial_Check returns false already > anyway. Yes, it is harmless expect for performances in a corner-case. If user have table-level permissions, it does not need to check column- level permissions, even if it is implemented. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- 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: Stephen Frost on 24 May 2010 21:13 KaiGai, * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > postgres=# ALTER TABLE pk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# ALTER TABLE fk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; > REVOKE > postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; > GRANT > > At that time, the 'ymj' has ownership and REFERENCES permissions on > both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return > and the fallback-seqscan will run. But, ymj may be considered an 'owner' on that table, but in this case, it doesn't have SELECT rights on it. Now, you might argue that we should assume that the owner has SELECT rights (since they're granted by default), even if they've been revoked, but that's a whole separate issue. > postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: permission denied for relation pk_tbl > CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" I think you've got another issue here that's not related. Perhaps something wrong with a patch you've applied? Otherwise, what version of PG is this? Using 8.2, 8.3, 8.4 and a recent git checkout, I get: postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT postgres=# SET ROLE ymj; SET postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ALTER TABLE postgres=> > I think we need a new SPI_*() interface which allows to run the given query > without any permission checks, because these queries are purely internal stuff, > so we can trust the query is harmless. > Is there any other idea? Yeah, I don't see that going anywhere... > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding > in the future. If it is ugly to deploy checker functions in separated dir/files, > I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms(). No, this is not a service of the executor, putting it in execMain.c does not make any sense. > It also suggest us where the checker functions should be deployed on the upcoming > DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database > for example? We'll worry about DDL when we get there. It won't be any time soon. I would strongly recommend that you concentrate on building an SELinux module using the hook function that Robert wrote or none of this is going to end up going anywhere. If and when we find other places which handle DML and need adjustment, we can identify how to handle them at that time. Hopefully by the time we're comfortable with DML, some of the DDL permissions checking rework will have been done and how to move forward with allowing external security modules to handle that will be clear. Thanks, Stephen
From: Stephen Frost on 24 May 2010 21:27
* KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > We have two options; If the checker function takes the list of RangeTblEntry, > it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, > if the checker function takes arguments in my patch, it will be comfortable > to DoCopy(), but ExecCheckRTPerms(). > > In my patch, it takes 6 arguments, but we can reference all of them from > the given RangeTblEntry. On the other hand, if DoCopy() has to set up > a pseudo RangeTblEntry to call checker function, it entirely needs to set > up similar or a bit large number of variables. I don't know that it's really all that difficult to set up an RT in DoCopy or RI_Initial_Check(). In my opinion, those are the strange or corner cases- not the Executor code, through which all 'regular' DML is done. It makes me wonder if COPY shouldn't have been implemented using the Executor instead, but that's, again, a completely separate topic. It wasn't, but it wants to play like it operates in the same kind of way as INSERT, so it needs to pick up the slack. > As I replied in the earlier message, it may be an idea to rename and change > the definition of ExecCheckRTEPerms() without moving it anywhere. And, again, I don't see that as a good idea at all. > >> * RI_Initial_Check() > > It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. I agree with this- my proposal would address this in a way whih would be guaranteed to be consistant: by using the same code path to do both checks. I'm still not thrilled with how RI_Initial_Check() works, but rewriting that isn't part of this. > In this case, we should execute the secondary query without permission checks, > because the permissions of ALTER TABLE is already checked, and we can trust > the given query is harmless. I dislike the idea of providing a new SPI interfance (on the face of it), and also dislike the idea of having a "skip all permissions checking" option for anything which resembles SPI. I would rather ask the question of if it really makes sense to use SPI to check FKs as they're being added, but we're not going to solve that issue here. Thanks, Stephen |