Re: [HACKERS] WITH CHECK and Column-Level Privileges
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 27 January 2015 at 22:45, Stephen Frost wrote: > > Here's the latest set with a few additional improvements (mostly > > comments but also a couple missed #include's and eliminating unnecessary > > whitespace changes). Unless there are issues with my testing tonight or > > concerns raised, I'll push these tomorrow. > > I spotted a couple of minor things reading the patches: > > - There's a typo in the comment for the GetModifiedColumns() macros > ("...stick in into..."). Fixed. > - The new regression test is not tidying up properly after itself, > because it's trying to drop the table t1 as the wrong user. Urgh. Not sure how I managed to miss that; guess I was just too focused on what I was testing. :) > Other than that, this looks reasonable to me, and I think that for > most common situations it won't reduce the detail in errors. Thanks! I'll be pushing this soon (finally!). Thanks again, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WITH CHECK and Column-Level Privileges
On 27 January 2015 at 22:45, Stephen Frost 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
* 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
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
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
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
* 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 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 FormIndexD
Re: [HACKERS] WITH CHECK and Column-Level Privileges
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
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
* 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 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
Re: [HACKERS] WITH CHECK and Column-Level Privileges
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
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
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 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
Re: [HACKERS] WITH CHECK and Column-Level Privileges
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
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 (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
On 13 January 2015 at 13:50, Stephen Frost wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> On 12 January 2015 at 22:16, 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. >> > >> > 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 (dean.a.rash...@gmail.com) wrote: > On 12 January 2015 at 22:16, 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. > > > > 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
On 12 January 2015 at 22:16, 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. > > 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
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 wrote: > > > * Robert Haas (robertmh...@gmail.com) wrote: > > >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost > > >> 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 > 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 > +
Re: [HACKERS] WITH CHECK and Column-Level Privileges
Dean, Robert, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 29 October 2014 13:04, Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost 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 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-leve
Re: [HACKERS] WITH CHECK and Column-Level Privileges
On 29 October 2014 13:04, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost 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
* Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost 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
On Wed, Oct 29, 2014 at 8:16 AM, Stephen Frost 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
Robert, all, * Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost 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_
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 30 September 2014 20:17, Stephen Frost wrote: > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > >> One of the main things that detail is useful for is identifying the > >> failing row in a multi-row update. In most real-world cases, I would > >> expect the column-level privileges to include the table's PK, so the > >> detail would meet that requirement. In fact the column-level > >> privileges would pretty much have to include sufficient columns to > >> usefully identify rows, otherwise updates wouldn't be practical. > > > > That may or may not be true- the user needs sufficient information to > > identify a row, but that doesn't mean they have access to all columns > > make up a unique constraint. It's not uncommon to have a surrogate key > > for identifying the rows and then an independent uniqueness constraint > > on some natural key for the table, which the user may not have access > > to. > > True, but then the surrogate key would be included in the error > details which would allow the failing row to be identified. Right, and is information which the user would have provided in the query itself. > >> > What do you think about returning just what the user provided in the > >> > first place in both of these cases..? I'm not quite sure what that > >> > would look like in the UPDATE case but for INSERT (and COPY) it would be > >> > the subset of columns being inserted and the values originally provided. > >> > That may not be what the actual conflict is due to, as there could be > >> > before triggers changing things in the middle, or the conflict could be > >> > on default values, but at least the input row could be identified and > >> > there wouldn't be this information leak risk. Not sure how difficult > >> > that would be to implement though. > >> > >> I could see that working for INSERTs, but for UPDATEs I don't think > >> that would be very useful in practice, because the columns most likely > >> to be useful for identifying the failing row (e.g., key columns) are > >> also the columns least likely to have been updated. > > > > I'm not sure that I follow this- if you're not updating the key columns > > then you're not likely to be having a conflict due to them... > > The constraint violation could well be due to updating a non-key > column such as a column with a NOT NULL constraint on it, in which > case only including that column in the error detail wouldn't do much > good -- you'd want to include the key columns if possible. This I can agree with- if the query doesn't include row-identifying information (which implies that they're updating multiple records with one statement) then it'd be helpful to know what row was failing the update. Practically, things get a bit tricky with this though- if we're only going to return the columns which the user has access to, how do we communicate which columns those are? The current error message doesn't contain that information explicitly, it depends on the user being knowledgable of (or able to look up) the table definition. The key violation case only returns the columns of the key violated and we could check that the user has either full table-level SELECT or has SELECT rights to all of the columns involved in the key. We could also check if the user has either table-level SELECT rights or has SELECT rights to all columns in the primary key of the table and then return the primary key in these other cases (what to do if there is no primary key?). I'm not sure if we'd want to back-patch a change like that and I'm a bit worried about the complexity of it in general- having the error message depend so much on the permissions seems like it would make things difficult for anything which is currently using that error message in a programatic way (which I fully expect there are cases of..). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] WITH CHECK and Column-Level Privileges
On 30 September 2014 20:17, Stephen Frost wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> On 30 September 2014 16:52, Stephen Frost 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
* Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 30 September 2014 16:52, Stephen Frost 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
On 30 September 2014 16:52, Stephen Frost wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> On 29 September 2014 16:46, Stephen Frost 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
* Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 29 September 2014 16:46, Stephen Frost 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. > However, for WCOs, I don't think the executor has the > information it needs to work out how to do that because it doesn't > even know which view the user updated, because it's not necessarily > the same one as failed the WCO. That's certainly an issue also. > > It certainly wouldn't be hard to add the same check around the WITH > > OPTION case that's around my proposed solution for the other issues- > > just check for SELECT rights on the underlying table. > > I guess that would work well for RLS, since the user would typically > have SELECT rights on the table they're updating, but I'm not > convinced it would do much good for auto-updatable views. I've been focusing on the 9.4 and back-branches concern, but as for RLS, if we're going to try and include the detail in this case then I'd suggest we do so only if the user has SELECT rights and RLS is disabled on the relation. Otherwise, we'd have to check that the row being returned actually passes the SELECT policy. I'm already not really thrilled that we are changing error message results based on user permissions, and that seems like it'd be worse. > > Another question > > is if we could/should limit this to the UPDATE case. With the INSERT > > case, any columns not provided by the user would be filled out by > > defaults, which can likely be seen in the catalog, or the functions in > > the catalog for the defaults or for any triggers might be able to be > > run by the user executing the INSERT anyway to see what would have been > > used in the resulting row. I'm not completely convinced there's no risk > > there though.. > > > > I think it's conceivable that a trigger could populate a column hidden > to the user with some confidential information, possibly pulled from > another table that the user doesn't have access to, so the fix has to > apply to INSERTs as well as UPDATEs. What do you think about returning just what the user provided in the first place in both of these cases..? I'm not quite sure what that would look like in the UPDATE case but for INSERT (and COPY) it would be the subset of columns being inserted and the values originally provided. That may not be what the actual conflict is due to, as there could be before triggers changing things in the middle, or the conflict could be on default values, but at least the input row could be identified and there wouldn't be this information leak risk. Not sure how difficult that would be to implement though. Thoughts? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WITH CHECK and Column-Level Privileges
On 29 September 2014 16:46, Stephen Frost 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. However, for WCOs, I don't think the executor has the information it needs to work out how to do that because it doesn't even know which view the user updated, because it's not necessarily the same one as failed the WCO. > It certainly wouldn't be hard to add the same check around the WITH > OPTION case that's around my proposed solution for the other issues- > just check for SELECT rights on the underlying table. I guess that would work well for RLS, since the user would typically have SELECT rights on the table they're updating, but I'm not convinced it would do much good for auto-updatable views. Another question > is if we could/should limit this to the UPDATE case. With the INSERT > case, any columns not provided by the user would be filled out by > defaults, which can likely be seen in the catalog, or the functions in > the catalog for the defaults or for any triggers might be able to be > run by the user executing the INSERT anyway to see what would have been > used in the resulting row. I'm not completely convinced there's no risk > there though.. > I think it's conceivable that a trigger could populate a column hidden to the user with some confidential information, possibly pulled from another table that the user doesn't have access to, so the fix has to apply to INSERTs as well as UPDATEs. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* 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
On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed >> 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
* Robert Haas (robertmh...@gmail.com) wrote: > On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed > 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
On Sat, Sep 27, 2014 at 1:19 PM, Dean Rasheed 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
On 27 September 2014 14:33, Stephen Frost 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
* 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) !
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* 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
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* 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
* 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
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