Thread: Re: [GENERAL] Strange permission problem regarding pg_settings
[ please respect moving of thread to pg-hackers ]
Joe Conway <mail@joeconway.com> writes:
> ISTM that we want the relations in the un-rewritten query checked based
> on the basis of the user referencing the rule and for the modes used in
> the un-rewritten query -- suggesting the change need be reverted.
Reverting the change will bring back the bug for which it was created.
It does seem though that we have an inadequate model of how to perform
permission checks. In particular, the "write" flag bit in RTEs is
context dependent: it can mean insert, update, or delete permission
depending on the surrounding command.
The problem the earlier bug report identified is really that when an RTE
is copied from one query to another, the meaning of its "write" flag bit
changes --- incorrectly --- if the new query is of a different type.
I thought when making that patch that we could make an end-run around
this problem by zeroing out the flag bit, but what we're now realizing
is that that leaves us with no check at all in some scenarios (because
the original query will be dropped completely when INSTEAD is specified).
I begin to think that the only real solution is to change the RTE
representation to identify the exact permission bits to be checked for
each entry (say, replace the read and write booleans with a permission
bitmask). Then a view reference specifying INSERT permission check
could be copied into an UPDATE query without changing its permission
semantics.
This would be a fairly extensive change though. Does anyone see an
easier way?
Also, does anyone see a case where it would be correct for the checked
permission to change when an RTE is copied to a query of a different
type?
regards, tom lane
Tom Lane wrote: > Reverting the change will bring back the bug for which it was created. > It does seem though that we have an inadequate model of how to perform > permission checks. In particular, the "write" flag bit in RTEs is > context dependent: it can mean insert, update, or delete permission > depending on the surrounding command. Sorry if I'm being thick, but what of this? > regression=> insert into table1 values (1);> NOTICE: relOid = 1245674> NOTICE: userid = 101> NOTICE: operation = CMD_INSERT>NOTICE: relOid = 1245674> NOTICE: userid = 101> NOTICE: operation = CMD_UPDATE> ERROR: table1: permissiondenied>> regression=> select oid, relname from pg_class where relname like 'table%';> oid | relname> ---------+---------> 1245674 | table1> 1245676 | table2> (2 rows) Given how rules are supposed to work, the first check looks correct: INSERT on table1 checked as pleb, userid = 101 But the second check is incorrect, not because of the mode being checked, but because of the reloid and userid. The second check should be: UPDATE on table2 checked as postgres, userid= 1 So why doesn't the second rte refer to table2 and userid=1? Joe
Joe Conway <mail@joeconway.com> writes:
> Sorry if I'm being thick, but what of this?
This is exactly what I'm talking about. The rtable for a query
generated by a rule is the concatenation of the original query's rtable
and the rule query's rtable. Therefore the RTE for table1 appears
twice, once in the original INSERT query and once in the generated
UPDATE query (even though the UPDATE query does not actually use that
RTE in this case). This would be okay if the RTE's write permission
flag were not context-dependent, but because it is, we have a problem.
The patch I put into 7.3.3 assumed that we could just suppress
permission checks on the copied RTEs, but if the original query is
getting dropped due to INSTEAD, we really need to carry out those
permission checks in the generated query --- there is no place else.
So AFAICS we must make the permission checks non-context-dependent.
regards, tom lane
Tom Lane wrote: > This is exactly what I'm talking about. The rtable for a query > generated by a rule is the concatenation of the original query's rtable > and the rule query's rtable. Therefore the RTE for table1 appears > twice, once in the original INSERT query and once in the generated > UPDATE query (even though the UPDATE query does not actually use that > RTE in this case). This would be okay if the RTE's write permission > flag were not context-dependent, but because it is, we have a problem. OK, that makes more sense now. But why isn't table2 also in the rule query's rtable? Joe
Joe Conway <mail@joeconway.com> writes:
> OK, that makes more sense now. But why isn't table2 also in the rule
> query's rtable?
It is, but you errored out before getting to it.
regards, tom lane
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >>OK, that makes more sense now. But why isn't table2 also in the rule >>query's rtable? > > It is, but you errored out before getting to it. The fog has finally started lifting, I think. Why wouldn't we force checkAsUser to the rule owner in the copied RTEs, similar to the rest of the rule query? It makes sense in that the rule query could possibly use the RTE (although as you pointed out it doesn't in this case), and therefore the permission check should be the same, no? Joe
Joe Conway <mail@joeconway.com> writes:
> Why wouldn't we force checkAsUser to the rule owner in the copied RTEs,
> similar to the rest of the rule query?
Because it would be the wrong check. We need to check that the rule
caller has permissions on the view for whatever he originally tried
to do (ie, the type of the original query that referenced the view).
In the non-INSTEAD case, this check will be redundant with a check
applied when the original query is executed ... but in the INSTEAD case,
it isn't redundant.
> It makes sense in that the rule
> query could possibly use the RTE (although as you pointed out it doesn't
> in this case), and therefore the permission check should be the same, no?
No; it's possible for the amalgamated query to include references to
tables that are referenced only in the original query and nowhere in the
text of the rule. (This is obviously possible right now, since we just
take the union of the two rtables and don't make any effort to discard
unreferenced RTEs ... but I think it could happen even if we did discard
unreferenced RTEs, because conditions from the original query get pushed
into the rule and might reference tables that the rule text doesn't
mention.) Checking such tables for rule-owner access would be wrong;
they have to be checked for access by the rule caller.
regards, tom lane
Tom Lane wrote: > No; it's possible for the amalgamated query to include references to > tables that are referenced only in the original query and nowhere in the > text of the rule. (This is obviously possible right now, since we just > take the union of the two rtables and don't make any effort to discard > unreferenced RTEs ... but I think it could happen even if we did discard > unreferenced RTEs, because conditions from the original query get pushed > into the rule and might reference tables that the rule text doesn't > mention.) Checking such tables for rule-owner access would be wrong; > they have to be checked for access by the rule caller. OK, so the permission check performed on the original query RTEs, while executing the rule query is: 1) redundant for non-INSTEAD cases and 2) wrong if the original query and rule query are different modes The patch at the root of this discussion eliminates both issues, but leaves us with no check at all in the INSTEAD case. Is there any way to do the permission checks on the original query in the INSTEAD case, even though the query itself will never be executed? Joe
Joe Conway <mail@joeconway.com> writes:
> The patch at the root of this discussion eliminates both issues, but
> leaves us with no check at all in the INSTEAD case. Is there any way to
> do the permission checks on the original query in the INSTEAD case, even
> though the query itself will never be executed?
I thought about that a bit, but I'm not sure it's any less invasive a
change. Currently the original query doesn't get shipped to the
executor at all, if it's suppressed by INSTEAD. In place of the
original query, the rewriter would have to generate some kind of
"no-op query" that the executor would run as far as checking the rtable
permissions for, and then stop. (There is a CMD_NOTHING value of
CmdType, but I don't think it's supported anywhere outside the rule
rewriter, and the rewriter's semantics for it may not be quite right
anyway.) Adding a new CmdType would have cascading effects in a bunch
of places. While it's probably not hard to fix any one of them
individually, overall this doesn't look any easier or cleaner than
extending the RTE permissions flag representation.
The only argument I can see in favor of doing it that way would be that
by avoiding a change in RTE representation, we would have a patch that
could be applied in the 7.4.* series. (If we change RTEs then we can
only fix it in 7.5, because the change to stored rule representation
would force initdb.) But the patch would be extensive enough that I'd
be pretty hesitant to apply it to 7.4.* anyway.
What might be the best compromise is to fix it properly in 7.5 and
revert the previous patch in 7.4.*. That would restore correct checking
of view permissions for all but the case where a rule substitutes a
different query type; which is a bug that we had for a very long time
before anyone noticed, so maybe leaving it unfixed in 7.4.* is okay.
regards, tom lane