Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 27 January 2015 at 22:45, Stephen Frost sfr...@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


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Alvaro,

Thanks for the review.

* Alvaro Herrera (alvhe...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Alvaro Herrera (alvhe...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Alvaro Herrera]

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 4c1..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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Noah Misch (n...@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
From d9f0e186a74e4d11e9c7f3181dbf98bae3224111 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@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, ExecBuildSlotValueDescription and
ri_ReportViolation 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 and a NULL value is returned from BuildIndexValueDescription
and ExecBuildSlotValueDescription.  Note that, for 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.

This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
---
 src/backend/access/index/genam.c |  59 ++-
 src/backend/access/nbtree/nbtinsert.c|  12 ++-
 src/backend/commands/copy.c  |   6 +-
 src/backend/commands/matview.c   |   7 ++
 src/backend/executor/execMain.c  | 171 ---
 src/backend/executor/execUtils.c |  12 ++-
 src/backend/utils/adt/ri_triggers.c  |  78 ++
 src/backend/utils/sort/tuplesort.c   |   9 +-
 src/test/regress/expected/privileges.out |  31 ++
 src/test/regress/sql/privileges.sql  |  24 +
 10 files changed, 338 insertions(+), 71 deletions(-)

diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..69cbbd5 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, a NULL is returned.
+ *
  * 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 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

All,

* Stephen Frost (sfr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Noah Misch]

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.


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Noah,

* Noah Misch (n...@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
From 3ea19711f06718fa3e43fa8e587a54bcd6acf8fa Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@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, ExecBuildSlotValueDescription and
ri_ReportViolation 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 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.

This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
---
 src/backend/access/index/genam.c |  59 ++-
 src/backend/access/nbtree/nbtinsert.c|  13 ++-
 src/backend/commands/copy.c  |   6 +-
 src/backend/commands/matview.c   |   7 ++
 src/backend/executor/execMain.c  | 170 ---
 src/backend/executor/execUtils.c |  12 ++-
 src/backend/utils/adt/ri_triggers.c  |  78 ++
 src/backend/utils/sort/tuplesort.c   |  28 +++--
 src/test/regress/expected/privileges.out |  31 ++
 src/test/regress/sql/privileges.sql  |  24 +
 10 files changed, 351 insertions(+), 77 deletions(-)

diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..e34a280 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 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Alvaro,

* Alvaro Herrera (alvhe...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
 Updated patch attached.

Ugh.  Hit send too quickly.  Attached.

Thanks!

Stephen
From f74dcef56bd3507c6bb26b0468655fd8e408fc80 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@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, ExecBuildSlotValueDescription and
ri_ReportViolation 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 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.

This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
---
 src/backend/access/index/genam.c |  59 ++-
 src/backend/access/nbtree/nbtinsert.c|  13 ++-
 src/backend/commands/copy.c  |   6 +-
 src/backend/commands/matview.c   |   7 ++
 src/backend/executor/execMain.c  | 170 ---
 src/backend/executor/execUtils.c |  12 ++-
 src/backend/utils/adt/ri_triggers.c  |  78 ++
 src/backend/utils/sort/tuplesort.c   |  10 +-
 src/test/regress/expected/privileges.out |  31 ++
 src/test/regress/sql/privileges.sql  |  24 +
 10 files changed, 339 insertions(+), 71 deletions(-)

diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 850008b..e34a280 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..2413c68 100644
--- 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Noah,

* Noah Misch (n...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Noah Misch]

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.


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 13 January 2015 at 13:50, Stephen Frost sfr...@snowman.net wrote:
 * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 12 January 2015 at 22:16, Stephen Frost sfr...@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


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 12 January 2015 at 22:16, Stephen Frost sfr...@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


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 12 January 2015 at 22:16, Stephen Frost sfr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Dean Rasheed (dean.a.rash...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Dean, Robert,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 29 October 2014 13:04, Stephen Frost sfr...@snowman.net wrote:
  * Robert Haas (robertmh...@gmail.com) wrote:
  On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@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 sfr...@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 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

All,

Apologies, forgot to mention- this is again 9.4.

Thanks,

Stephen

* Stephen Frost (sfr...@snowman.net) wrote:
 Dean, Robert,
 
 * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
  On 29 October 2014 13:04, Stephen Frost sfr...@snowman.net wrote:
   * Robert Haas (robertmh...@gmail.com) wrote:
   On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@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 sfr...@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 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 29 October 2014 13:04, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@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


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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
 On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost sfr...@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
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
new file mode 100644
index 8849c08..889b813
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
***
*** 25,35 
--- 25,37 
  #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/ruleutils.h
  #include utils/snapmgr.h
+ #include utils/syscache.h
  #include utils/tqual.h
  
  
*** BuildIndexValueDescription(Relation inde
*** 164,176 
  		   Datum *values, bool *isnull)
  {
  	StringInfoData buf;
  	int			natts = indexRelation-rd_rel-relnatts;
  	int			i;
  
  	initStringInfo(buf);
  	appendStringInfo(buf, (%s)=(,
! 	 pg_get_indexdef_columns(RelationGetRelid(indexRelation),
! 			 true));
  
  	for (i = 0; i  natts; i++)
  	{
--- 166,227 
  		   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 unknown instead.
+ 	 *
+ 	 * 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 'unknown' */
+ ReleaseSysCache(ht_idx);
+ return (unknown);
+ 			}
+ 		}
+ 	}
+ 	ReleaseSysCache(ht_idx);
  
  	initStringInfo(buf);
  	appendStringInfo(buf, (%s)=(,
! 	 pg_get_indexdef_columns(indexrelid, true));
  
  	for (i = 

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Robert Haas]

On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
 On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost sfr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 30 September 2014 16:52, Stephen Frost sfr...@snowman.net wrote:
 * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 29 September 2014 16:46, Stephen Frost sfr...@snowman.net wrote:
  * Robert Haas (robertmh...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 30 September 2014 16:52, Stephen Frost sfr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 30 September 2014 20:17, Stephen Frost sfr...@snowman.net wrote:
 * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 30 September 2014 16:52, Stephen Frost sfr...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Robert Haas]

On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed dean.a.rash...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
 On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed dean.a.rash...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Robert Haas]

On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed dean.a.rash...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Robert Haas (robertmh...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Stephen Frost (sfr...@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
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
new file mode 100644
index 17d9765..f9a8bff
*** a/src/backend/access/nbtree/nbtinsert.c
--- b/src/backend/access/nbtree/nbtinsert.c
***
*** 21,26 
--- 21,27 
  #include miscadmin.h
  #include storage/lmgr.h
  #include storage/predicate.h
+ #include utils/acl.h
  #include utils/inval.h
  #include utils/tqual.h
  
*** _bt_check_unique(Relation rel, IndexTupl
*** 387,400 
  
  		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;
  	}
  }
--- 388,420 
  
  		index_deform_tuple(itup, RelationGetDescr(rel),
  		   values, isnull);
! 		/*
! 		 * When reporting a failure, it can be handy to have
! 		 * the key which failed reported.  Unfortunately, when
! 		 * using column-level privileges, this could expose
! 		 * a column the user does not have rights for.  Instead,
! 		 * only report the key if the user has full table-level
! 		 * SELECT rights on the relation.
! 		 */
! 		if (pg_class_aclcheck(RelationGetRelid(rel),
! 			  GetUserId(), ACL_SELECT) ==
! ACLCHECK_OK)
! 			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;
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNIQUE_VIOLATION),
! 	 errmsg(duplicate key value violates unique constraint \%s\,
! 			RelationGetRelationName(rel)),
! 	 errtableconstraint(heapRel,
  			 RelationGetRelationName(rel;
  	}
  }
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
new file mode 100644
index 85d1c63..fe98599
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*** ExecConstraints(ResultRelInfo *resultRel
*** 1600,1614 
  		{
  			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)));
  		}
  	}
  
--- 1600,1631 
  		{
  			if (tupdesc-attrs[attrChk - 1]-attnotnull 
  slot_attisnull(slot, attrChk))
! 			{
! /*
!  * When reporting failure, it's handy to have the whole row
!  * which failed returned, but we can only show it if the user
!  * has full SELECT rights on the relation, otherwise we might
!  * end up showing fields the user does not have access to, due
!  * to column-level privileges.
!  */
! if (pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
! 	  ACL_SELECT) == ACLCHECK_OK)
! 	ereport(ERROR,
! 			

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Dean Rasheed]

On 27 September 2014 14:33, Stephen Frost sfr...@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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Heikki Linnakangas]

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

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Heikki Linnakangas (hlinnakan...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Stephen Frost (sfr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] WITH CHECK and Column-Level Privileges

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
 * Stephen Frost (sfr...@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


signature.asc
Description: Digital signature