Обсуждение: WITH CHECK and Column-Level Privileges
All,
Through continued testing, we've discovered an issue in the WITH CHECK OPTION code when it comes to column-level
privilegeswhich impacts 9.4.
It's pretty straight-forward, thankfully, but:
postgres=# create view myview
postgres-# with (security_barrier = true,
postgres-# check_option = 'local')
postgres-# as select * from passwd where username = current_user;
CREATE VIEW
postgres=# grant select (username) on myview to public;
GRANT
postgres=# grant update on myview to public;
GRANT
postgres=# set role alice;
SET
postgres=> update myview set username = 'joe';
ERROR: new row violates WITH CHECK OPTION for "myview"
DETAIL: Failing row contains (joe, abc).
Note that the entire failing tuple is returned, including the 'password' column, even though the 'alice' user does not
haveselect rights on that column.
The detail information is useful for debugging, but I believe we have to remove it from the error message.
Barring objections, and in the hopes of getting the next beta out the door soon, I'll move forward with this change
andback-patch it to 9.4 after a few hours (or I can do it tomorrow if there is contention; I don't know what, if any,
specificplans there are for the next beta, just that it's hopefully 'soon'). To hopefully shorten the discussion about
9.4,I'll clarify that I'm happy to discuss trying to re-work this in 9.5 to include what columns the user should be
ableto see (if there is consensus that we should do that at all) but I don't see that as a change which should be
back-patchedto 9.4 at this point given that we're trying to get it out the door.
Thanks!
Stephen
On 09/26/2014 05:20 PM, Stephen Frost wrote: > All, > > Through continued testing, we've discovered an issue in the > WITH CHECK OPTION code when it comes to column-level privileges > which impacts 9.4. > > It's pretty straight-forward, thankfully, but: > > postgres=# create view myview > postgres-# with (security_barrier = true, > postgres-# check_option = 'local') > postgres-# as select * from passwd where username = current_user; > CREATE VIEW > postgres=# grant select (username) on myview to public; > GRANT > postgres=# grant update on myview to public; > GRANT > postgres=# set role alice; > SET > postgres=> update myview set username = 'joe'; > ERROR: new row violates WITH CHECK OPTION for "myview" > DETAIL: Failing row contains (joe, abc). > > Note that the entire failing tuple is returned, including the > 'password' column, even though the 'alice' user does not have select > rights on that column. Is there similar problems with unique or exclusion constraints? > The detail information is useful for debugging, but I believe we have > to remove it from the error message. > > Barring objections, and in the hopes of getting the next beta out the > door soon, I'll move forward with this change and back-patch it to > 9.4 after a few hours What exactly are you going to commit? Did you forget to attach a patch? > (or I can do it tomorrow if there is contention; > I don't know what, if any, specific plans there are for the next beta, > just that it's hopefully 'soon'). Probably would be wise to wait 'till tomorrow; there's no need to rush this. - Heikki
* Heikki Linnakangas (hlinnakangas@vmware.com) wrote:
> On 09/26/2014 05:20 PM, Stephen Frost wrote:
> > Note that the entire failing tuple is returned, including the
> > 'password' column, even though the 'alice' user does not have select
> > rights on that column.
>
> Is there similar problems with unique or exclusion constraints?
That's certainly an excellent question.. I'll have to go look.
> > The detail information is useful for debugging, but I believe we have
> > to remove it from the error message.
> >
> > Barring objections, and in the hopes of getting the next beta out the
> > door soon, I'll move forward with this change and back-patch it to
> > 9.4 after a few hours
>
> What exactly are you going to commit? Did you forget to attach a patch?
I had, but now it seems like it might be insufficient anyway.. Let me
go poke at the unique constraint question.
> >(or I can do it tomorrow if there is contention;
> > I don't know what, if any, specific plans there are for the next beta,
> > just that it's hopefully 'soon').
>
> Probably would be wise to wait 'till tomorrow; there's no need to rush this.
Sure, works for me.
Would be great to get an idea of when the next beta is going to be cut,
if there's been any thought to that..
Thanks,
Stephen
* Stephen Frost (sfrost@snowman.net) wrote:
> > Is there similar problems with unique or exclusion constraints?
>
> That's certainly an excellent question.. I'll have to go look.
Looks like there is an issue here with CHECK constraints and NOT NULL
constraints, yes. The uniqueness check complains about the key already
existing and returns the key, but I don't think that's actually a
problem- to get that to happen you have to specify the new key and
that's what is returned.
Looks like this goes all the way back to column-level privileges and was
just copied into WithCheckOptions from ExecConstraints. :(
Here's the test case I used:
create table passwd (username text primary key, password text);
grant select (username) on passwd to public;
grant update on passwd to public;
insert into passwd values ('abc','hidden');
insert into passwd values ('def','hidden2');
set role alice;
update passwd set username = 'def';
update passwd set username = null;
Results in:
postgres=> update passwd set username = 'def';
ERROR: duplicate key value violates unique constraint "passwd_pkey"
DETAIL: Key (username)=(def) already exists.
postgres=> update passwd set username = null;
ERROR: null value in column "username" violates not-null constraint
DETAIL: Failing row contains (null, hidden).
Thoughts?
Thanks,
Stephen
* Stephen Frost (sfrost@snowman.net) wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > > Is there similar problems with unique or exclusion constraints?
> >
> > That's certainly an excellent question.. I'll have to go look.
>
> Looks like there is an issue here with CHECK constraints and NOT NULL
> constraints, yes. The uniqueness check complains about the key already
> existing and returns the key, but I don't think that's actually a
> problem- to get that to happen you have to specify the new key and
> that's what is returned.
Yeah, I take that back. If there is a composite key involved then you
can run into the same issue- you update one of the columns to a
conflicting value and get back the entire key, including columns you
shouldn't be allowed to see.
Ugh.
Thanks,
Stephen
* Stephen Frost (sfrost@snowman.net) wrote:
> > Looks like there is an issue here with CHECK constraints and NOT NULL
> > constraints, yes. The uniqueness check complains about the key already
> > existing and returns the key, but I don't think that's actually a
> > problem- to get that to happen you have to specify the new key and
> > that's what is returned.
>
> Yeah, I take that back. If there is a composite key involved then you
> can run into the same issue- you update one of the columns to a
> conflicting value and get back the entire key, including columns you
> shouldn't be allowed to see.
Alright, attached is a patch which addresses this by checking if the
user has SELECT rights on the relation first and, if so, the existing
error message is returned with the full row as usual, but if not, then
the row data is omitted.
Also attached is a patch for 9.4 which does the same, but also removes
the row reporting (completely) from the WITH CHECK case. It could be
argued that it would be helpful to have it there also, perhaps, but I'm
not convinced at this point that it's really valuable- and we'd have to
think about how this would work with views (permission on the view? or
on the table underneath? what if there is more than one?, etc).
Thanks,
Stephen
Вложения
On 27 September 2014 14:33, Stephen Frost <sfrost@snowman.net> wrote: >> Yeah, I take that back. If there is a composite key involved then you >> can run into the same issue- you update one of the columns to a >> conflicting value and get back the entire key, including columns you >> shouldn't be allowed to see. > > Alright, attached is a patch which addresses this by checking if the > user has SELECT rights on the relation first and, if so, the existing > error message is returned with the full row as usual, but if not, then > the row data is omitted. > Seems reasonable. > Also attached is a patch for 9.4 which does the same, but also removes > the row reporting (completely) from the WITH CHECK case. It could be > argued that it would be helpful to have it there also, perhaps, but I'm > not convinced at this point that it's really valuable- and we'd have to > think about how this would work with views (permission on the view? or > on the table underneath? what if there is more than one?, etc). > Well by that point in the code, the query has been rewritten and the row being reported is for the underlying table, so it's the table's ACLs that should apply. In fact not all the values from the table are even necessarily exported in the view, so its ACLs are not appropriate to the values being reported. I suspect that in many/most real-world cases, the user will only have permissions on the view, not on the underlying table, in which case it won't work anyway. So +1 for just removing it. Regards, Dean
On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Also attached is a patch for 9.4 which does the same, but also removes >> the row reporting (completely) from the WITH CHECK case. It could be >> argued that it would be helpful to have it there also, perhaps, but I'm >> not convinced at this point that it's really valuable- and we'd have to >> think about how this would work with views (permission on the view? or >> on the table underneath? what if there is more than one?, etc). > > Well by that point in the code, the query has been rewritten and the > row being reported is for the underlying table, so it's the table's > ACLs that should apply. In fact not all the values from the table are > even necessarily exported in the view, so its ACLs are not appropriate > to the values being reported. I suspect that in many/most real-world > cases, the user will only have permissions on the view, not on the > underlying table, in which case it won't work anyway. So +1 for just > removing it. Wait, what? I think it's clear that the right thing to report would be the columns that the user had permission to see via the view. The decision as to what is visible in the error message has to be consistent with the underlying permissions structure. Removing the detail altogether is OK security-wise because it's just a subset of what the user can be allowed to see, but I think checking the table permissions can never be right. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >> Also attached is a patch for 9.4 which does the same, but also removes
> >> the row reporting (completely) from the WITH CHECK case. It could be
> >> argued that it would be helpful to have it there also, perhaps, but I'm
> >> not convinced at this point that it's really valuable- and we'd have to
> >> think about how this would work with views (permission on the view? or
> >> on the table underneath? what if there is more than one?, etc).
> >
> > Well by that point in the code, the query has been rewritten and the
> > row being reported is for the underlying table, so it's the table's
> > ACLs that should apply. In fact not all the values from the table are
> > even necessarily exported in the view, so its ACLs are not appropriate
> > to the values being reported. I suspect that in many/most real-world
> > cases, the user will only have permissions on the view, not on the
> > underlying table, in which case it won't work anyway. So +1 for just
> > removing it.
>
> Wait, what?
>
> I think it's clear that the right thing to report would be the columns
> that the user had permission to see via the view. The decision as to
> what is visible in the error message has to be consistent with the
> underlying permissions structure. Removing the detail altogether is
> OK security-wise because it's just a subset of what the user can be
> allowed to see, but I think checking the table permissions can never
> be right.
What I believe Dean was getting at is that if the user has direct
permissions on the underlying table then they could see the row by
querying the table directly too, so it'd be alright to report the detail
in the error. He then further points out that you're not likely to be
using a view over top of a table which you have direct access to, so
this is not a very interesting case.
In the end, it sounds like we all agree that the right approach is to
simply remove this detail and avoid the issue entirely.
Thanks,
Stephen
On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> >> Also attached is a patch for 9.4 which does the same, but also removes >> >> the row reporting (completely) from the WITH CHECK case. It could be >> >> argued that it would be helpful to have it there also, perhaps, but I'm >> >> not convinced at this point that it's really valuable- and we'd have to >> >> think about how this would work with views (permission on the view? or >> >> on the table underneath? what if there is more than one?, etc). >> > >> > Well by that point in the code, the query has been rewritten and the >> > row being reported is for the underlying table, so it's the table's >> > ACLs that should apply. In fact not all the values from the table are >> > even necessarily exported in the view, so its ACLs are not appropriate >> > to the values being reported. I suspect that in many/most real-world >> > cases, the user will only have permissions on the view, not on the >> > underlying table, in which case it won't work anyway. So +1 for just >> > removing it. >> >> Wait, what? >> >> I think it's clear that the right thing to report would be the columns >> that the user had permission to see via the view. The decision as to >> what is visible in the error message has to be consistent with the >> underlying permissions structure. Removing the detail altogether is >> OK security-wise because it's just a subset of what the user can be >> allowed to see, but I think checking the table permissions can never >> be right. > > What I believe Dean was getting at is that if the user has direct > permissions on the underlying table then they could see the row by > querying the table directly too, so it'd be alright to report the detail > in the error. Hmm, yeah. True. > He then further points out that you're not likely to be > using a view over top of a table which you have direct access to, so > this is not a very interesting case. > > In the end, it sounds like we all agree that the right approach is to > simply remove this detail and avoid the issue entirely. Well, I think that's an acceptable approach from the point of view of fixing the security exposure, but it's far from ideal. Good error messages are important for usability. I can live with this as a short-term fix, but in the long run I strongly believe we should try to do better. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
> Well, I think that's an acceptable approach from the point of view of
> fixing the security exposure, but it's far from ideal. Good error
> messages are important for usability. I can live with this as a
> short-term fix, but in the long run I strongly believe we should try
> to do better.
It certainly wouldn't be hard to add the same check around the WITH
OPTION case that's around my proposed solution for the other issues-
just check for SELECT rights on the underlying table. Another question
is if we could/should limit this to the UPDATE case. With the INSERT
case, any columns not provided by the user would be filled out by
defaults, which can likely be seen in the catalog, or the functions in
the catalog for the defaults or for any triggers might be able to be
run by the user executing the INSERT anyway to see what would have been
used in the resulting row. I'm not completely convinced there's no risk
there though..
Thoughts?
Thanks,
Stephen
On 29 September 2014 16:46, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> Well, I think that's an acceptable approach from the point of view of >> fixing the security exposure, but it's far from ideal. Good error >> messages are important for usability. I can live with this as a >> short-term fix, but in the long run I strongly believe we should try >> to do better. > Yes agreed, error messages are important, and longer term it would be better to report on the specific columns the user has access to (for all constraints), rather than the all-or-nothing approach of the current patch. However, for WCOs, I don't think the executor has the information it needs to work out how to do that because it doesn't even know which view the user updated, because it's not necessarily the same one as failed the WCO. > It certainly wouldn't be hard to add the same check around the WITH > OPTION case that's around my proposed solution for the other issues- > just check for SELECT rights on the underlying table. I guess that would work well for RLS, since the user would typically have SELECT rights on the table they're updating, but I'm not convinced it would do much good for auto-updatable views. Another question > is if we could/should limit this to the UPDATE case. With the INSERT > case, any columns not provided by the user would be filled out by > defaults, which can likely be seen in the catalog, or the functions in > the catalog for the defaults or for any triggers might be able to be > run by the user executing the INSERT anyway to see what would have been > used in the resulting row. I'm not completely convinced there's no risk > there though.. > I think it's conceivable that a trigger could populate a column hidden to the user with some confidential information, possibly pulled from another table that the user doesn't have access to, so the fix has to apply to INSERTs as well as UPDATEs. Regards, Dean
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 29 September 2014 16:46, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> Well, I think that's an acceptable approach from the point of view of
> >> fixing the security exposure, but it's far from ideal. Good error
> >> messages are important for usability. I can live with this as a
> >> short-term fix, but in the long run I strongly believe we should try
> >> to do better.
>
> Yes agreed, error messages are important, and longer term it would be
> better to report on the specific columns the user has access to (for
> all constraints), rather than the all-or-nothing approach of the
> current patch.
If the user only has column-level privileges on the table then I'm not
really sure how useful the detail will be.
> However, for WCOs, I don't think the executor has the
> information it needs to work out how to do that because it doesn't
> even know which view the user updated, because it's not necessarily
> the same one as failed the WCO.
That's certainly an issue also.
> > It certainly wouldn't be hard to add the same check around the WITH
> > OPTION case that's around my proposed solution for the other issues-
> > just check for SELECT rights on the underlying table.
>
> I guess that would work well for RLS, since the user would typically
> have SELECT rights on the table they're updating, but I'm not
> convinced it would do much good for auto-updatable views.
I've been focusing on the 9.4 and back-branches concern, but as for RLS,
if we're going to try and include the detail in this case then I'd
suggest we do so only if the user has SELECT rights and RLS is disabled
on the relation. Otherwise, we'd have to check that the row being
returned actually passes the SELECT policy. I'm already not really
thrilled that we are changing error message results based on user
permissions, and that seems like it'd be worse.
> > Another question
> > is if we could/should limit this to the UPDATE case. With the INSERT
> > case, any columns not provided by the user would be filled out by
> > defaults, which can likely be seen in the catalog, or the functions in
> > the catalog for the defaults or for any triggers might be able to be
> > run by the user executing the INSERT anyway to see what would have been
> > used in the resulting row. I'm not completely convinced there's no risk
> > there though..
> >
>
> I think it's conceivable that a trigger could populate a column hidden
> to the user with some confidential information, possibly pulled from
> another table that the user doesn't have access to, so the fix has to
> apply to INSERTs as well as UPDATEs.
What do you think about returning just what the user provided in the
first place in both of these cases..? I'm not quite sure what that
would look like in the UPDATE case but for INSERT (and COPY) it would be
the subset of columns being inserted and the values originally provided.
That may not be what the actual conflict is due to, as there could be
before triggers changing things in the middle, or the conflict could be
on default values, but at least the input row could be identified and
there wouldn't be this information leak risk. Not sure how difficult
that would be to implement though.
Thoughts?
Thanks!
Stephen
On 30 September 2014 16:52, Stephen Frost <sfrost@snowman.net> wrote: > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote: >> On 29 September 2014 16:46, Stephen Frost <sfrost@snowman.net> wrote: >> > * Robert Haas (robertmhaas@gmail.com) wrote: >> >> Well, I think that's an acceptable approach from the point of view of >> >> fixing the security exposure, but it's far from ideal. Good error >> >> messages are important for usability. I can live with this as a >> >> short-term fix, but in the long run I strongly believe we should try >> >> to do better. >> >> Yes agreed, error messages are important, and longer term it would be >> better to report on the specific columns the user has access to (for >> all constraints), rather than the all-or-nothing approach of the >> current patch. > > If the user only has column-level privileges on the table then I'm not > really sure how useful the detail will be. > One of the main things that detail is useful for is identifying the failing row in a multi-row update. In most real-world cases, I would expect the column-level privileges to include the table's PK, so the detail would meet that requirement. In fact the column-level privileges would pretty much have to include sufficient columns to usefully identify rows, otherwise updates wouldn't be practical. >> However, for WCOs, I don't think the executor has the >> information it needs to work out how to do that because it doesn't >> even know which view the user updated, because it's not necessarily >> the same one as failed the WCO. > > That's certainly an issue also. > >> > It certainly wouldn't be hard to add the same check around the WITH >> > OPTION case that's around my proposed solution for the other issues- >> > just check for SELECT rights on the underlying table. >> >> I guess that would work well for RLS, since the user would typically >> have SELECT rights on the table they're updating, but I'm not >> convinced it would do much good for auto-updatable views. > > I've been focusing on the 9.4 and back-branches concern, but as for RLS, > if we're going to try and include the detail in this case then I'd > suggest we do so only if the user has SELECT rights and RLS is disabled > on the relation. Huh? If RLS is disabled, isn't the check option also disabled? Otherwise, we'd have to check that the row being > returned actually passes the SELECT policy. I'm already not really > thrilled that we are changing error message results based on user > permissions, and that seems like it'd be worse. > That's one of the things I never liked about allowing different RLS policies for different commands --- the idea that the user can UPDATE a row that they can't even SELECT just doesn't make sense to me. >> > Another question >> > is if we could/should limit this to the UPDATE case. With the INSERT >> > case, any columns not provided by the user would be filled out by >> > defaults, which can likely be seen in the catalog, or the functions in >> > the catalog for the defaults or for any triggers might be able to be >> > run by the user executing the INSERT anyway to see what would have been >> > used in the resulting row. I'm not completely convinced there's no risk >> > there though.. >> > >> >> I think it's conceivable that a trigger could populate a column hidden >> to the user with some confidential information, possibly pulled from >> another table that the user doesn't have access to, so the fix has to >> apply to INSERTs as well as UPDATEs. > > What do you think about returning just what the user provided in the > first place in both of these cases..? I'm not quite sure what that > would look like in the UPDATE case but for INSERT (and COPY) it would be > the subset of columns being inserted and the values originally provided. > That may not be what the actual conflict is due to, as there could be > before triggers changing things in the middle, or the conflict could be > on default values, but at least the input row could be identified and > there wouldn't be this information leak risk. Not sure how difficult > that would be to implement though. > I could see that working for INSERTs, but for UPDATEs I don't think that would be very useful in practice, because the columns most likely to be useful for identifying the failing row (e.g., key columns) are also the columns least likely to have been updated. Regards, Dean
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 30 September 2014 16:52, Stephen Frost <sfrost@snowman.net> wrote:
> > If the user only has column-level privileges on the table then I'm not
> > really sure how useful the detail will be.
>
> One of the main things that detail is useful for is identifying the
> failing row in a multi-row update. In most real-world cases, I would
> expect the column-level privileges to include the table's PK, so the
> detail would meet that requirement. In fact the column-level
> privileges would pretty much have to include sufficient columns to
> usefully identify rows, otherwise updates wouldn't be practical.
That may or may not be true- the user needs sufficient information to
identify a row, but that doesn't mean they have access to all columns
make up a unique constraint. It's not uncommon to have a surrogate key
for identifying the rows and then an independent uniqueness constraint
on some natural key for the table, which the user may not have access
to.
> > I've been focusing on the 9.4 and back-branches concern, but as for RLS,
> > if we're going to try and include the detail in this case then I'd
> > suggest we do so only if the user has SELECT rights and RLS is disabled
> > on the relation.
>
> Huh? If RLS is disabled, isn't the check option also disabled?
Not quite. If RLS is disabled on the relation then the RLS policies
don't add to the with check option, but a view overtop of a RLS table
might have a with check option. That's the situation which I was
getting at when it comes to the with-check option. The other cases of
constraint violation which we're discussing here would need to be
handled also though, so it's not just the with-check case.
> Otherwise, we'd have to check that the row being
> > returned actually passes the SELECT policy. I'm already not really
> > thrilled that we are changing error message results based on user
> > permissions, and that seems like it'd be worse.
>
> That's one of the things I never liked about allowing different RLS
> policies for different commands --- the idea that the user can UPDATE
> a row that they can't even SELECT just doesn't make sense to me.
The reason for having the per-command RLS policies was that you might
want a different policy applied for different commands (ie: you can see
all rows, but can only update your row); the ability to define a policy
which allows you to UPDATE rows which are not visible to a normal SELECT
is also available through that but isn't really the reason for the
capability. That said, I agree it isn't common to define policies that
way, but not unheard of either.
> > What do you think about returning just what the user provided in the
> > first place in both of these cases..? I'm not quite sure what that
> > would look like in the UPDATE case but for INSERT (and COPY) it would be
> > the subset of columns being inserted and the values originally provided.
> > That may not be what the actual conflict is due to, as there could be
> > before triggers changing things in the middle, or the conflict could be
> > on default values, but at least the input row could be identified and
> > there wouldn't be this information leak risk. Not sure how difficult
> > that would be to implement though.
>
> I could see that working for INSERTs, but for UPDATEs I don't think
> that would be very useful in practice, because the columns most likely
> to be useful for identifying the failing row (e.g., key columns) are
> also the columns least likely to have been updated.
I'm not sure that I follow this- if you're not updating the key columns
then you're not likely to be having a conflict due to them...
Thanks,
Stephen
On 30 September 2014 20:17, Stephen Frost <sfrost@snowman.net> wrote: > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote: >> On 30 September 2014 16:52, Stephen Frost <sfrost@snowman.net> wrote: >> > If the user only has column-level privileges on the table then I'm not >> > really sure how useful the detail will be. >> >> One of the main things that detail is useful for is identifying the >> failing row in a multi-row update. In most real-world cases, I would >> expect the column-level privileges to include the table's PK, so the >> detail would meet that requirement. In fact the column-level >> privileges would pretty much have to include sufficient columns to >> usefully identify rows, otherwise updates wouldn't be practical. > > That may or may not be true- the user needs sufficient information to > identify a row, but that doesn't mean they have access to all columns > make up a unique constraint. It's not uncommon to have a surrogate key > for identifying the rows and then an independent uniqueness constraint > on some natural key for the table, which the user may not have access > to. > True, but then the surrogate key would be included in the error details which would allow the failing row to be identified. >> > What do you think about returning just what the user provided in the >> > first place in both of these cases..? I'm not quite sure what that >> > would look like in the UPDATE case but for INSERT (and COPY) it would be >> > the subset of columns being inserted and the values originally provided. >> > That may not be what the actual conflict is due to, as there could be >> > before triggers changing things in the middle, or the conflict could be >> > on default values, but at least the input row could be identified and >> > there wouldn't be this information leak risk. Not sure how difficult >> > that would be to implement though. >> >> I could see that working for INSERTs, but for UPDATEs I don't think >> that would be very useful in practice, because the columns most likely >> to be useful for identifying the failing row (e.g., key columns) are >> also the columns least likely to have been updated. > > I'm not sure that I follow this- if you're not updating the key columns > then you're not likely to be having a conflict due to them... > The constraint violation could well be due to updating a non-key column such as a column with a NOT NULL constraint on it, in which case only including that column in the error detail wouldn't do much good -- you'd want to include the key columns if possible. Regards, Dean
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 30 September 2014 20:17, Stephen Frost <sfrost@snowman.net> wrote:
> > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> >> One of the main things that detail is useful for is identifying the
> >> failing row in a multi-row update. In most real-world cases, I would
> >> expect the column-level privileges to include the table's PK, so the
> >> detail would meet that requirement. In fact the column-level
> >> privileges would pretty much have to include sufficient columns to
> >> usefully identify rows, otherwise updates wouldn't be practical.
> >
> > That may or may not be true- the user needs sufficient information to
> > identify a row, but that doesn't mean they have access to all columns
> > make up a unique constraint. It's not uncommon to have a surrogate key
> > for identifying the rows and then an independent uniqueness constraint
> > on some natural key for the table, which the user may not have access
> > to.
>
> True, but then the surrogate key would be included in the error
> details which would allow the failing row to be identified.
Right, and is information which the user would have provided in the
query itself.
> >> > What do you think about returning just what the user provided in the
> >> > first place in both of these cases..? I'm not quite sure what that
> >> > would look like in the UPDATE case but for INSERT (and COPY) it would be
> >> > the subset of columns being inserted and the values originally provided.
> >> > That may not be what the actual conflict is due to, as there could be
> >> > before triggers changing things in the middle, or the conflict could be
> >> > on default values, but at least the input row could be identified and
> >> > there wouldn't be this information leak risk. Not sure how difficult
> >> > that would be to implement though.
> >>
> >> I could see that working for INSERTs, but for UPDATEs I don't think
> >> that would be very useful in practice, because the columns most likely
> >> to be useful for identifying the failing row (e.g., key columns) are
> >> also the columns least likely to have been updated.
> >
> > I'm not sure that I follow this- if you're not updating the key columns
> > then you're not likely to be having a conflict due to them...
>
> The constraint violation could well be due to updating a non-key
> column such as a column with a NOT NULL constraint on it, in which
> case only including that column in the error detail wouldn't do much
> good -- you'd want to include the key columns if possible.
This I can agree with- if the query doesn't include row-identifying
information (which implies that they're updating multiple records with
one statement) then it'd be helpful to know what row was failing the
update.
Practically, things get a bit tricky with this though- if we're only
going to return the columns which the user has access to, how do we
communicate which columns those are? The current error message doesn't
contain that information explicitly, it depends on the user being
knowledgable of (or able to look up) the table definition. The key
violation case only returns the columns of the key violated and we could
check that the user has either full table-level SELECT or has SELECT
rights to all of the columns involved in the key.
We could also check if the user has either table-level SELECT rights or
has SELECT rights to all columns in the primary key of the table and
then return the primary key in these other cases (what to do if there is
no primary key?). I'm not sure if we'd want to back-patch a change like
that and I'm a bit worried about the complexity of it in general- having
the error message depend so much on the permissions seems like it would
make things difficult for anything which is currently using that error
message in a programatic way (which I fully expect there are cases
of..).
Thanks,
Stephen
Robert, all,
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > In the end, it sounds like we all agree that the right approach is to
> > simply remove this detail and avoid the issue entirely.
>
> Well, I think that's an acceptable approach from the point of view of
> fixing the security exposure, but it's far from ideal. Good error
> messages are important for usability. I can live with this as a
> short-term fix, but in the long run I strongly believe we should try
> to do better.
I've been working to try and address this. Attached is a new patch
which moves the responsibility of figuring out what should be returned
down into the functions which build up the error detail which includes
the data (BuildIndexValueDescription and ExecBuildSlotValueDescription).
This allows us to avoid having to change any of the regression tests,
nor do we need to remove the information from the WITH CHECK option.
The patch is against master though it'd need to be back-patched, of
course. This will return either the full and unchanged-from-previous
information, if the user has SELECT on the table or SELECT on the
columns which would be displayed, or "(unknown)" if the user does not
have access to view the entire key (in a key violation case), or the
subset of columns the user has access to (in a "Failing row contains"
case). I'm not wedded to '(unknown)' by any means and welcome
suggestions. If the user does not have table-level SELECT rights,
they'll see for the "Failing row contains" case, they'll get:
Failing row contains (col1, col2, col3) = (1, 2, 3).
Or, if they have no access to any columns:
Failing row contains () = ()
These could be changed, of course. I don't consider this patch quite
ready to be committed and plan to do more testing and give it more
thought, but wanted to put it out there for others to comment on the
idea, the patch, and provide their own thoughts about what is safe and
sane to backpatch when it comes to error message changes like this.
As mentioned up-thread, another option would be to just omit the row
data detail completely unless the user has SELECT rights at the table
level.
Thanks!
Stephen
Вложения
On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost <sfrost@snowman.net> wrote: > suggestions. If the user does not have table-level SELECT rights, > they'll see for the "Failing row contains" case, they'll get: > > Failing row contains (col1, col2, col3) = (1, 2, 3). > > Or, if they have no access to any columns: > > Failing row contains () = () I haven't looked at the code, but that sounds nice, except that if they have no access to any columns, I'd leave the message out altogether instead of emitting it with no useful content. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > suggestions. If the user does not have table-level SELECT rights,
> > they'll see for the "Failing row contains" case, they'll get:
> >
> > Failing row contains (col1, col2, col3) = (1, 2, 3).
> >
> > Or, if they have no access to any columns:
> >
> > Failing row contains () = ()
>
> I haven't looked at the code, but that sounds nice, except that if
> they have no access to any columns, I'd leave the message out
> altogether instead of emitting it with no useful content.
Alright, I can change things around to make that happen without too much
trouble.
Thanks,
Stephen
On 29 October 2014 13:04, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost <sfrost@snowman.net> wrote: >> > suggestions. If the user does not have table-level SELECT rights, >> > they'll see for the "Failing row contains" case, they'll get: >> > >> > Failing row contains (col1, col2, col3) = (1, 2, 3). >> > >> > Or, if they have no access to any columns: >> > >> > Failing row contains () = () >> >> I haven't looked at the code, but that sounds nice, except that if >> they have no access to any columns, I'd leave the message out >> altogether instead of emitting it with no useful content. > > Alright, I can change things around to make that happen without too much > trouble. > Yes, skim-reading the patch, it looks like a good approach to me, and also +1 for not emitting anything if no values are visible. I fear, however, that for updatable views, in the most common case, the user won't have any permissions on the underlying table, and so the error detail will always be omitted. I wonder if one way we can improve upon that is to include the RTE's modifiedCols set in the set of columns the user can see, since for those columns what we would be reporting are the new user-supplied values, and so there is no information leakage. Regards, Dean
Dean, Robert,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 29 October 2014 13:04, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost <sfrost@snowman.net> wrote:
> >> > suggestions. If the user does not have table-level SELECT rights,
> >> > they'll see for the "Failing row contains" case, they'll get:
> >> >
> >> > Failing row contains (col1, col2, col3) = (1, 2, 3).
> >> >
> >> > Or, if they have no access to any columns:
> >> >
> >> > Failing row contains () = ()
> >>
> >> I haven't looked at the code, but that sounds nice, except that if
> >> they have no access to any columns, I'd leave the message out
> >> altogether instead of emitting it with no useful content.
> >
> > Alright, I can change things around to make that happen without too much
> > trouble.
>
> Yes, skim-reading the patch, it looks like a good approach to me, and
> also +1 for not emitting anything if no values are visible.
Alright, here's an updated patch which doesn't return any detail if no
values are visible or if only a partial key is visible.
Please take a look. I'm not thrilled with simply returning an empty
string and then checking that for BuildIndexValueDescription and
ExecBuildSlotValueDescription, but I figured we might have other users
of BuildIndexValueDescription and I wasn't seeing a particularly better
solution. Suggestions welcome, of course.
Thanks!
Stephen
Вложения
All,
Apologies, forgot to mention- this is again 9.4.
Thanks,
Stephen
* Stephen Frost (sfrost@snowman.net) wrote:
> Dean, Robert,
>
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> > On 29 October 2014 13:04, Stephen Frost <sfrost@snowman.net> wrote:
> > > * Robert Haas (robertmhaas@gmail.com) wrote:
> > >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > >> > suggestions. If the user does not have table-level SELECT rights,
> > >> > they'll see for the "Failing row contains" case, they'll get:
> > >> >
> > >> > Failing row contains (col1, col2, col3) = (1, 2, 3).
> > >> >
> > >> > Or, if they have no access to any columns:
> > >> >
> > >> > Failing row contains () = ()
> > >>
> > >> I haven't looked at the code, but that sounds nice, except that if
> > >> they have no access to any columns, I'd leave the message out
> > >> altogether instead of emitting it with no useful content.
> > >
> > > Alright, I can change things around to make that happen without too much
> > > trouble.
> >
> > Yes, skim-reading the patch, it looks like a good approach to me, and
> > also +1 for not emitting anything if no values are visible.
>
> Alright, here's an updated patch which doesn't return any detail if no
> values are visible or if only a partial key is visible.
>
> Please take a look. I'm not thrilled with simply returning an empty
> string and then checking that for BuildIndexValueDescription and
> ExecBuildSlotValueDescription, but I figured we might have other users
> of BuildIndexValueDescription and I wasn't seeing a particularly better
> solution. Suggestions welcome, of course.
>
> Thanks!
>
> Stephen
> From e00cc2f5be4524989e8d670296f82bb99f3774a1 Mon Sep 17 00:00:00 2001
> From: Stephen Frost <sfrost@snowman.net>
> Date: Mon, 12 Jan 2015 17:04:11 -0500
> Subject: [PATCH] Fix column-privilege leak in error-message paths
>
> While building error messages to return to the user,
> BuildIndexValueDescription and ExecBuildSlotValueDescription would
> happily include the entire key or entire row in the result returned to
> the user, even if the user didn't have access to view all of the columns
> being included.
>
> Instead, include only those columns which the user is providing or which
> the user has select rights on. If the user does not have any rights
> to view the table or any of the columns involved then no detail is
> provided. Note that, for duplicate key cases, the user must have access
> to all of the columns for the key to be shown; a partial key will not be
> returned.
>
> Back-patch all the way, as column-level privileges are now in all
> supported versions.
> ---
> src/backend/access/index/genam.c | 59 ++++++++-
> src/backend/access/nbtree/nbtinsert.c | 30 +++--
> src/backend/commands/copy.c | 6 +-
> src/backend/executor/execMain.c | 215 +++++++++++++++++++++++--------
> src/backend/executor/execUtils.c | 12 +-
> src/backend/utils/sort/tuplesort.c | 28 ++--
> src/test/regress/expected/privileges.out | 31 +++++
> src/test/regress/sql/privileges.sql | 24 ++++
> 8 files changed, 328 insertions(+), 77 deletions(-)
>
> diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
> index 850008b..1090568 100644
> --- a/src/backend/access/index/genam.c
> +++ b/src/backend/access/index/genam.c
> @@ -25,10 +25,12 @@
> #include "lib/stringinfo.h"
> #include "miscadmin.h"
> #include "storage/bufmgr.h"
> +#include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> +#include "utils/syscache.h"
> #include "utils/tqual.h"
>
>
> @@ -154,6 +156,9 @@ IndexScanEnd(IndexScanDesc scan)
> * form "(key_name, ...)=(key_value, ...)". This is currently used
> * for building unique-constraint and exclusion-constraint error messages.
> *
> + * Note that if the user does not have permissions to view the columns
> + * involved, an empty string "" will be returned instead.
> + *
> * The passed-in values/nulls arrays are the "raw" input to the index AM,
> * e.g. results of FormIndexDatum --- this is not necessarily what is stored
> * in the index, but it's what the user perceives to be stored.
> @@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
> Datum *values, bool *isnull)
> {
> StringInfoData buf;
> + Form_pg_index idxrec;
> + HeapTuple ht_idx;
> int natts = indexRelation->rd_rel->relnatts;
> int i;
> + int keyno;
> + Oid indexrelid = RelationGetRelid(indexRelation);
> + Oid indrelid;
> + AclResult aclresult;
> +
> + /*
> + * Check permissions- if the users does not have access to view the
> + * data in the key columns, we return "" instead, which callers can
> + * test for and use or not accordingly.
> + *
> + * First we need to check table-level SELECT access and then, if
> + * there is no access there, check column-level permissions.
> + */
> +
> + /*
> + * Fetch the pg_index tuple by the Oid of the index
> + */
> + ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexrelid));
> + if (!HeapTupleIsValid(ht_idx))
> + elog(ERROR, "cache lookup failed for index %u", indexrelid);
> + idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
> +
> + indrelid = idxrec->indrelid;
> + Assert(indexrelid == idxrec->indexrelid);
> +
> + /* Table-level SELECT is enough, if the user has it */
> + aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
> + if (aclresult != ACLCHECK_OK)
> + {
> + /*
> + * No table-level access, so step through the columns in the
> + * index and make sure the user has SELECT rights on all of them.
> + */
> + for (keyno = 0; keyno < idxrec->indnatts; keyno++)
> + {
> + AttrNumber attnum = idxrec->indkey.values[keyno];
> +
> + aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
> + ACL_SELECT);
> +
> + if (aclresult != ACLCHECK_OK)
> + {
> + /* No access, so clean up and just return "" */
> + ReleaseSysCache(ht_idx);
> + return "";
> + }
> + }
> + }
> + ReleaseSysCache(ht_idx);
>
> initStringInfo(&buf);
> appendStringInfo(&buf, "(%s)=(",
> - pg_get_indexdef_columns(RelationGetRelid(indexRelation),
> - true));
> + pg_get_indexdef_columns(indexrelid, true));
>
> for (i = 0; i < natts; i++)
> {
> diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
> index 59d7006..4279416 100644
> --- a/src/backend/access/nbtree/nbtinsert.c
> +++ b/src/backend/access/nbtree/nbtinsert.c
> @@ -388,18 +388,30 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
> {
> Datum values[INDEX_MAX_KEYS];
> bool isnull[INDEX_MAX_KEYS];
> + char *key_desc;
>
> index_deform_tuple(itup, RelationGetDescr(rel),
> values, isnull);
> - ereport(ERROR,
> - (errcode(ERRCODE_UNIQUE_VIOLATION),
> - errmsg("duplicate key value violates unique constraint \"%s\"",
> - RelationGetRelationName(rel)),
> - errdetail("Key %s already exists.",
> - BuildIndexValueDescription(rel,
> - values, isnull)),
> - errtableconstraint(heapRel,
> - RelationGetRelationName(rel))));
> +
> + key_desc = BuildIndexValueDescription(rel, values,
> + isnull);
> +
> + if (!strlen(key_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errdetail("Key %s already exists.",
> + key_desc),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
> }
> }
> else if (all_dead)
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index fbd7492..9ab1e19 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -160,6 +160,7 @@ typedef struct CopyStateData
> int *defmap; /* array of default att numbers */
> ExprState **defexprs; /* array of default att expressions */
> bool volatile_defexprs; /* is any of defexprs volatile? */
> + List *range_table;
>
> /*
> * These variables are used to reduce overhead in textual COPY FROM.
> @@ -784,6 +785,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
> bool pipe = (stmt->filename == NULL);
> Relation rel;
> Oid relid;
> + RangeTblEntry *rte;
>
> /* Disallow COPY to/from file or program except to superusers. */
> if (!pipe && !superuser())
> @@ -806,7 +808,6 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
> {
> TupleDesc tupDesc;
> AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT);
> - RangeTblEntry *rte;
> List *attnums;
> ListCell *cur;
>
> @@ -856,6 +857,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
>
> cstate = BeginCopyFrom(rel, stmt->filename, stmt->is_program,
> stmt->attlist, stmt->options);
> + cstate->range_table = list_make1(rte);
> *processed = CopyFrom(cstate); /* copy from file to database */
> EndCopyFrom(cstate);
> }
> @@ -864,6 +866,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
> cstate = BeginCopyTo(rel, stmt->query, queryString,
> stmt->filename, stmt->is_program,
> stmt->attlist, stmt->options);
> + cstate->range_table = list_make1(rte);
> *processed = DoCopyTo(cstate); /* copy from database to file */
> EndCopyTo(cstate);
> }
> @@ -2184,6 +2187,7 @@ CopyFrom(CopyState cstate)
> estate->es_result_relations = resultRelInfo;
> estate->es_num_result_relations = 1;
> estate->es_result_relation_info = resultRelInfo;
> + estate->es_range_table = cstate->range_table;
>
> /* Set up a tuple slot too */
> myslot = ExecInitExtraTupleSlot(estate);
> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> index 01eda70..49e4c81 100644
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -82,12 +82,17 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
> DestReceiver *dest);
> static bool ExecCheckRTEPerms(RangeTblEntry *rte);
> static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
> -static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
> +static char *ExecBuildSlotValueDescription(Oid reloid,
> + TupleTableSlot *slot,
> TupleDesc tupdesc,
> + Bitmapset *modifiedCols,
> int maxfieldlen);
> static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
> Plan *planTree);
>
> +#define GetModifiedColumns(relinfo, estate) \
> + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
> +
> /* end of local decls */
>
>
> @@ -1590,9 +1595,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
> Relation rel = resultRelInfo->ri_RelationDesc;
> TupleDesc tupdesc = RelationGetDescr(rel);
> TupleConstr *constr = tupdesc->constr;
> + Bitmapset *modifiedCols;
>
> Assert(constr);
>
> + modifiedCols = GetModifiedColumns(resultRelInfo, estate);
> +
> if (constr->has_not_null)
> {
> int natts = tupdesc->natts;
> @@ -1602,15 +1610,29 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
> {
> if (tupdesc->attrs[attrChk - 1]->attnotnull &&
> slot_attisnull(slot, attrChk))
> - ereport(ERROR,
> - (errcode(ERRCODE_NOT_NULL_VIOLATION),
> - errmsg("null value in column \"%s\" violates not-null constraint",
> - NameStr(tupdesc->attrs[attrChk - 1]->attname)),
> - errdetail("Failing row contains %s.",
> - ExecBuildSlotValueDescription(slot,
> - tupdesc,
> - 64)),
> - errtablecol(rel, attrChk)));
> + {
> + char *val_desc;
> +
> + val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
> + slot,
> + tupdesc,
> + modifiedCols,
> + 64);
> +
> + if (!strlen(val_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_NOT_NULL_VIOLATION),
> + errmsg("null value in column \"%s\" violates not-null constraint",
> + NameStr(tupdesc->attrs[attrChk - 1]->attname)),
> + errtablecol(rel, attrChk)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_NOT_NULL_VIOLATION),
> + errmsg("null value in column \"%s\" violates not-null constraint",
> + NameStr(tupdesc->attrs[attrChk - 1]->attname)),
> + errdetail("Failing row contains %s.", val_desc),
> + errtablecol(rel, attrChk)));
> + }
> }
> }
>
> @@ -1619,15 +1641,28 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
> const char *failed;
>
> if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
> - ereport(ERROR,
> - (errcode(ERRCODE_CHECK_VIOLATION),
> - errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
> - RelationGetRelationName(rel), failed),
> - errdetail("Failing row contains %s.",
> - ExecBuildSlotValueDescription(slot,
> - tupdesc,
> - 64)),
> - errtableconstraint(rel, failed)));
> + {
> + char *val_desc;
> +
> + val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
> + slot,
> + tupdesc,
> + modifiedCols,
> + 64);
> + if (!strlen(val_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_CHECK_VIOLATION),
> + errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
> + RelationGetRelationName(rel), failed),
> + errtableconstraint(rel, failed)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_CHECK_VIOLATION),
> + errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
> + RelationGetRelationName(rel), failed),
> + errdetail("Failing row contains %s.", val_desc),
> + errtableconstraint(rel, failed)));
> + }
> }
> }
>
> @@ -1638,9 +1673,13 @@ void
> ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> TupleTableSlot *slot, EState *estate)
> {
> + Relation rel = resultRelInfo->ri_RelationDesc;
> ExprContext *econtext;
> ListCell *l1,
> *l2;
> + Bitmapset *modifiedCols;
> +
> + modifiedCols = GetModifiedColumns(resultRelInfo, estate);
>
> /*
> * We will use the EState's per-tuple context for evaluating constraint
> @@ -1666,14 +1705,27 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> * above for CHECK constraints).
> */
> if (!ExecQual((List *) wcoExpr, econtext, false))
> - ereport(ERROR,
> - (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
> - errmsg("new row violates WITH CHECK OPTION for view \"%s\"",
> - wco->viewname),
> - errdetail("Failing row contains %s.",
> - ExecBuildSlotValueDescription(slot,
> - RelationGetDescr(resultRelInfo->ri_RelationDesc),
> - 64))));
> + {
> + char *val_desc;
> +
> + val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
> + slot,
> + RelationGetDescr(resultRelInfo->ri_RelationDesc),
> + modifiedCols,
> + 64);
> +
> + if (!strlen(val_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
> + errmsg("new row violates WITH CHECK OPTION for view \"%s\"",
> + wco->viewname)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
> + errmsg("new row violates WITH CHECK OPTION for view \"%s\"",
> + wco->viewname),
> + errdetail("Failing row contains %s.", val_desc)));
> + }
> }
> }
>
> @@ -1689,25 +1741,51 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> * dropped columns. We used to use the slot's tuple descriptor to decode the
> * data, but the slot's descriptor doesn't identify dropped columns, so we
> * now need to be passed the relation's descriptor.
> + *
> + * Note that, like BuildIndexValueDescription, if the user does not have
> + * permission to view any of the columns involved, an empty string is returned.
> */
> static char *
> -ExecBuildSlotValueDescription(TupleTableSlot *slot,
> +ExecBuildSlotValueDescription(Oid reloid,
> + TupleTableSlot *slot,
> TupleDesc tupdesc,
> + Bitmapset *modifiedCols,
> int maxfieldlen)
> {
> StringInfoData buf;
> + StringInfoData collist;
> bool write_comma = false;
> + bool write_comma_collist = false;
> int i;
> -
> - /* Make sure the tuple is fully deconstructed */
> - slot_getallattrs(slot);
> + AclResult aclresult;
> + bool table_perm = false;
> + bool any_perm = false;
>
> initStringInfo(&buf);
>
> appendStringInfoChar(&buf, '(');
>
> + /*
> + * Check that the user has permissions to see the row. If the user
> + * has table-level SELECT then that is good enough. If not, we have
> + * to check all the columns.
> + */
> + aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_SELECT);
> + if (aclresult != ACLCHECK_OK)
> + {
> + /* Set up the buffer for the column list */
> + initStringInfo(&collist);
> + appendStringInfoChar(&collist, '(');
> + }
> + else
> + table_perm = any_perm = true;
> +
> + /* Make sure the tuple is fully deconstructed */
> + slot_getallattrs(slot);
> +
> for (i = 0; i < tupdesc->natts; i++)
> {
> + bool column_perm = false;
> char *val;
> int vallen;
>
> @@ -1715,37 +1793,70 @@ ExecBuildSlotValueDescription(TupleTableSlot *slot,
> if (tupdesc->attrs[i]->attisdropped)
> continue;
>
> - if (slot->tts_isnull[i])
> - val = "null";
> - else
> + if (!table_perm)
> {
> - Oid foutoid;
> - bool typisvarlena;
> + aclresult = pg_attribute_aclcheck(reloid, tupdesc->attrs[i]->attnum,
> + GetUserId(), ACL_SELECT);
> + if (bms_is_member(tupdesc->attrs[i]->attnum - FirstLowInvalidHeapAttributeNumber,
> + modifiedCols) || aclresult == ACLCHECK_OK)
> + {
> + column_perm = any_perm = true;
>
> - getTypeOutputInfo(tupdesc->attrs[i]->atttypid,
> - &foutoid, &typisvarlena);
> - val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
> - }
> + if (write_comma_collist)
> + appendStringInfoString(&collist, ", ");
> + else
> + write_comma_collist = true;
>
> - if (write_comma)
> - appendStringInfoString(&buf, ", ");
> - else
> - write_comma = true;
> + appendStringInfoString(&collist, NameStr(tupdesc->attrs[i]->attname));
> + }
> + }
>
> - /* truncate if needed */
> - vallen = strlen(val);
> - if (vallen <= maxfieldlen)
> - appendStringInfoString(&buf, val);
> - else
> + if (table_perm || column_perm)
> {
> - vallen = pg_mbcliplen(val, vallen, maxfieldlen);
> - appendBinaryStringInfo(&buf, val, vallen);
> - appendStringInfoString(&buf, "...");
> + if (slot->tts_isnull[i])
> + val = "null";
> + else
> + {
> + Oid foutoid;
> + bool typisvarlena;
> +
> + getTypeOutputInfo(tupdesc->attrs[i]->atttypid,
> + &foutoid, &typisvarlena);
> + val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
> + }
> +
> + if (write_comma)
> + appendStringInfoString(&buf, ", ");
> + else
> + write_comma = true;
> +
> + /* truncate if needed */
> + vallen = strlen(val);
> + if (vallen <= maxfieldlen)
> + appendStringInfoString(&buf, val);
> + else
> + {
> + vallen = pg_mbcliplen(val, vallen, maxfieldlen);
> + appendBinaryStringInfo(&buf, val, vallen);
> + appendStringInfoString(&buf, "...");
> + }
> }
> }
>
> + /* If there were no permissions found, return an empty string. */
> + if (!any_perm)
> + return "";
> +
> appendStringInfoChar(&buf, ')');
>
> + if (!table_perm)
> + {
> + appendStringInfoString(&collist, ") = ");
> + appendStringInfoString(&collist, buf.data);
> +
> + return collist.data;
> + }
> +
> return buf.data;
> }
>
> diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
> index d5e1273..4860356 100644
> --- a/src/backend/executor/execUtils.c
> +++ b/src/backend/executor/execUtils.c
> @@ -1323,8 +1323,10 @@ retry:
> (errcode(ERRCODE_EXCLUSION_VIOLATION),
> errmsg("could not create exclusion constraint \"%s\"",
> RelationGetRelationName(index)),
> - errdetail("Key %s conflicts with key %s.",
> - error_new, error_existing),
> + strlen(error_new) == 0 || strlen(error_existing) == 0 ?
> + errdetail("Key conflicts exist.") :
> + errdetail("Key %s conflicts with key %s.",
> + error_new, error_existing),
> errtableconstraint(heap,
> RelationGetRelationName(index))));
> else
> @@ -1332,8 +1334,10 @@ retry:
> (errcode(ERRCODE_EXCLUSION_VIOLATION),
> errmsg("conflicting key value violates exclusion constraint \"%s\"",
> RelationGetRelationName(index)),
> - errdetail("Key %s conflicts with existing key %s.",
> - error_new, error_existing),
> + strlen(error_new) == 0 || strlen(error_existing) == 0 ?
> + errdetail("Key conflicts with existing key.") :
> + errdetail("Key %s conflicts with existing key %s.",
> + error_new, error_existing),
> errtableconstraint(heap,
> RelationGetRelationName(index))));
> }
> diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
> index aa0f6d8..6aba499 100644
> --- a/src/backend/utils/sort/tuplesort.c
> +++ b/src/backend/utils/sort/tuplesort.c
> @@ -3240,6 +3240,7 @@ comparetup_index_btree(const SortTuple *a, const SortTuple *b,
> {
> Datum values[INDEX_MAX_KEYS];
> bool isnull[INDEX_MAX_KEYS];
> + char *key_desc;
>
> /*
> * Some rather brain-dead implementations of qsort (such as the one in
> @@ -3250,15 +3251,24 @@ comparetup_index_btree(const SortTuple *a, const SortTuple *b,
> Assert(tuple1 != tuple2);
>
> index_deform_tuple(tuple1, tupDes, values, isnull);
> - ereport(ERROR,
> - (errcode(ERRCODE_UNIQUE_VIOLATION),
> - errmsg("could not create unique index \"%s\"",
> - RelationGetRelationName(state->indexRel)),
> - errdetail("Key %s is duplicated.",
> - BuildIndexValueDescription(state->indexRel,
> - values, isnull)),
> - errtableconstraint(state->heapRel,
> - RelationGetRelationName(state->indexRel))));
> +
> + key_desc = BuildIndexValueDescription(state->indexRel, values, isnull);
> +
> + if (!strlen(key_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("could not create unique index \"%s\"",
> + RelationGetRelationName(state->indexRel)),
> + errtableconstraint(state->heapRel,
> + RelationGetRelationName(state->indexRel))));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("could not create unique index \"%s\"",
> + RelationGetRelationName(state->indexRel)),
> + errdetail("Key %s is duplicated.", key_desc),
> + errtableconstraint(state->heapRel,
> + RelationGetRelationName(state->indexRel))));
> }
>
> /*
> diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
> index 1675075..b1ecd39 100644
> --- a/src/test/regress/expected/privileges.out
> +++ b/src/test/regress/expected/privileges.out
> @@ -381,6 +381,37 @@ SELECT atest6 FROM atest6; -- ok
> (0 rows)
>
> COPY atest6 TO stdout; -- ok
> +-- check error reporting with column privs
> +SET SESSION AUTHORIZATION regressuser1;
> +CREATE TABLE t1 (c1 int, c2 int, c3 int check (c3 < 5), primary key (c1, c2));
> +GRANT SELECT (c1) ON t1 TO regressuser2;
> +GRANT INSERT (c1, c2, c3) ON t1 TO regressuser2;
> +GRANT UPDATE (c1, c2, c3) ON t1 TO regressuser2;
> +-- seed data
> +INSERT INTO t1 VALUES (1, 1, 1);
> +INSERT INTO t1 VALUES (1, 2, 1);
> +INSERT INTO t1 VALUES (2, 1, 2);
> +INSERT INTO t1 VALUES (2, 2, 2);
> +INSERT INTO t1 VALUES (3, 1, 3);
> +SET SESSION AUTHORIZATION regressuser2;
> +INSERT INTO t1 (c1, c2) VALUES (1, 1); -- fail, but row not shown
> +ERROR: duplicate key value violates unique constraint "t1_pkey"
> +UPDATE t1 SET c2 = 1; -- fail, but row not shown
> +ERROR: duplicate key value violates unique constraint "t1_pkey"
> +INSERT INTO t1 (c1, c2) VALUES (null, null); -- fail, but see columns being inserted
> +ERROR: null value in column "c1" violates not-null constraint
> +DETAIL: Failing row contains (c1, c2) = (null, null).
> +INSERT INTO t1 (c3) VALUES (null); -- fail, but see columns being inserted or have SELECT
> +ERROR: null value in column "c1" violates not-null constraint
> +DETAIL: Failing row contains (c1, c3) = (null, null).
> +INSERT INTO t1 (c1) VALUES (5); -- fail, but see columns being inserted or have SELECT
> +ERROR: null value in column "c2" violates not-null constraint
> +DETAIL: Failing row contains (c1) = (5).
> +UPDATE t1 SET c3 = 10; -- fail, but see columns with SELECT rights, or being modified
> +ERROR: new row for relation "t1" violates check constraint "t1_c3_check"
> +DETAIL: Failing row contains (c1, c3) = (1, 10).
> +DROP TABLE t1;
> +ERROR: must be owner of relation t1
> -- test column-level privileges when involved with DELETE
> SET SESSION AUTHORIZATION regressuser1;
> ALTER TABLE atest6 ADD COLUMN three integer;
> diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
> index a0ff953..c850a2e 100644
> --- a/src/test/regress/sql/privileges.sql
> +++ b/src/test/regress/sql/privileges.sql
> @@ -256,6 +256,30 @@ UPDATE atest5 SET one = 1; -- fail
> SELECT atest6 FROM atest6; -- ok
> COPY atest6 TO stdout; -- ok
>
> +-- check error reporting with column privs
> +SET SESSION AUTHORIZATION regressuser1;
> +CREATE TABLE t1 (c1 int, c2 int, c3 int check (c3 < 5), primary key (c1, c2));
> +GRANT SELECT (c1) ON t1 TO regressuser2;
> +GRANT INSERT (c1, c2, c3) ON t1 TO regressuser2;
> +GRANT UPDATE (c1, c2, c3) ON t1 TO regressuser2;
> +
> +-- seed data
> +INSERT INTO t1 VALUES (1, 1, 1);
> +INSERT INTO t1 VALUES (1, 2, 1);
> +INSERT INTO t1 VALUES (2, 1, 2);
> +INSERT INTO t1 VALUES (2, 2, 2);
> +INSERT INTO t1 VALUES (3, 1, 3);
> +
> +SET SESSION AUTHORIZATION regressuser2;
> +INSERT INTO t1 (c1, c2) VALUES (1, 1); -- fail, but row not shown
> +UPDATE t1 SET c2 = 1; -- fail, but row not shown
> +INSERT INTO t1 (c1, c2) VALUES (null, null); -- fail, but see columns being inserted
> +INSERT INTO t1 (c3) VALUES (null); -- fail, but see columns being inserted or have SELECT
> +INSERT INTO t1 (c1) VALUES (5); -- fail, but see columns being inserted or have SELECT
> +UPDATE t1 SET c3 = 10; -- fail, but see columns with SELECT rights, or being modified
> +
> +DROP TABLE t1;
> +
> -- test column-level privileges when involved with DELETE
> SET SESSION AUTHORIZATION regressuser1;
> ALTER TABLE atest6 ADD COLUMN three integer;
> --
> 1.9.1
>
On 12 January 2015 at 22:16, Stephen Frost <sfrost@snowman.net> wrote: > Alright, here's an updated patch which doesn't return any detail if no > values are visible or if only a partial key is visible. > > Please take a look. I'm not thrilled with simply returning an empty > string and then checking that for BuildIndexValueDescription and > ExecBuildSlotValueDescription, but I figured we might have other users > of BuildIndexValueDescription and I wasn't seeing a particularly better > solution. Suggestions welcome, of course. > Actually I'm starting to wonder if it's even worth bothering about the index case. To leak information, you'd need to have a composite key for which you only had access to a subset of the key columns (which in itself seems like a pretty rare case), and then you'd need to specify new values to make the entire key conflict with an existing value, at which point the fact that an exception is thrown tells you that the values in the index must be the same as your new values. I'm struggling to imagine a realistic scenario where this would be a problem. Also, if we change BuildIndexValueDescription() in this way, it's going to make it more or less useless for updatable views, since in the most common case the user won't have permissions on the underlying table. For CHECK constraints/options, the change looks more reasonable, and I guess that approach could be extended to RLS by only including the modified columns, not the ones with select permissions, if RLS is enabled on the table. One minor comment on the code -- you could save a few cycles by only calling GetModifiedColumns() in the exceptional case. Regards, Dean
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 12 January 2015 at 22:16, Stephen Frost <sfrost@snowman.net> wrote:
> > Alright, here's an updated patch which doesn't return any detail if no
> > values are visible or if only a partial key is visible.
> >
> > Please take a look. I'm not thrilled with simply returning an empty
> > string and then checking that for BuildIndexValueDescription and
> > ExecBuildSlotValueDescription, but I figured we might have other users
> > of BuildIndexValueDescription and I wasn't seeing a particularly better
> > solution. Suggestions welcome, of course.
>
> Actually I'm starting to wonder if it's even worth bothering about the
> index case. To leak information, you'd need to have a composite key
> for which you only had access to a subset of the key columns (which in
> itself seems like a pretty rare case), and then you'd need to specify
> new values to make the entire key conflict with an existing value, at
> which point the fact that an exception is thrown tells you that the
> values in the index must be the same as your new values. I'm
> struggling to imagine a realistic scenario where this would be a
> problem.
I'm not sure that I follow.. From re-reading the above a couple times,
I take it you're making an argument that "people would be silly to set
their database up that way," but that's not an argument we can stand on
when it comes to privileges. Additionally, as the regression tests
hopefully show, if you have update on one column of a composite key, you
could find out the entire key today by issuing an update against that
column to set it to the same value throughout. You don't need to know
what the rest of the key is but only that two records somewhere have the
same values except for the one column you have update rights on.
> Also, if we change BuildIndexValueDescription() in this way, it's
> going to make it more or less useless for updatable views, since in
> the most common case the user won't have permissions on the underlying
> table.
That's certainly something to consider. I looked at trying to get which
columns the user had provided down to BuildIndexValueDescription, but I
couldn't find a straight-forward way to do that as it involves the index
AMs which we can't change (and I'm not really sure we'd want to anyway).
> For CHECK constraints/options, the change looks more reasonable, and I
> guess that approach could be extended to RLS by only including the
> modified columns, not the ones with select permissions, if RLS is
> enabled on the table. One minor comment on the code -- you could save
> a few cycles by only calling GetModifiedColumns() in the exceptional
> case.
Agreed, that sounds like a good approach for how to address the RLS
concern. Also, good point about GetModifiedColumns. There's a few
other minor changes that I want to make on re-reading it also. I should
be able to post a new version later today.
Thanks!
Stephen
On 13 January 2015 at 13:50, Stephen Frost <sfrost@snowman.net> wrote: > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote: >> On 12 January 2015 at 22:16, Stephen Frost <sfrost@snowman.net> wrote: >> > Alright, here's an updated patch which doesn't return any detail if no >> > values are visible or if only a partial key is visible. >> > >> > Please take a look. I'm not thrilled with simply returning an empty >> > string and then checking that for BuildIndexValueDescription and >> > ExecBuildSlotValueDescription, but I figured we might have other users >> > of BuildIndexValueDescription and I wasn't seeing a particularly better >> > solution. Suggestions welcome, of course. >> >> Actually I'm starting to wonder if it's even worth bothering about the >> index case. To leak information, you'd need to have a composite key >> for which you only had access to a subset of the key columns (which in >> itself seems like a pretty rare case), and then you'd need to specify >> new values to make the entire key conflict with an existing value, at >> which point the fact that an exception is thrown tells you that the >> values in the index must be the same as your new values. I'm >> struggling to imagine a realistic scenario where this would be a >> problem. > > I'm not sure that I follow.. From re-reading the above a couple times, > I take it you're making an argument that "people would be silly to set > their database up that way," but that's not an argument we can stand on > when it comes to privileges. Additionally, as the regression tests > hopefully show, if you have update on one column of a composite key, you > could find out the entire key today by issuing an update against that > column to set it to the same value throughout. You don't need to know > what the rest of the key is but only that two records somewhere have the > same values except for the one column you have update rights on. > Hmm, yes I guess that's right. One improvement we could trivially make is to only do this for multi-column indexes. If there is only one column there is no danger of information leakage, right? >> Also, if we change BuildIndexValueDescription() in this way, it's >> going to make it more or less useless for updatable views, since in >> the most common case the user won't have permissions on the underlying >> table. > > That's certainly something to consider. I looked at trying to get which > columns the user had provided down to BuildIndexValueDescription, but I > couldn't find a straight-forward way to do that as it involves the index > AMs which we can't change (and I'm not really sure we'd want to anyway). > Yeah I couldn't see any easy way of doing it. 2 possibilities sprung to mind -- (1) wrap the index update in a PG_TRY() and add the detail in the catch block, or (2) track the currently active EState and make GetModifiedColumns() into an exported function taking a single EState argument (the EState has the currently active ResultRelInfo on it). Neither of those alternatives seems particularly attractive to me though. Regards, Dean
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> One improvement we could trivially make is to only do this for
> multi-column indexes. If there is only one column there is no danger
> of information leakage, right?
That's an interesting thought. If there's only one column then to have
a conflict you must be changing it and providing a new value with either
a constant, through a column on which you must have select rights, or
with a function you have execute rights on.
So, no, I can't think of a way that would leak information. I'm still
on the fence about it though as it might be confusing to have
single-column indexes behave differently and I'm a bit worried that,
even if there isn't a way now to exploit this, there might be in the
future.
> Yeah I couldn't see any easy way of doing it. 2 possibilities sprung
> to mind -- (1) wrap the index update in a PG_TRY() and add the detail
> in the catch block, or (2) track the currently active EState and make
> GetModifiedColumns() into an exported function taking a single EState
> argument (the EState has the currently active ResultRelInfo on it).
> Neither of those alternatives seems particularly attractive to me
> though.
The EState is available when dealing with exclusion constraints but it's
not available to _bt_check_unique easily, which is the bigger issue.
GetModifiedColumns() could (and probably should, really) be moved into a
.h somewhere as it's also in trigger.c (actually, that's where I pulled
it from :).
Thanks,
Stephen
On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
> Alright, here's an updated patch which doesn't return any detail if no
> values are visible or if only a partial key is visible.
I browsed this patch. There's been no mention of foreign key constraints, but
ri_ReportViolation() deserves similar hardening. If a user has only DELETE
privilege on a PK table, FK violation messages should not leak the PK values.
Modifications to the foreign side are less concerning, since the user will
often know the attempted value; still, I would lock down both sides.
Please add a comment explaining the safety of each row-emitting message you
haven't changed. For example, the one in refresh_by_match_merge() is safe
because getting there requires MV ownership.
> --- a/src/backend/access/nbtree/nbtinsert.c
> +++ b/src/backend/access/nbtree/nbtinsert.c
> @@ -388,18 +388,30 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
> {
> Datum values[INDEX_MAX_KEYS];
> bool isnull[INDEX_MAX_KEYS];
> + char *key_desc;
>
> index_deform_tuple(itup, RelationGetDescr(rel),
> values, isnull);
> - ereport(ERROR,
> - (errcode(ERRCODE_UNIQUE_VIOLATION),
> - errmsg("duplicate key value violates unique constraint \"%s\"",
> - RelationGetRelationName(rel)),
> - errdetail("Key %s already exists.",
> - BuildIndexValueDescription(rel,
> - values, isnull)),
> - errtableconstraint(heapRel,
> - RelationGetRelationName(rel))));
> +
> + key_desc = BuildIndexValueDescription(rel, values,
> + isnull);
> +
> + if (!strlen(key_desc))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_UNIQUE_VIOLATION),
> + errmsg("duplicate key value violates unique constraint \"%s\"",
> + RelationGetRelationName(rel)),
> + errdetail("Key %s already exists.",
> + key_desc),
> + errtableconstraint(heapRel,
> + RelationGetRelationName(rel))));
Instead of duplicating an entire ereport() to change whether you include an
errdetail, use "condition ? errdetail(...) : 0".
Noah,
* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
> > Alright, here's an updated patch which doesn't return any detail if no
> > values are visible or if only a partial key is visible.
>
> I browsed this patch. There's been no mention of foreign key constraints, but
> ri_ReportViolation() deserves similar hardening. If a user has only DELETE
> privilege on a PK table, FK violation messages should not leak the PK values.
> Modifications to the foreign side are less concerning, since the user will
> often know the attempted value; still, I would lock down both sides.
Ah, yes, good point.
> Please add a comment explaining the safety of each row-emitting message you
> haven't changed. For example, the one in refresh_by_match_merge() is safe
> because getting there requires MV ownership.
Sure.
> Instead of duplicating an entire ereport() to change whether you include an
> errdetail, use "condition ? errdetail(...) : 0".
Yeah, that's a bit nicer, will do.
Thanks!
Stephen
Noah,
* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
> > Alright, here's an updated patch which doesn't return any detail if no
> > values are visible or if only a partial key is visible.
>
> I browsed this patch. There's been no mention of foreign key constraints, but
> ri_ReportViolation() deserves similar hardening. If a user has only DELETE
> privilege on a PK table, FK violation messages should not leak the PK values.
> Modifications to the foreign side are less concerning, since the user will
> often know the attempted value; still, I would lock down both sides.
Done.
> Please add a comment explaining the safety of each row-emitting message you
> haven't changed. For example, the one in refresh_by_match_merge() is safe
> because getting there requires MV ownership.
Done.
[...]
> Instead of duplicating an entire ereport() to change whether you include an
> errdetail, use "condition ? errdetail(...) : 0".
Done.
I've also updated the commit message to note the assigned CVE.
One remaining question is about single-column key violations. Should we
special-case those and allow them to be shown or no? I can't see a
reason not to currently but I wonder if we might have cause to act
differently in the future (not that I can think of a reason we'd ever
need to).
Certainly happy to change the specific messages around, if folks would
prefer something different from what I've chosen. I've kept errdetail's
for the cases where I feel it's still useful clarification.
Thanks!
Stephen
Вложения
I'm confused. Why does ExecBuildSlotValueDescription() return an empty string instead of NULL? There doesn't seem to be any value left in that idea, and returning NULL would make the callsites slightly simpler as well. (Also, I think the comment on top of it should be updated to reflect the permissions-related issues.) It seems that the reason for this is to be consistent with BuildIndexValueDescription, which seems to do the same thing to simplify the stuff going on at check_exclusion_constraint() -- by returning an empty string, that code doesn't need to check which of the returned values is empty, only whether both are. That seems an odd choice to me; it seems better to me to be specific, i.e. include each of the %s depending on whether the returned string is null or not. You would have three possible different errdetails, but that seems a good thing anyway. (Also, ISTM the "if (!strlen(foo))" idiom should be avoided because it is confusing; better test explicitely for zero or nonzero. Anyway if you change the functions to return NULL, you can test for NULL-ness easily and there's no possible confusion anymore.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> I'm confused. Why does ExecBuildSlotValueDescription() return an empty
> string instead of NULL? There doesn't seem to be any value left in that
> idea, and returning NULL would make the callsites slightly simpler as
> well. (Also, I think the comment on top of it should be updated to
> reflect the permissions-related issues.)
The comment on top of ExecBuildSlotValueDescription() does include this:
* Note that, like BuildIndexValueDescription, if the user does not have* permission to view any of the columns
involved,an empty string is returned.
Is that insufficient?
> It seems that the reason for this is to be consistent with
> BuildIndexValueDescription, which seems to do the same thing to simplify
> the stuff going on at check_exclusion_constraint() -- by returning an
> empty string, that code doesn't need to check which of the returned
> values is empty, only whether both are. That seems an odd choice to me;
> it seems better to me to be specific, i.e. include each of the %s
> depending on whether the returned string is null or not. You would have
> three possible different errdetails, but that seems a good thing anyway.
Right, ExecBuildSlotValueDescription() was made to be consistent with
BuildIndexValueDescription. The reason for BuildIndexValueDescription
returning an empty string is different from why you hypothosize though-
it's exported and I was a bit worried that existing external callers
might not be prepared for it to return a NULL instead of a string of
some kind. If this was a green field instead of a back-patch fix, I'd
certainly return NULL instead.
If others feel that's not a good reason to avoid returning NULL, I can
certainly change it around.
> (Also, ISTM the "if (!strlen(foo))" idiom should be avoided because it
> is confusing; better test explicitely for zero or nonzero. Anyway if
> you change the functions to return NULL, you can test for NULL-ness
> easily and there's no possible confusion anymore.)
Yeah, I thought I had eliminated all of those on my own review, but
looks like I missed one.
Updated patch attached.
Thanks!
Stephen
* Stephen Frost (sfrost@snowman.net) wrote:
> Updated patch attached.
Ugh. Hit send too quickly. Attached.
Thanks!
Stephen
Вложения
All,
* Stephen Frost (sfrost@snowman.net) wrote:
> > It seems that the reason for this is to be consistent with
> > BuildIndexValueDescription, which seems to do the same thing to simplify
> > the stuff going on at check_exclusion_constraint() -- by returning an
> > empty string, that code doesn't need to check which of the returned
> > values is empty, only whether both are. That seems an odd choice to me;
> > it seems better to me to be specific, i.e. include each of the %s
> > depending on whether the returned string is null or not. You would have
> > three possible different errdetails, but that seems a good thing anyway.
>
> Right, ExecBuildSlotValueDescription() was made to be consistent with
> BuildIndexValueDescription. The reason for BuildIndexValueDescription
> returning an empty string is different from why you hypothosize though-
> it's exported and I was a bit worried that existing external callers
> might not be prepared for it to return a NULL instead of a string of
> some kind. If this was a green field instead of a back-patch fix, I'd
> certainly return NULL instead.
>
> If others feel that's not a good reason to avoid returning NULL, I can
> certainly change it around.
Does anyone else want to weigh in on this?
It's no guarantee, but checking codesearch.debian.net, the only packages
which include BuildIndexValueDescription are PG core and derivatives.
Thanks!
Stephen
On Mon, Jan 19, 2015 at 11:05:22AM -0500, Stephen Frost wrote: > One remaining question is about single-column key violations. Should we > special-case those and allow them to be shown or no? I can't see a > reason not to currently but I wonder if we might have cause to act > differently in the future (not that I can think of a reason we'd ever > need to). Keep them hidden. Attempting to delete a PK row should not reveal otherwise-inaccessible values involved in any constraint violation. It's tempting to make an exception for single-column UNIQUE constraints, but some of the ensuing data disclosures are questionable. What if the violation arose from a column default expression or from index corruption? On Mon, Jan 19, 2015 at 11:46:35AM -0500, Stephen Frost wrote: > Right, ExecBuildSlotValueDescription() was made to be consistent with > BuildIndexValueDescription. The reason for BuildIndexValueDescription > returning an empty string is different from why you hypothosize though- > it's exported and I was a bit worried that existing external callers > might not be prepared for it to return a NULL instead of a string of > some kind. If this was a green field instead of a back-patch fix, I'd > certainly return NULL instead. > > If others feel that's not a good reason to avoid returning NULL, I can > certainly change it around. I won't lose sleep if it does return "" for that reason, but I'm relatively unconcerned about extension API compatibility here. That function is called from datatype-independent index uniqueness checks. This mailing list has discussed the difficulties of implementing an index access method in an extension, and no such extension has come forward. Your latest patch has whitespace errors; visit "git diff --check".
* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Jan 19, 2015 at 11:05:22AM -0500, Stephen Frost wrote:
> > One remaining question is about single-column key violations. Should we
> > special-case those and allow them to be shown or no? I can't see a
> > reason not to currently but I wonder if we might have cause to act
> > differently in the future (not that I can think of a reason we'd ever
> > need to).
>
> Keep them hidden. Attempting to delete a PK row should not reveal
> otherwise-inaccessible values involved in any constraint violation. It's
> tempting to make an exception for single-column UNIQUE constraints, but some
> of the ensuing data disclosures are questionable. What if the violation arose
> from a column default expression or from index corruption?
Interesting point. I've kept them hidden in this latest version.
> On Mon, Jan 19, 2015 at 11:46:35AM -0500, Stephen Frost wrote:
> > Right, ExecBuildSlotValueDescription() was made to be consistent with
> > BuildIndexValueDescription. The reason for BuildIndexValueDescription
> > returning an empty string is different from why you hypothosize though-
> > it's exported and I was a bit worried that existing external callers
> > might not be prepared for it to return a NULL instead of a string of
> > some kind. If this was a green field instead of a back-patch fix, I'd
> > certainly return NULL instead.
> >
> > If others feel that's not a good reason to avoid returning NULL, I can
> > certainly change it around.
>
> I won't lose sleep if it does return "" for that reason, but I'm relatively
> unconcerned about extension API compatibility here. That function is called
> from datatype-independent index uniqueness checks. This mailing list has
> discussed the difficulties of implementing an index access method in an
> extension, and no such extension has come forward.
Alright, I've reworked this to have those functions return NULL instead.
> Your latest patch has whitespace errors; visit "git diff --check".
Yeah, I had done that but then made a few additional tweaks and didn't
recheck, sorry about that. I'm playing around w/ my git workflow a bit
and hopefully it won't happen again. :)
Updated patch attached.
Thanks!
Stephen
Вложения
Note the first comment in this hunk was not update to talk about NULL
instead of "":
> @@ -163,13 +168,63 @@ BuildIndexValueDescription(Relation indexRelation,
> Datum *values, bool *isnull)
> {
> StringInfoData buf;
> + Form_pg_index idxrec;
> + HeapTuple ht_idx;
> int natts = indexRelation->rd_rel->relnatts;
> int i;
> + int keyno;
> + Oid indexrelid = RelationGetRelid(indexRelation);
> + Oid indrelid;
> + AclResult aclresult;
> +
> + /*
> + * Check permissions- if the users does not have access to view the
> + * data in the key columns, we return "" instead, which callers can
> + * test for and use or not accordingly.
> + *
> + * First we need to check table-level SELECT access and then, if
> + * there is no access there, check column-level permissions.
> + */
[hunk continues]
> + /* Table-level SELECT is enough, if the user has it */
> + aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
> + if (aclresult != ACLCHECK_OK)
> + {
> + /*
> + * No table-level access, so step through the columns in the
> + * index and make sure the user has SELECT rights on all of them.
> + */
> + for (keyno = 0; keyno < idxrec->indnatts; keyno++)
> + {
> + AttrNumber attnum = idxrec->indkey.values[keyno];
> +
> + aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
> + ACL_SELECT);
> +
> + if (aclresult != ACLCHECK_OK)
> + {
> + /* No access, so clean up and return */
> + ReleaseSysCache(ht_idx);
> + return NULL;
> + }
> + }
> + }
Hm, this is a bit odd. I thought you were going to return the subset of
columns that the user had permission to read, not empty if any of them
was inaccesible. Did I misunderstand?
> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> index 4c55551..20acf04 100644
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -82,12 +82,17 @@ static void ExecutePlan(EState *estate, PlanState *planstate,
> DestReceiver *dest);
> static bool ExecCheckRTEPerms(RangeTblEntry *rte);
> static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
> -static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
> +static char *ExecBuildSlotValueDescription(Oid reloid,
> + TupleTableSlot *slot,
> TupleDesc tupdesc,
> + Bitmapset *modifiedCols,
> int maxfieldlen);
> static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
> Plan *planTree);
>
> +#define GetModifiedColumns(relinfo, estate) \
> + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
I assume you are aware that this GetModifiedColumns() macro is a
duplicate of another one found elsewhere. However I think this is not
such a hot idea; the UPSERT patch series has a preparatory patch that
changes that other macro definition, as far as I recall; probably it'd
be a good idea to move it elsewhere, to avoid a future divergence.
> @@ -1689,25 +1722,54 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> * dropped columns. We used to use the slot's tuple descriptor to decode the
> * data, but the slot's descriptor doesn't identify dropped columns, so we
> * now need to be passed the relation's descriptor.
> + *
> + * Note that, like BuildIndexValueDescription, if the user does not have
> + * permission to view any of the columns involved, a NULL is returned. Unlike
> + * BuildIndexValueDescription, if the user has access to view a subset of the
> + * column involved, that subset will be returned with a key identifying which
> + * columns they are.
> */
Ah, I now see that you are aware of the NULL-or-nothing nature of
BuildIndexValueDescription ... but the comments there don't explain the
reason. Perhaps a comment in BuildIndexValueDescription is in order
rather than a code change?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
Thanks for the review.
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Note the first comment in this hunk was not update to talk about NULL
> instead of "":
Ah, good catch, will fix.
> Hm, this is a bit odd. I thought you were going to return the subset of
> columns that the user had permission to read, not empty if any of them
> was inaccesible. Did I misunderstand?
The subset is for regular relations. For indexes and keys, we only
return either the whole key or nothing. Returning a partial key didn't
make much sense to me and we also don't know which columns were actually
provided by the user since it's going through the index AM, so we can't
return just what the user provided.
> > +#define GetModifiedColumns(relinfo, estate) \
> > + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
>
> I assume you are aware that this GetModifiedColumns() macro is a
> duplicate of another one found elsewhere. However I think this is not
> such a hot idea; the UPSERT patch series has a preparatory patch that
> changes that other macro definition, as far as I recall; probably it'd
> be a good idea to move it elsewhere, to avoid a future divergence.
Yeah, I had meant to do something about that and had looked around but
didn't find any particularly good place to put it. Any suggestions on
that?
> > @@ -1689,25 +1722,54 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> > * dropped columns. We used to use the slot's tuple descriptor to decode the
> > * data, but the slot's descriptor doesn't identify dropped columns, so we
> > * now need to be passed the relation's descriptor.
> > + *
> > + * Note that, like BuildIndexValueDescription, if the user does not have
> > + * permission to view any of the columns involved, a NULL is returned. Unlike
> > + * BuildIndexValueDescription, if the user has access to view a subset of the
> > + * column involved, that subset will be returned with a key identifying which
> > + * columns they are.
> > */
>
> Ah, I now see that you are aware of the NULL-or-nothing nature of
> BuildIndexValueDescription ... but the comments there don't explain the
> reason. Perhaps a comment in BuildIndexValueDescription is in order
> rather than a code change?
Yeah, I'll add comments to BuildIndexValueDescription to explain why
it's all-or-nothing.
I've also been working on back-patching the fixes; the next update will
hopefully include patches for all branches.
Thanks!
Stephen
Stephen Frost wrote: > > > +#define GetModifiedColumns(relinfo, estate) \ > > > + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols) > > > > I assume you are aware that this GetModifiedColumns() macro is a > > duplicate of another one found elsewhere. However I think this is not > > such a hot idea; the UPSERT patch series has a preparatory patch that > > changes that other macro definition, as far as I recall; probably it'd > > be a good idea to move it elsewhere, to avoid a future divergence. > > Yeah, I had meant to do something about that and had looked around but > didn't find any particularly good place to put it. Any suggestions on > that? Hmm, tough call now that I look it up. This macro depends on ResultRelInfo and EState, both of which are in execnodes.h, and also on rt_fetch which is in parsetree.h. There is no existing header that includes parsetree.h (only .c files), so we would have to add one inclusion on some other header file, or create a new header with just this definition. None of these sounds real satisfactory (including parsetree.h in execnodes.h sounds very bad). Maybe just add a comment on both definitions to note that they are dupes of each other? That would be more backpatchable that anything else that occurs to me right away. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > > > +#define GetModifiedColumns(relinfo, estate) \
> > > > + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
> > >
> > > I assume you are aware that this GetModifiedColumns() macro is a
> > > duplicate of another one found elsewhere. However I think this is not
> > > such a hot idea; the UPSERT patch series has a preparatory patch that
> > > changes that other macro definition, as far as I recall; probably it'd
> > > be a good idea to move it elsewhere, to avoid a future divergence.
> >
> > Yeah, I had meant to do something about that and had looked around but
> > didn't find any particularly good place to put it. Any suggestions on
> > that?
>
> Hmm, tough call now that I look it up. This macro depends on
> ResultRelInfo and EState, both of which are in execnodes.h, and also on
> rt_fetch which is in parsetree.h. There is no existing header that
> includes parsetree.h (only .c files), so we would have to add one
> inclusion on some other header file, or create a new header with just
> this definition. None of these sounds real satisfactory (including
> parsetree.h in execnodes.h sounds very bad). Maybe just add a comment
> on both definitions to note that they are dupes of each other? That
> would be more backpatchable that anything else that occurs to me right
> away.
Good thought, I'll do that.
Thanks!
Stephen
Alvaro, all,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Hmm, tough call now that I look it up. This macro depends on
> ResultRelInfo and EState, both of which are in execnodes.h, and also on
> rt_fetch which is in parsetree.h. There is no existing header that
> includes parsetree.h (only .c files), so we would have to add one
> inclusion on some other header file, or create a new header with just
> this definition. None of these sounds real satisfactory (including
> parsetree.h in execnodes.h sounds very bad). Maybe just add a comment
> on both definitions to note that they are dupes of each other? That
> would be more backpatchable that anything else that occurs to me right
> away.
Alright, I've made these changes and backpatched it all the way to 9.0,
and forward-patched it to master and made the changes for RLS too. Note
that the RLS work ended up requiring a bit of refactoring and moving
things into a new utils/rls.c and /rls.h. I also changed a few of the
existing rewrite/rowsecurity.h #include's to be just utils/rls.h, which
seemed like a good idea to do too.
Would certainly appreciate any additional review/comments, especially
since I think Alvaro might be out of pocket for a bit. I'll be going
back over all the diffs myself and checking for any issues too, of
course. Note that things get pretty different as you go back through
the various releases, for example, we didn't have
ExecBuildSlotValueDescriptions until 9.2.
Thanks!
Stephen
Вложения
All,
* Stephen Frost (sfrost@snowman.net) wrote:
> Would certainly appreciate any additional review/comments, especially
> since I think Alvaro might be out of pocket for a bit. I'll be going
> back over all the diffs myself and checking for any issues too, of
> course. Note that things get pretty different as you go back through
> the various releases, for example, we didn't have
> ExecBuildSlotValueDescriptions until 9.2.
Here's the latest set with a few additional improvements (mostly
comments but also a couple missed #include's and eliminating unnecessary
whitespace changes). Unless there are issues with my testing tonight or
concerns raised, I'll push these tomorrow.
Thanks!
Stephen
Вложения
On 27 January 2015 at 22:45, Stephen Frost <sfrost@snowman.net> wrote:
> Here's the latest set with a few additional improvements (mostly
> comments but also a couple missed #include's and eliminating unnecessary
> whitespace changes). Unless there are issues with my testing tonight or
> concerns raised, I'll push these tomorrow.
>
I spotted a couple of minor things reading the patches:
- There's a typo in the comment for the GetModifiedColumns() macros
("...stick in into...").
- The new regression test is not tidying up properly after itself,
because it's trying to drop the table t1 as the wrong user.
Other than that, this looks reasonable to me, and I think that for
most common situations it won't reduce the detail in errors.
Regards,
Dean
Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 27 January 2015 at 22:45, Stephen Frost <sfrost@snowman.net> wrote:
> > Here's the latest set with a few additional improvements (mostly
> > comments but also a couple missed #include's and eliminating unnecessary
> > whitespace changes). Unless there are issues with my testing tonight or
> > concerns raised, I'll push these tomorrow.
>
> I spotted a couple of minor things reading the patches:
>
> - There's a typo in the comment for the GetModifiedColumns() macros
> ("...stick in into...").
Fixed.
> - The new regression test is not tidying up properly after itself,
> because it's trying to drop the table t1 as the wrong user.
Urgh. Not sure how I managed to miss that; guess I was just too focused
on what I was testing. :)
> Other than that, this looks reasonable to me, and I think that for
> most common situations it won't reduce the detail in errors.
Thanks! I'll be pushing this soon (finally!).
Thanks again,
Stephen