Thread: copy.c handling for RLS is insecure
In DoCopy, some RLS-specific code constructs a SelectStmt to handle the case where COPY TO is invoked on an RLS-protected relation. But I think this step is bogus in two ways: /* Build FROM clause */ from = makeRangeVar(NULL, RelationGetRelationName(rel), 1); First, because relations are schema objects, there could be multiple relations with the same name. The RangeVar might end up referring to a different one of those objects than the user originally specified. That seems like it could be surprising at best and a security vulnerability at worst. So at the very list I think this needs to pass the schema name as well as the relation name. I'm not 100% sure it's OK even then, because what about a concurrent rename of the schema? Second, the third argument to makeRangeVar is a parse location, and I believe it it is conventional to use -1, rather than 1, when no location can be identified. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote: > In DoCopy, some RLS-specific code constructs a SelectStmt to handle > the case where COPY TO is invoked on an RLS-protected relation. But I > think this step is bogus in two ways: > > /* Build FROM clause */ > from = makeRangeVar(NULL, RelationGetRelationName(rel), 1); > > First, because relations are schema objects, there could be multiple > relations with the same name. The RangeVar might end up referring to > a different one of those objects than the user originally specified. Argh. That's certainly no good. It should just be using the RangeVar relation passed in from CopyStmt, no? We don't have to address the case where it's NULL (tho we should perhaps Assert(), just to be sure), as that would only happen in the COPY select_with_parens ... production and this is only for the normal 'COPY relname' case. > That seems like it could be surprising at best and a security > vulnerability at worst. So at the very list I think this needs to > pass the schema name as well as the relation name. I'm not 100% sure > it's OK even then, because what about a concurrent rename of the > schema? Hmm, that's certainly an interesting point, but I'm trying to work out how this is different from normal COPY..? pg_analyze_and_rewrite() happens for both cases down in BeginCopy(). > Second, the third argument to makeRangeVar is a parse location, and I > believe it it is conventional to use -1, rather than 1, when no > location can be identified. Err, you're right, but I think we should just eliminate the entire makeRangeVar() call, and then the location would be correctly set too. Thanks! Stephen
Stephen Frost <sfrost@snowman.net> writes: > * Robert Haas (robertmhaas@gmail.com) wrote: >> First, because relations are schema objects, there could be multiple >> relations with the same name. The RangeVar might end up referring to >> a different one of those objects than the user originally specified. > Argh. That's certainly no good. It should just be using the RangeVar > relation passed in from CopyStmt, no? No, it shouldn't be doing that either. That would imply looking up the relation a second time, and then you have a race condition against concurrent renames (the same type of security bug Robert spent a great deal of time on, not so long ago). Once you've identified the target relation by OID, nothing else later in the command should be doing a fresh lookup by name. Period. If you've got APIs in here that depend on passing RangeVars to identify relations, those APIs are broken and need to be changed. regards, tom lane
On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> In DoCopy, some RLS-specific code constructs a SelectStmt to handle >> the case where COPY TO is invoked on an RLS-protected relation. But I >> think this step is bogus in two ways: >> >> /* Build FROM clause */ >> from = makeRangeVar(NULL, RelationGetRelationName(rel), 1); >> >> First, because relations are schema objects, there could be multiple >> relations with the same name. The RangeVar might end up referring to >> a different one of those objects than the user originally specified. > > Argh. That's certainly no good. It should just be using the RangeVar > relation passed in from CopyStmt, no? I don't think that's adequate. You can't do a RangeVar-to-OID translation, use the resulting OID for some security-relevant decision, and then repeat the RangeVar-to-OID translation and hope you get the same answer. That's what led to CVE-2014-0062, fixed by commit 5f173040e324f6c2eebb90d86cf1b0cdb5890f0a. I can't work out off-hand whether the issue is exploitable in this instance, but it sure seems like a bad idea to rely on it not being so. > We don't have to address the case > where it's NULL (tho we should perhaps Assert(), just to be sure), as > that would only happen in the COPY select_with_parens ... production and > this is only for the normal 'COPY relname' case. The antecedent of "it" in "the case where it's NULL" is unclear to me. > Hmm, that's certainly an interesting point, but I'm trying to work out > how this is different from normal COPY..? pg_analyze_and_rewrite() > happens for both cases down in BeginCopy(). As far as I can see, the previous code only looked up any given name once. If you got a relation name, DoCopy() looked it up, and then BeginCopy() references it only by the passed-down Relation descriptor; if you got a query, DoCopy() ignores it, and then BeginCopy. All of which is fine, at least AFAICS; if you think otherwise, that should be reported to pgsql-security. The problem with your code is that you start with a relation name (and thus look it up in DoCopy()) and then construct a query (which causes the name to be looked up again when the query is passed to pg_analyze_and_rewrite() from BeginCopy()) -- and the lookup might not get the same answer both times. That is, not to put to fine a point on it, bad news. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I left out a few words there. On Mon, Oct 6, 2014 at 3:07 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Hmm, that's certainly an interesting point, but I'm trying to work out >> how this is different from normal COPY..? pg_analyze_and_rewrite() >> happens for both cases down in BeginCopy(). > > As far as I can see, the previous code only looked up any given name > once. If you got a relation name, DoCopy() looked it up, and then > BeginCopy() references it only by the passed-down Relation descriptor; > if you got a query, DoCopy() ignores it, and then BeginCopy. ...passes it to pg_analyze_and_rewrite(), which looks up any names it contains. > All of > which is fine, at least AFAICS; if you think otherwise, that should be > reported to pgsql-security. The problem with your code is that you > start with a relation name (and thus look it up in DoCopy()) and then > construct a query (which causes the name to be looked up again when > the query is passed to pg_analyze_and_rewrite() from BeginCopy()) -- > and the lookup might not get the same answer both times. That is, not > to put to fine a point on it, bad news. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote: > On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Argh. That's certainly no good. It should just be using the RangeVar > > relation passed in from CopyStmt, no? > > I don't think that's adequate. You can't do a RangeVar-to-OID > translation, use the resulting OID for some security-relevant > decision, and then repeat the RangeVar-to-OID translation and hope you > get the same answer. That's what led to CVE-2014-0062, fixed by > commit 5f173040e324f6c2eebb90d86cf1b0cdb5890f0a. I can't work out > off-hand whether the issue is exploitable in this instance, but it > sure seems like a bad idea to rely on it not being so. Ok, makes sense, I see now. > > We don't have to address the case > > where it's NULL (tho we should perhaps Assert(), just to be sure), as > > that would only happen in the COPY select_with_parens ... production and > > this is only for the normal 'COPY relname' case. > > The antecedent of "it" in "the case where it's NULL" is unclear to me. Sorry, wasn't clear- was referring to when 'relation' in CopyStmt is NULL (which happens when the production with the sub-select is used). > > Hmm, that's certainly an interesting point, but I'm trying to work out > > how this is different from normal COPY..? pg_analyze_and_rewrite() > > happens for both cases down in BeginCopy(). > > As far as I can see, the previous code only looked up any given name > once. If you got a relation name, DoCopy() looked it up, and then > BeginCopy() references it only by the passed-down Relation descriptor; > if you got a query, DoCopy() ignores it, and then BeginCopy. All of > which is fine, at least AFAICS; if you think otherwise, that should be > reported to pgsql-security. Yeah, that's correct. I suppose there's some possible risk of things changing between when you parse the query and when it actually gets analyzed and rewritten, but that's not a security risk per-se.. > The problem with your code is that you > start with a relation name (and thus look it up in DoCopy()) and then > construct a query (which causes the name to be looked up again when > the query is passed to pg_analyze_and_rewrite() from BeginCopy()) -- > and the lookup might not get the same answer both times. That is, not > to put to fine a point on it, bad news. Right, ok. Will need to think a bit more about how to address this such that we aren't doing another lookup... Thanks! Stephen
On Mon, Oct 06, 2014 at 03:15:25PM -0400, Stephen Frost wrote: > > As far as I can see, the previous code only looked up any given name > > once. If you got a relation name, DoCopy() looked it up, and then > > BeginCopy() references it only by the passed-down Relation descriptor; > > if you got a query, DoCopy() ignores it, and then BeginCopy. All of > > which is fine, at least AFAICS; if you think otherwise, that should be > > reported to pgsql-security. > > Yeah, that's correct. I suppose there's some possible risk of things > changing between when you parse the query and when it actually gets > analyzed and rewritten, but that's not a security risk per-se.. I'm not sure I understand. If that change violates an access control, it's a security risk /per se/, as you put it. Are you saying that such changes, even though they might be bugs, categorically couldn't violate an access control? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David,
On Monday, October 6, 2014, David Fetter <david@fetter.org> wrote:
On Monday, October 6, 2014, David Fetter <david@fetter.org> wrote:
On Mon, Oct 06, 2014 at 03:15:25PM -0400, Stephen Frost wrote:
> > As far as I can see, the previous code only looked up any given name
> > once. If you got a relation name, DoCopy() looked it up, and then
> > BeginCopy() references it only by the passed-down Relation descriptor;
> > if you got a query, DoCopy() ignores it, and then BeginCopy. All of
> > which is fine, at least AFAICS; if you think otherwise, that should be
> > reported to pgsql-security.
>
> Yeah, that's correct. I suppose there's some possible risk of things
> changing between when you parse the query and when it actually gets
> analyzed and rewritten, but that's not a security risk per-se..
I'm not sure I understand. If that change violates an access control,
it's a security risk /per se/, as you put it.
The case I was referring to doesn't violate an access control. I was merely pointing out that things can change between when the query is submitted by the user (or even later, during parse analysis) and when we actually resolve names to OIDs.
Thanks,
Stephen
Robert, * Stephen Frost (sfrost@snowman.net) wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: > > In DoCopy, some RLS-specific code constructs a SelectStmt to handle > > the case where COPY TO is invoked on an RLS-protected relation. But I > > think this step is bogus in two ways: > > > > /* Build FROM clause */ > > from = makeRangeVar(NULL, RelationGetRelationName(rel), 1); > > > > First, because relations are schema objects, there could be multiple > > relations with the same name. The RangeVar might end up referring to > > a different one of those objects than the user originally specified. > > Argh. That's certainly no good. It should just be using the RangeVar > relation passed in from CopyStmt, no? We don't have to address the case > where it's NULL (tho we should perhaps Assert(), just to be sure), as > that would only happen in the COPY select_with_parens ... production and > this is only for the normal 'COPY relname' case. Alright, I've done the change to use the RangeVar from CopyStmt, but also added a check wherein we verify that the relation's OID returned from the planned query is the same as the relation's OID that we did the RLS check on- if they're different, we throw an error. Please let me know if there are any remaining concerns. Thanks! Stephen
On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost@snowman.net> wrote: > Alright, I've done the change to use the RangeVar from CopyStmt, but > also added a check wherein we verify that the relation's OID returned > from the planned query is the same as the relation's OID that we did the > RLS check on- if they're different, we throw an error. Please let me > know if there are any remaining concerns. That's clearly an improvement, but I'm not sure it's water-tight. What if the name that originally referenced a table ended up referencing a view? Then you could get list_length(plan->relationOids) != 1. (And, in that case, I also wonder if you could get eval_const_expressions() to do evil things on your behalf while planning.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote: > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost@snowman.net> wrote: > > Alright, I've done the change to use the RangeVar from CopyStmt, but > > also added a check wherein we verify that the relation's OID returned > > from the planned query is the same as the relation's OID that we did the > > RLS check on- if they're different, we throw an error. Please let me > > know if there are any remaining concerns. > > That's clearly an improvement, but I'm not sure it's water-tight. > What if the name that originally referenced a table ended up > referencing a view? Then you could get > list_length(plan->relationOids) != 1. I'll test it out and see what happens. Certainly a good question and if there's an issue there then I'll get it addressed. > (And, in that case, I also wonder if you could get > eval_const_expressions() to do evil things on your behalf while > planning.) If it can be made to reference a view then there's an issue as the view might include a function call itself which is provided by the attacker.. I'm not sure that we have to really worry about anything more complicated than that. Clearly, if we found a relation originally then we need that same relation with the same OID after the conversion to a query. Thanks, Stephen
On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: > > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost@snowman.net> wrote: > > > Alright, I've done the change to use the RangeVar from CopyStmt, but > > > also added a check wherein we verify that the relation's OID returned > > > from the planned query is the same as the relation's OID that we did the > > > RLS check on- if they're different, we throw an error. Please let me > > > know if there are any remaining concerns. Here is the check in question (added in commit 143b39c): plan = planner(query, 0, NULL); /* * If we were passed in a relid, make sure we got the same one back * after planning out the query. It's possiblethat it changed * between when we checked the policies on the table and decided to * use a query and now. */ if (queryRelId != InvalidOid) { Oid relid = linitial_oid(plan->relationOids); /* * There should only be one relationOid in this case, since we * will only get here when we havechanged the command for the * user from a "COPY relation TO" to "COPY (SELECT * FROM * relation) TO",to allow row level security policies to be * applied. */ Assert(list_length(plan->relationOids)== 1); if (relid != queryRelId) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("relation referenced by COPY statement has changed"))); } > > That's clearly an improvement, but I'm not sure it's water-tight. > > What if the name that originally referenced a table ended up > > referencing a view? Then you could get > > list_length(plan->relationOids) != 1. > > I'll test it out and see what happens. Certainly a good question and > if there's an issue there then I'll get it addressed. Yes, it can be made to reference a view and trip the assertion. > > (And, in that case, I also wonder if you could get > > eval_const_expressions() to do evil things on your behalf while > > planning.) > > If it can be made to reference a view then there's an issue as the view > might include a function call itself which is provided by the attacker.. Indeed. As the parenthetical remark supposed, the check happens too late to prevent a security breach. planner() has run eval_const_expressions(), executing code of the view owner's choosing. > Clearly, if we found a relation originally then we need that same > relation with the same OID after the conversion to a query. That is necessary but not sufficient. CREATE RULE can convert a table to a view without changing the OID, thereby fooling the check. Test procedure: -- as superuser (or createrole) create user blackhat; create user alice; -- as blackhat begin; create table exploit_rls_copy (c int); alter table exploit_rls_copy enable row level security; grant select on exploit_rls_copy to public; commit; -- as alice -- first, set breakpoint on BeginCopy copy exploit_rls_copy to stdout; -- as blackhat begin; create or replace function leak() returns int immutable as $$beginraise notice 'in leak()'; return 7; end$$ language plpgsql; create rule "_RETURN" as on select to exploit_rls_copy do insteadselect leak() as c from (values (0)) dummy; commit; -- Release breakpoint. leak() function call happens. After that, assertion -- fires if enabled. ERROR does not fire in any case.
Noah, * Noah Misch (noah@leadboat.com) wrote: > On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote: > > * Robert Haas (robertmhaas@gmail.com) wrote: > > > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost@snowman.net> wrote: > > > > Alright, I've done the change to use the RangeVar from CopyStmt, but > > > > also added a check wherein we verify that the relation's OID returned > > > > from the planned query is the same as the relation's OID that we did the > > > > RLS check on- if they're different, we throw an error. Please let me > > > > know if there are any remaining concerns. > > Here is the check in question (added in commit 143b39c): > > plan = planner(query, 0, NULL); > > /* > * If we were passed in a relid, make sure we got the same one back > * after planning out the query. It's possible that it changed > * between when we checked the policies on the table and decided to > * use a query and now. > */ > if (queryRelId != InvalidOid) > { > Oid relid = linitial_oid(plan->relationOids); > > /* > * There should only be one relationOid in this case, since we > * will only get here when we have changed the command for the > * user from a "COPY relation TO" to "COPY (SELECT * FROM > * relation) TO", to allow row level security policies to be > * applied. > */ > Assert(list_length(plan->relationOids) == 1); > > if (relid != queryRelId) > ereport(ERROR, > (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg("relation referenced by COPY statement has changed"))); > } > > > > That's clearly an improvement, but I'm not sure it's water-tight. > > > What if the name that originally referenced a table ended up > > > referencing a view? Then you could get > > > list_length(plan->relationOids) != 1. > > > > I'll test it out and see what happens. Certainly a good question and > > if there's an issue there then I'll get it addressed. > > Yes, it can be made to reference a view and trip the assertion. Ok. We can certainly make that assertion be a run-time consideration instead, though I'm not thrilled with that being the only safe-guard. > > > (And, in that case, I also wonder if you could get > > > eval_const_expressions() to do evil things on your behalf while > > > planning.) > > > > If it can be made to reference a view then there's an issue as the view > > might include a function call itself which is provided by the attacker.. > > Indeed. As the parenthetical remark supposed, the check happens too late to > prevent a security breach. planner() has run eval_const_expressions(), > executing code of the view owner's choosing. Right. > > Clearly, if we found a relation originally then we need that same > > relation with the same OID after the conversion to a query. > > That is necessary but not sufficient. CREATE RULE can convert a table to a > view without changing the OID, thereby fooling the check. Test procedure: Ugh, yes, rules would cause a problem for this.. > -- as superuser (or createrole) > create user blackhat; > create user alice; > > -- as blackhat > begin; > create table exploit_rls_copy (c int); > alter table exploit_rls_copy enable row level security; > grant select on exploit_rls_copy to public; > commit; > > -- as alice > -- first, set breakpoint on BeginCopy > copy exploit_rls_copy to stdout; > > -- as blackhat > begin; > create or replace function leak() returns int immutable as $$begin > raise notice 'in leak()'; return 7; end$$ language plpgsql; > create rule "_RETURN" as on select to exploit_rls_copy do instead > select leak() as c from (values (0)) dummy; > commit; > > -- Release breakpoint. leak() function call happens. After that, assertion > -- fires if enabled. ERROR does not fire in any case. Thanks for pointing this out. I'll look into it. Stephen
Noah, * Noah Misch (noah@leadboat.com) wrote: > On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote: > > * Robert Haas (robertmhaas@gmail.com) wrote: > > > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost@snowman.net> wrote: > > > > Alright, I've done the change to use the RangeVar from CopyStmt, but > > > > also added a check wherein we verify that the relation's OID returned > > > > from the planned query is the same as the relation's OID that we did the > > > > RLS check on- if they're different, we throw an error. Please let me > > > > know if there are any remaining concerns. > > Here is the check in question (added in commit 143b39c): > > plan = planner(query, 0, NULL); > > /* > * If we were passed in a relid, make sure we got the same one back > * after planning out the query. It's possible that it changed > * between when we checked the policies on the table and decided to > * use a query and now. > */ > if (queryRelId != InvalidOid) > { > Oid relid = linitial_oid(plan->relationOids); > > /* > * There should only be one relationOid in this case, since we > * will only get here when we have changed the command for the > * user from a "COPY relation TO" to "COPY (SELECT * FROM > * relation) TO", to allow row level security policies to be > * applied. > */ > Assert(list_length(plan->relationOids) == 1); > > if (relid != queryRelId) > ereport(ERROR, > (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg("relation referenced by COPY statement has changed"))); > } > > > > That's clearly an improvement, but I'm not sure it's water-tight. > > > What if the name that originally referenced a table ended up > > > referencing a view? Then you could get > > > list_length(plan->relationOids) != 1. > > > > I'll test it out and see what happens. Certainly a good question and > > if there's an issue there then I'll get it addressed. > > Yes, it can be made to reference a view and trip the assertion. There's a different issue with that Assertion, actually- if you've got an RLS policy which references another table directly in a subselect then you'll also trip it up, but more below.. > > > (And, in that case, I also wonder if you could get > > > eval_const_expressions() to do evil things on your behalf while > > > planning.) > > > > If it can be made to reference a view then there's an issue as the view > > might include a function call itself which is provided by the attacker.. > > Indeed. As the parenthetical remark supposed, the check happens too late to > prevent a security breach. planner() has run eval_const_expressions(), > executing code of the view owner's choosing. It happens too late to prevent the user from running code specified by the table owner- but there's not a solution to that risk except to set 'row_security = off' and use a mechanism which doesn't respect on-select rules, which is only COPY, right? A normal SELECT will certainly fire the on-select rule. > > Clearly, if we found a relation originally then we need that same > > relation with the same OID after the conversion to a query. > > That is necessary but not sufficient. CREATE RULE can convert a table to a > view without changing the OID, thereby fooling the check. Test procedure: It's interesting to consider that COPY purportedly operates under the SELECT privilege, yet fails to respect on-select rules. Having spent a bit of time thinking about this now, it occurs to me that we've drifted from the original concern and are now trying to solve an insolvable issue here. We're not trying to prevent against an attacker who owns the table we're going to COPY and wants to get us to run code they've written- that can happen by simply having RLS and that's why it's not enabled by default for superusers and why we have 'row_security = off', which pg_dump sets by default. The original issue that Robert brought up was the concern about multiple lookups of RangeVar->Oid. That was a problem in the CVE highlighted and the original/current coding because we weren't doing fully qualified lookups based on the originally found and locked Oid. I'm trying to figure out why weren't not simply doing that here. After a bit of discussion with Andres, my thinking on this is to do the following: - Fully qualify the name based on the opened relation - Keep the initial lock on the relation throughout - Remove the Assert() (other relations can be pulled in by RLS) - Keep the OID check, shouldn't hurt to have it Thoughts? Thanks! Stephen
On Wed, Jul 08, 2015 at 10:55:47AM -0400, Stephen Frost wrote: > It's interesting to consider that COPY purportedly operates under the > SELECT privilege, yet fails to respect on-select rules. In released branches, COPY consistently refuses to operate directly on a view. (There's no (longer?) such thing as a table with an ON SELECT rule. CREATE RULE "_RETURN" AS ON SELECT ... converts a table to a view.) > Having spent a bit of time thinking about this now, it occurs to me that > we've drifted from the original concern and are now trying to solve an > insolvable issue here. We're not trying to prevent against an attacker > who owns the table we're going to COPY and wants to get us to run code > they've written- that can happen by simply having RLS and that's why > it's not enabled by default for superusers and why we have > 'row_security = off', which pg_dump sets by default. The problem I wanted to see solved was the fact that, by running a DDL command concurrent with a "COPY rel TO" command, you can make the COPY read from a view. That is not possible in any serial execution of COPY with DDL. Now, you make a good point that before this undesirable outcome can happen, the user issuing the COPY command will have already trusted the roles that can pass owner checks for "rel". That probably makes this useless as an attack tool. Nonetheless, I don't want "COPY rejects views" to soften into "COPY rejects views, except in XYZ race condition." > After a bit of discussion with Andres, my thinking on this is to do the > following: > > - Fully qualify the name based on the opened relation > - Keep the initial lock on the relation throughout > - Remove the Assert() (other relations can be pulled in by RLS) That's much better. We have considerable experience with designs like that. > - Keep the OID check, shouldn't hurt to have it What benefit is left? The planner does not promise any particular order within relationOids, and an order change could make false alarms here.
On 2015-07-09 01:28:28 -0400, Noah Misch wrote: > > - Keep the OID check, shouldn't hurt to have it > > What benefit is left? A bit of defense in depth. We execute user defined code in COPY (e.g. BEFORE triggers). That user defined code could very well replace the relation. Now I think right now that'd happen late enough, so the second lookup already happened. But a bit more robust defense against that sounds good to me.
Noah, Andres, * Andres Freund (andres@anarazel.de) wrote: > On 2015-07-09 01:28:28 -0400, Noah Misch wrote: > > > - Keep the OID check, shouldn't hurt to have it > > > > What benefit is left? > > A bit of defense in depth. We execute user defined code in COPY > (e.g. BEFORE triggers). That user defined code could very well replace > the relation. Now I think right now that'd happen late enough, so the > second lookup already happened. But a bit more robust defense against > that sounds good to me. Attached patch keeps the relation locked, fully qualifies it when building up the query, and uses list_member_oid() to check that the relation's OID ends up in the resulting relationOids list (to address Noah's point that the planner doesn't guarantee the ordering; I doubt that list will ever be more than a few entries long). Also removes the misguided Assert(). Barring objections, I'll commit this (and backpatch to 9.5) tomorrow. Thanks! Stephen
Attachment
All, * Stephen Frost (sfrost@snowman.net) wrote: > * Andres Freund (andres@anarazel.de) wrote: > > On 2015-07-09 01:28:28 -0400, Noah Misch wrote: > > > > - Keep the OID check, shouldn't hurt to have it > > > > > > What benefit is left? > > > > A bit of defense in depth. We execute user defined code in COPY > > (e.g. BEFORE triggers). That user defined code could very well replace > > the relation. Now I think right now that'd happen late enough, so the > > second lookup already happened. But a bit more robust defense against > > that sounds good to me. > > Attached patch keeps the relation locked, fully qualifies it when > building up the query, and uses list_member_oid() to check that the > relation's OID ends up in the resulting relationOids list (to address > Noah's point that the planner doesn't guarantee the ordering; I doubt > that list will ever be more than a few entries long). > > Also removes the misguided Assert(). > > Barring objections, I'll commit this (and backpatch to 9.5) tomorrow. Apologies for not pushing this before I left on vacation. I've done so now. Thanks! Stephen