Обсуждение: Re: [COMMITTERS] pgsql: Fix column-privilege leak in error-message paths

Stephen Frost <sfrost@snowman.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

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[]).

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
      {          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);
 

(though a comment about it wouldn't be a bad thing either)
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Fix column-privilege leak in error-message paths

[...]

> 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[]).
>
> 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

Commit pushed with this approach.

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

and a comment added explaining it.

Thanks again for pointing it out and please let me know if you see any
further issues.
Stephen