Date: Tue, 14 May 2019 00:36:05 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Multivariate MCV stats can leak data to unprivileged users
Message-ID: <20190513223605.lwj65akict3tpetv@development>
References: 
 <CAEZATCUhT9rt7Ui=Vdx4N==VV5XOK5dsXfnGgVOz_JhAicB=ZA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3vqy7joikhwcykmv"
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCUhT9rt7Ui=Vdx4N==VV5XOK5dsXfnGgVOz_JhAicB=ZA@mail.gmail.com>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk


--3vqy7joikhwcykmv
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

On Fri, May 10, 2019 at 10:19:44AM +0100, Dean Rasheed wrote:
>While working on 1aebfbea83c, I noticed that the new multivariate MCV
>stats feature suffers from the same problem, and also the original
>problems that were fixed in e2d4ef8de8 and earlier --- namely that a
>user can see values in the MCV lists that they shouldn't see (values
>from tables that they don't have privileges on).
>
>I think there are 2 separate issues here:
>
>1). The table pg_statistic_ext is accessible to anyone, so any user
>can see the MCV lists of any table. I think we should give this the
>same treatment as pg_statistic, and hide it behind a security barrier
>view, revoking public access from the table.
>
>2). The multivariate MCV stats planner code can be made to invoke
>user-defined operators, so a user can create a leaky operator and use
>it to reveal data values from the MCV lists even if they have no
>permissions on the table.
>
>Attached is a draft patch to fix (2), which hooks into
>statext_is_compatible_clause().
>

I think that patch is good.

>I haven't thought much about (1). There are some questions about what
>exactly the view should look like. Probably it should translate table
>oids to names, like pg_stats does, but should it also translate column
>indexes to names? That could get fiddly. Should it unpack MCV items?
>

Yeah. I suggest we add a simple pg_stats_ext view, similar to pg_stats.
It would:

(1) translate the schema / relation / attribute names

  I don't see why translating column indexes to names would be fiddly.
  It's a matter of simple unnest + join, no? Or what issues you see?

(2) include values for ndistinct / dependencies, if built

  Those don't include any actual values, so this should be OK. You might
  make the argument that even this does leak a bit of information (e.g.
  when you can see values in one column, and you know there's a strong
  functional dependence, it tells you something about the other column
  which you may not see). But I think we kinda already leak information
  about that through estimates, so maybe that's not an issue.

(3) include MCV list only when user has access to all columns

  Essentially, if the user is missing 'select' privilege on at least one
  of the columns, there'll be NULL. Otherwise the MCV value.

The attached patch adds pg_stats_ext doing this. I don't claim it's the
best possible query backing the view, so any improvements are welcome.


I've been thinking we might somehow filter the statistics values, e.g. by
not showing values for attributes the user has no 'select' privilege on,
but that seems like a can of worms. It'd lead to MCV items that can't be
distinguished because the only difference was the removed attribute, and
so on. So I propose we simply show/hide the whole MCV list.

Likewise, I don't think it makes sense to expand the MCV list in this
view - that works for the single-dimensional case, because then the
list is expanded into two arrays (values + frequencies), which are easy
to process further. But for multivariate MCV lists that's much more
complex - we don't know how many attributes are there, for example.

So I suggest we just show the pg_mcv_list value as is, and leave it up
to the user to call the pg_mcv_list_items() function if needed.

This will also work for histograms, where expanding the value in the
pg_stats_ext would be even trickier.


-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

--3vqy7joikhwcykmv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="add-pg-stats-ext.patch"

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 566100d6df..b1302cfa5d 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -253,6 +253,26 @@ CREATE VIEW pg_stats WITH (security_barrier) AS
 
 REVOKE ALL on pg_statistic FROM public;
 
+CREATE VIEW pg_stats_ext WITH (security_barrier) AS
+    SELECT
+        nspname AS schemaname,
+        relname AS tablename,
+        (SELECT array_agg(attname) FROM unnest(stxkeys) k
+           JOIN pg_attribute a ON (a.attnum = k)
+          WHERE attrelid = stxrelid) AS attnames,
+        stxkind,
+        stxndistinct,
+        stxdependencies,
+        (CASE WHEN EXISTS (SELECT 1 FROM unnest(stxkeys) k
+           JOIN pg_attribute a ON (a.attnum = k)
+          WHERE attrelid = stxrelid AND NOT has_column_privilege(c.oid, a.attnum, 'select')) THEN NULL
+         ELSE stxmcv END) stxmcv
+    FROM pg_statistic_ext s JOIN pg_class c ON (c.oid = s.stxrelid)
+         LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
+    WHERE (c.relrowsecurity = false OR NOT row_security_active(c.oid));
+
+REVOKE ALL on pg_statistic_ext FROM public;
+
 CREATE VIEW pg_publication_tables AS
     SELECT
         P.pubname AS pubname,

--3vqy7joikhwcykmv--