Robert, all, * Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Sep 29, 2014 at 10:26 AM, Stephen Frost <sfr...@snowman.net> wrote: > > In the end, it sounds like we all agree that the right approach is to > > simply remove this detail and avoid the issue entirely. > > Well, I think that's an acceptable approach from the point of view of > fixing the security exposure, but it's far from ideal. Good error > messages are important for usability. I can live with this as a > short-term fix, but in the long run I strongly believe we should try > to do better.
I've been working to try and address this. Attached is a new patch
which moves the responsibility of figuring out what should be returned
down into the functions which build up the error detail which includes
the data (BuildIndexValueDescription and ExecBuildSlotValueDescription).
This allows us to avoid having to change any of the regression tests,
nor do we need to remove the information from the WITH CHECK option.
The patch is against master though it'd need to be back-patched, of
course. This will return either the full and unchanged-from-previous
information, if the user has SELECT on the table or SELECT on the
columns which would be displayed, or "(unknown)" if the user does not
have access to view the entire key (in a key violation case), or the
subset of columns the user has access to (in a "Failing row contains"
case). I'm not wedded to '(unknown)' by any means and welcome
suggestions. If the user does not have table-level SELECT rights,
they'll see for the "Failing row contains" case, they'll get:
Failing row contains (col1, col2, col3) = (1, 2, 3).
Or, if they have no access to any columns:
Failing row contains () = ()
These could be changed, of course. I don't consider this patch quite
ready to be committed and plan to do more testing and give it more
thought, but wanted to put it out there for others to comment on the
idea, the patch, and provide their own thoughts about what is safe and
sane to backpatch when it comes to error message changes like this.
As mentioned up-thread, another option would be to just omit the row
data detail completely unless the user has SELECT rights at the table
level.
Thanks!
Stephen
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
new file mode 100644
index 8849c08..889b813
*** a/src/backend/access/index/genam.c
--- b/src/backend/access/index/genam.c
***************
*** 25,35 ****
--- 25,37 ----
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+ #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
+ #include "utils/syscache.h"
#include "utils/tqual.h"
*************** BuildIndexValueDescription(Relation inde
*** 164,176 ****
Datum *values, bool *isnull)
{
StringInfoData buf;
int natts = indexRelation->rd_rel->relnatts;
int i;
initStringInfo(&buf);
appendStringInfo(&buf, "(%s)=(",
! pg_get_indexdef_columns(RelationGetRelid(indexRelation),
! true));
for (i = 0; i < natts; i++)
{
--- 166,227 ----
Datum *values, bool *isnull)
{
StringInfoData buf;
+ Form_pg_index idxrec;
+ HeapTuple ht_idx;
int natts = indexRelation->rd_rel->relnatts;
int i;
+ int keyno;
+ Oid indexrelid = RelationGetRelid(indexRelation);
+ Oid indrelid;
+ AclResult aclresult;
+
+ /*
+ * Check permissions- if the users does not have access to view the
+ * data in the key columns, we return "unknown" instead.
+ *
+ * First we need to check table-level SELECT access and then, if
+ * there is no access there, check column-level permissions.
+ */
+
+ /*
+ * Fetch the pg_index tuple by the Oid of the index
+ */
+ ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexrelid));
+ if (!HeapTupleIsValid(ht_idx))
+ elog(ERROR, "cache lookup failed for index %u", indexrelid);
+ idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
+
+ indrelid = idxrec->indrelid;
+ Assert(indexrelid == idxrec->indexrelid);
+
+ /* Table-level SELECT is enough, if the user has it */
+ aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /*
+ * No table-level access, so step through the columns in the
+ * index and make sure the user has SELECT rights on all of them.
+ */
+ for (keyno = 0; keyno < idxrec->indnatts; keyno++)
+ {
+ AttrNumber attnum = idxrec->indkey.values[keyno];
+
+ aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
+ ACL_SELECT);
+
+ if (aclresult != ACLCHECK_OK)
+ {
+ /* No access, so clean up and just return 'unknown' */
+ ReleaseSysCache(ht_idx);
+ return "(unknown)";
+ }
+ }
+ }
+ ReleaseSysCache(ht_idx);
initStringInfo(&buf);
appendStringInfo(&buf, "(%s)=(",
! pg_get_indexdef_columns(indexrelid, true));
for (i = 0; i < natts; i++)
{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
new file mode 100644
index a753b20..a33264f
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** static void ExecutePlan(EState *estate,
*** 82,88 ****
DestReceiver *dest);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
! static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
TupleDesc tupdesc,
int maxfieldlen);
static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
--- 82,89 ----
DestReceiver *dest);
static bool ExecCheckRTEPerms(RangeTblEntry *rte);
static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
! static char *ExecBuildSlotValueDescription(Oid reloid,
! TupleTableSlot *slot,
TupleDesc tupdesc,
int maxfieldlen);
static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
*************** ExecConstraints(ResultRelInfo *resultRel
*** 1613,1619 ****
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)));
--- 1614,1621 ----
errmsg("null value in column \"%s\" violates not-null constraint",
NameStr(tupdesc->attrs[attrChk - 1]->attname)),
errdetail("Failing row contains %s.",
! ExecBuildSlotValueDescription(RelationGetRelid(rel),
! slot,
tupdesc,
64)),
errtablecol(rel, attrChk)));
*************** ExecConstraints(ResultRelInfo *resultRel
*** 1630,1636 ****
errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
RelationGetRelationName(rel), failed),
errdetail("Failing row contains %s.",
! ExecBuildSlotValueDescription(slot,
tupdesc,
64)),
errtableconstraint(rel, failed)));
--- 1632,1639 ----
errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
RelationGetRelationName(rel), failed),
errdetail("Failing row contains %s.",
! ExecBuildSlotValueDescription(RelationGetRelid(rel),
! slot,
tupdesc,
64)),
errtableconstraint(rel, failed)));
*************** void
*** 1644,1649 ****
--- 1647,1653 ----
ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
+ Relation rel = resultRelInfo->ri_RelationDesc;
ExprContext *econtext;
ListCell *l1,
*l2;
*************** ExecWithCheckOptions(ResultRelInfo *resu
*** 1679,1685 ****
errmsg("new row violates WITH CHECK OPTION for \"%s\"",
wco->viewname),
errdetail("Failing row contains %s.",
! ExecBuildSlotValueDescription(slot,
RelationGetDescr(resultRelInfo->ri_RelationDesc),
64))));
}
--- 1683,1689 ----
errmsg("new row violates WITH CHECK OPTION for \"%s\"",
wco->viewname),
errdetail("Failing row contains %s.",
! ExecBuildSlotValueDescription(RelationGetRelid(rel), slot,
RelationGetDescr(resultRelInfo->ri_RelationDesc),
64))));
}
*************** ExecWithCheckOptions(ResultRelInfo *resu
*** 1699,1721 ****
* now need to be passed the relation's descriptor.
*/
static char *
! ExecBuildSlotValueDescription(TupleTableSlot *slot,
TupleDesc tupdesc,
int maxfieldlen)
{
StringInfoData buf;
bool write_comma = false;
int i;
!
! /* Make sure the tuple is fully deconstructed */
! slot_getallattrs(slot);
initStringInfo(&buf);
appendStringInfoChar(&buf, '(');
for (i = 0; i < tupdesc->natts; i++)
{
char *val;
int vallen;
--- 1703,1746 ----
* now need to be passed the relation's descriptor.
*/
static char *
! ExecBuildSlotValueDescription(Oid reloid,
! TupleTableSlot *slot,
TupleDesc tupdesc,
int maxfieldlen)
{
StringInfoData buf;
+ StringInfoData collist;
bool write_comma = false;
+ bool write_comma_collist = false;
int i;
! AclResult aclresult;
! bool table_perm = false;
initStringInfo(&buf);
appendStringInfoChar(&buf, '(');
+ /*
+ * Check that the user has permissions to see the row. If the user
+ * has table-level SELECT then that is good enough. If not, we have
+ * to check all the columns.
+ */
+ aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ {
+ /* Set up the buffer for the column list */
+ initStringInfo(&collist);
+ appendStringInfoChar(&collist, '(');
+ }
+ else
+ table_perm = true;
+
+ /* Make sure the tuple is fully deconstructed */
+ slot_getallattrs(slot);
+
for (i = 0; i < tupdesc->natts; i++)
{
+ bool column_perm = false;
char *val;
int vallen;
*************** ExecBuildSlotValueDescription(TupleTable
*** 1723,1759 ****
if (tupdesc->attrs[i]->attisdropped)
continue;
! if (slot->tts_isnull[i])
! val = "null";
! else
{
! Oid foutoid;
! bool typisvarlena;
! getTypeOutputInfo(tupdesc->attrs[i]->atttypid,
! &foutoid, &typisvarlena);
! val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
! }
! if (write_comma)
! appendStringInfoString(&buf, ", ");
! else
! write_comma = true;
! /* truncate if needed */
! vallen = strlen(val);
! if (vallen <= maxfieldlen)
! appendStringInfoString(&buf, val);
! else
{
! vallen = pg_mbcliplen(val, vallen, maxfieldlen);
! appendBinaryStringInfo(&buf, val, vallen);
! appendStringInfoString(&buf, "...");
}
}
appendStringInfoChar(&buf, ')');
return buf.data;
}
--- 1748,1812 ----
if (tupdesc->attrs[i]->attisdropped)
continue;
! if (!table_perm)
{
! aclresult = pg_attribute_aclcheck(reloid, tupdesc->attrs[i]->attnum,
! GetUserId(), ACL_SELECT);
! if (aclresult == ACLCHECK_OK)
! {
! column_perm = true;
! if (write_comma_collist)
! appendStringInfoString(&buf, ", ");
! else
! write_comma_collist = true;
! appendStringInfoString(&collist, NameStr(tupdesc->attrs[i]->attname));
! }
! }
! if (table_perm || column_perm)
{
! if (slot->tts_isnull[i])
! val = "null";
! else
! {
! Oid foutoid;
! bool typisvarlena;
!
! getTypeOutputInfo(tupdesc->attrs[i]->atttypid,
! &foutoid, &typisvarlena);
! val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
! }
!
! if (write_comma)
! appendStringInfoString(&buf, ", ");
! else
! write_comma = true;
!
! /* truncate if needed */
! vallen = strlen(val);
! if (vallen <= maxfieldlen)
! appendStringInfoString(&buf, val);
! else
! {
! vallen = pg_mbcliplen(val, vallen, maxfieldlen);
! appendBinaryStringInfo(&buf, val, vallen);
! appendStringInfoString(&buf, "...");
! }
}
}
appendStringInfoChar(&buf, ')');
+ if (!table_perm)
+ {
+ appendStringInfoString(&collist, ") = ");
+ appendStringInfoString(&collist, buf.data);
+
+ return collist.data;
+ }
+
return buf.data;
}
signature.asc
Description: Digital signature
