Re: API change advice: Passing plan invalidation info from the rewriter into the planner?

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I'm really concerned about the security implications of this patch. I
> think we're setting ourselves up for a whole lot of hurt for somewhat
> unclear gain.

I'm certainly of a different opinion and, for the most part, I feel that
if there are security concerns then they need to be addressed- and
better by us than by asking users to use some other mechanism to
implement RLS.

> In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically
> *is* row-level security: instead of applying a row-level security
> policy to a table, just create a security-barrier view over the table
> and grant access to the view. Forget that the table ever existed.
> Done.

This argument could have been made for column-level privileges also, no?
Yet I don't hear any calls for that to be ripped out now that you could
implement it through updatable security-barrier views. That commit was
the ground-work to allow us to finally get proper RLS and I'm very
disappointed to hear that the mechanical pieces around making RLS easy
for users to use (and getting that check-box taken care of in a wide
variety of fields that we are being exposed to now, see the PGConf.NYC
keynote speakers...) is receiving such push-back.

> With this approach, there's a lot of stuff that we don't have to
> reinvent. We've talked a lot about whether row-level security should
> only be concerned with the rows it scans, or whether it should also
> restrict the new rows that can be created. You can get either
> behavior by choosing whether or not to use WITH CHECK OPTION. And
> then there's this question of who should be RLS-exempt; that's
> basically a question of to whom you grant privileges on the underlying
> table. Note that this can be very fine-grained: for example, you can
> allow someone to exempt themselves for selects but not for updates by
> granting them SELECT privileges but not UPDATE privileges on the
> underlying table. And potentially-exempt users can choose whether
> they want a particular access to actually be exempt by targeting the
> view when they don't want to be exempt and the table when they do.

I agree that views, or even security-definer functions, offer a great
deal of flexibility, and that may be necessary in some use-cases, but I
fail to see why that means we should avoid providing the mechanics to
achieve simple and usable RLS akin to what other major RDBMS's have.

> That's mighty useful for debugging, at least IMHO. And, if you want
> to have several row-level security policies for different classes of
> users, just create more than one view and grant different privileges
> on each.

I'm really not impressed with the idea that RLS should be done with
multiple different views of the same underlying table.

> By contrast, it seems to me that every design so far proposed for
> something that is actually called row-level security - as opposed to
> commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is*
> row-level security, is extremely limited. Look back at all the things
> listed in the previous paragraph; can you do those things easily with
> the designs that have been proposed? As far as I can see, not really.

I don't feel that RLS will, or even *should*, have the same level of
flexibility that you can achieve with views and/or security definer
functions. I expect that, over time, we will add more capabilities to
it, but it's never going to be able to redefine the contents of a column
as a view can, nor will it be able to add columns to a table as views
can. I don't see those as reasons against having support for RLS.

> Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough
> equivalent of WITH CHECK OPTION, probably because we've talked a lot
> about that specific issue, but it doesn't line up exactly to what WITH
> CHECK OPTION actually does. There's no independently-grantable
> RLS-exemption privilege - and even when we talk about that, it's
> usually some kind of global bit that applies to all tables and all
> operations equally - whereas with the above approach it can be
> per-table and per-operation and doesn't require superuser intervention
> to flip the bit.

I'm glad to hear your thoughts on the level of granularity which might
be nice to have with RLS. What would be great is to spend a bit more
time reviewing what other systems provide in this area and considering
what makes sense for us. This will also be a feature and an area which
we will be improving for a long time to come, but we do need this
capability and we have to start somewhere.

> There's no way for users who are RLS exempt to turn
> off their exemption for testing purposes, let alone on a per-table
> basis.

I don't follow this argument entirely- users can't turn off the existing
permissions system for testing either, unless an authorized user with
the correct permissions makes the change to allow it- or the user bumps
themselves up to superuser, or to a role which has broader permissions,
both of which would also be possible to do with RLS.

> There's no way to have multiple RLS policies on a single
> table. All of those are things that we get "for free" in the
> view-over-table model, and implementing formal RLS basically requires
> us to either invent a new RLS-specific way of doing each of those
> things, or suffer along with a subset of the functionality. Yuck.

What would probably be good is to review the use-cases which the current
patch already addresses- and we've had good responses from actual users
who are already playing with the patch and are hearing that it is
addressing their requirements.

> But what's really awful about this whole design is that it breaks the
> invariant that reading from a table doesn't run anybody else's code.

You're suggesting that we use views instead, which clearly could run
someone else's code. Perhaps the user will notice that they're
selecting from a view instead of a table, but I've never seen a security
design around making sure that what is being select'd from is a table
vs. a view. Have you seen applications which implement such a check
prior to running a query?

> It's already the case that users need to be awfully careful about
> modifying tables, because that might fire triggers that do bad things.
> But at least you can SELECT from a table and it will either work, or
> it will fail with a permission denied error. What it will not do is
> unexpectedly run some code that you weren't expecting it to run. You
> can't be so blithe about selecting from views, but reading a plain
> table is always OK. Now, as soon as we introduce the concept that
> selecting from a table might not really mean "read from the table" but
> "read from the table after applying this owner-specified qual", we're
> opening up a whole new set of attack surfaces.

With this, I agree, there is risk associated with the implementation
we're looking at for RLS. We could narrow the case by reducing the
capabilities of RLS in PG by only allowing certain functions to be used
in the definition of a RLS policy (eg: btree operators of known data
types, or something similar to our "leak-proof" attribute), but I don't
see that it really buys us much. There are a *lot* of ways in which an
individual who has the ability to create objects inside the database can
cause problems, but that comes with the flexibility we provide users
with. That will always be a balance but, I believe, we wouldn't have
the same level of success or have such an awesome system without that
flexibility.

> Every pg_dump is an
> opportunity to hack somebody else's account, or at least audit their
> activity. Protecting the superuser against everybody else is nice,
> but I think it's just as important to protect non-superusers against
> each other, and I think that's going to be hard -- because in the RLS
> world, SELECT * FROM tab is now *fundamentally* ambiguous. Maybe it's
> reading from the table, and maybe it's really clandestinely reading
> from a view over the table, and the user has no way of being really
> clear about which behavior they want. From a security point of view,
> that seems very bad.

I don't see this as being an insurmountable issue. I agree that having
a way for pg_dump to run safely is important and the superuser check
does address that, given that we don't have a "read-only (and
everything)" capability today. Once we do (and I surely hope that will
come sooner rather than later), such a role should also have the 'no
RLS' bit, as it wouldn't make any sense for such a role anyway. The
lack of that is not a strike against RLS though.

> To recap:
>
> 1. Reinventing RLS-specific ways to do all of the things that can
> already be done in the view-over-table model is a lot of work.

I agree that there's a fair bit of work involved, but I do not see
reimplementing views as RLS as the goal.

> 2. There's a danger that the functionality available in the two models
> will diverge, so that certain things can only be done in one world or
> the other.

They will always be distinct, intentionally so.

> 3. On the whole, it seems likely that the RLS-specific world will
> remain impoverished compared to the view-over-table model.

Agreed. As is the case with views vs. security definer functions.

> 4. Making SELECT * FROM tab ambiguous seems likely to be a security minefield.

While I agree that we need to consider this, I don't think it will be a
"minefield", but rather something we need to document and educate our
users about. If you'd like a "disable-all-RLS" GUC, I'm all for it.

Tossing out any hope of having RLS in PG is tossing the baby out with
the bathwater though, imv.

Thanks,

Stephen