Thread: Restrict EXPLAIN (ANALYZE) for RLS and security_barrier views

Currently, it is pretty easy to subvert the restrictions imposed
by row-level security and security_barrier views.  All you have to
to is use EXPLAIN (ANALYZE) and see how many rows were filtered
out by the RLS policy or the view condition.

This is not considered a security bug (I asked), but I still think
it should be fixed.

My idea is to forbid EXPLAIN (ANALYZE) for ordinary users whenever
a statement uses either of these features.  But restricting it to
superusers would be too restrictive (with a superuser, you can never
observe RLS, since superusers are exempt) and it would also be
dangerous (you shouldn't perform DML on untrusted tables as superuser).

So I thought we could restrict the use of EXPLAIN (ANALYZE) in these
situations to the members of a predefined role.  That could be a new
predefined role, but I think it might as well be "pg_read_all_stats",
since that role allows you to view sensitive data like the MCV in
pg_statistic, and EXPLAIN (ANALYZE) can be seen as provideing executor
statistics.

Attached is a POC patch that implements that (documentation and
regression tests are still missing) to form a basis for a discussion.

There are a few things I would like feedback about:

- is it OK to use "pg_read_all_stats" for that?

- should the check be moved to standard_ExplainOneQuery()?

Yours,
Laurenz Albe

On Mon, 2024-05-06 at 16:46 +0200, Laurenz Albe wrote:
> Attached is a POC patch that implements that (documentation and
> regression tests are still missing) to form a basis for a discussion.

... and here is a complete patch with regression tests and documentation.

Yours,
Laurenz Albe

On Mon, 6 May 2024 at 15:46, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> Currently, it is pretty easy to subvert the restrictions imposed
> by row-level security and security_barrier views.  All you have to
> to is use EXPLAIN (ANALYZE) and see how many rows were filtered
> out by the RLS policy or the view condition.
>
> This is not considered a security bug (I asked), but I still think
> it should be fixed.
>
> My idea is to forbid EXPLAIN (ANALYZE) for ordinary users whenever
> a statement uses either of these features.
>

Hmm, but there are other ways to see how many rows were filtered out:

 - Use pg_stat_get_tuples_returned()
 - Use pg_class.reltuples
 - Use the row estimates from a plain EXPLAIN

and probably more.

Given that this isn't really a security bug, I think EXPLAIN should
probably be left as-is. Otherwise, you'd have to go round closing all
those other "holes" too.

Regards,
Dean

On Tue, 2024-07-16 at 18:36 +0100, Dean Rasheed wrote:
> On Mon, 6 May 2024 at 15:46, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> >
> > Currently, it is pretty easy to subvert the restrictions imposed
> > by row-level security and security_barrier views.  All you have to
> > to is use EXPLAIN (ANALYZE) and see how many rows were filtered
> > out by the RLS policy or the view condition.
> >
> > This is not considered a security bug (I asked), but I still think
> > it should be fixed.
> >
> > My idea is to forbid EXPLAIN (ANALYZE) for ordinary users whenever
> > a statement uses either of these features.
>
> Hmm, but there are other ways to see how many rows were filtered out:
>
>  - Use pg_stat_get_tuples_returned()

That is true, but it will only work if there is no concurrent DML activity
on the database.  Still, I agree that it would be good to improve that,
for example by imposing a similar restriction on viewing these statistics.

>  - Use pg_class.reltuples

I don't accept that.  The estimated row count doesn't tell me anything
about the contents of the table.

>  - Use the row estimates from a plain EXPLAIN

I don't buy that either.  plain EXPLAIN will just tell you how many
rows PostgreSQL estimates for each node, and it won't tell you how
many rows will get filtered out by a RLS policy.  Also, these are only
estimates.

> and probably more.
>
> Given that this isn't really a security bug, I think EXPLAIN should
> probably be left as-is. Otherwise, you'd have to go round closing all
> those other "holes" too.

The only reason this is not a security bug is because we document these
weaknesses of row-level security and security barrier views.  I don't
think that should count as an argument against improving the situation.

With the exception of the table statistics you mention above, all the
other ways to derive knowledge about the "hidden" data leak very little
information, as does examining the query execution time (which is mentioned
in the documentation).

The information you can glean from EXPLAIN (ANALYZE) is much more
conclusive: "rows removed by filter: 1"

The patch that I am proposing certainly won't close all possibilities
to make an educated guess about "hidden" data, but it will close the
simplest way to reliably subvert RLS and security barrier views.

Is that not a worthy goal?
Shouldn't we plug the most glaring hole in PostgreSQL's data security
features?

I am aware that this change will make performance analysis more
cumbersome, but that's the price to pay for improved security.
I'd be ready to look at restricting pg_stat_get_tuples_returned(),
but perhaps that should be a separate patch.

Yours,
Laurenz Albe

On Mon, 2024-05-06 at 16:46 +0200, Laurenz Albe wrote:
> Currently, it is pretty easy to subvert the restrictions imposed
> by row-level security and security_barrier views.  All you have to
> to is use EXPLAIN (ANALYZE) and see how many rows were filtered
> out by the RLS policy or the view condition.
>
> This is not considered a security bug (I asked), but I still think
> it should be fixed.
>
> My idea is to forbid EXPLAIN (ANALYZE) for ordinary users whenever
> a statement uses either of these features.  But restricting it to
> superusers would be too restrictive (with a superuser, you can never
> observe RLS, since superusers are exempt) and it would also be
> dangerous (you shouldn't perform DML on untrusted tables as superuser).
>
> So I thought we could restrict the use of EXPLAIN (ANALYZE) in these
> situations to the members of a predefined role.  That could be a new
> predefined role, but I think it might as well be "pg_read_all_stats",
> since that role allows you to view sensitive data like the MCV in
> pg_statistic, and EXPLAIN (ANALYZE) can be seen as provideing executor
> statistics.

After a discussion on the pgsql-security list (ZppxEDkyPd6tCSiq@msg.df7cb.de),
I am going to mark this patch as rejected.

The gist of that discussion was that even without EXPLAIN (ANALYZE),
it is easy enough for a determined attacker who can run arbitrary
SQL to subvert row-level security.
Therefore, restricting EXPLAIN (ANALYZE) will do more harm than good,
since it will make analyzing query performance harder without a
real security gain.

Yours,
Laurenz Albe