Re: [COMMITTERS] pgsql: Fix column-privilege leak in error-message paths

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Fix column-privilege leak in error-message paths
>
> This patch is at least one brick shy of a load:
>
> regression=# create table t1 (f1 int);
> CREATE TABLE
> regression=# create unique index on t1 (abs(f1));
> CREATE INDEX
> regression=# create user joe;
> CREATE ROLE
> regression=# grant insert on t1 to joe;
> GRANT
> regression=# \c - joe
> You are now connected to database "regression" as user "joe".
> regression=> insert into t1 values (1);
> INSERT 0 1
> regression=> insert into t1 values (1);
> ERROR: attribute 0 of relation with OID 45155 does not exist

Urgh.

> The cause of that is the logic added to BuildIndexValueDescription, which
> ignores the possibility that some of the index columns are expressions
> (which will have a zero in indkey[]).

Ah, yes, I didn't expect that, clearly.

> I'm not sure that it's worth trying to drill down and determine exactly
> which column(s) are referenced by an expression. I'd be content if we
> just decided that any index expression is off-limits to someone without
> full SELECT access, which could be achieved with something like

Sounds perfectly reasonable to me.

> {
> AttrNumber attnum = idxrec->indkey.values[keyno];
>
> - aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
> - ACL_SELECT);
> -
> - if (aclresult != ACLCHECK_OK)
> + if (attnum == InvalidAttrNumber ||
> + pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
> + ACL_SELECT) != ACLCHECK_OK)
> {
> /* No access, so clean up and return */
> ReleaseSysCache(ht_idx);

Yup, that looks good to me.

> (though a comment about it wouldn't be a bad thing either)

Agreed.

Will handle shortly.

Thanks!

Stephen