WITH CHECK and Column-Level Privileges

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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