Re: Row-security writer-side checks proposal

Hi

I've been looking some more into write-side checks in row-security and
have a suggestion.

Even though write-side checks are actually fairly separate to read
checks, and can be done as another step, I'd like to think about them
before the catalog format and syntax are settled. I think we need fields
for write operations in pg_rowsecurity and the syntax to set them so
that the catalog information can be used by triggers to enforce write
checks. Even if, for the first cut, they're not supported by built-in
auto-created triggers.

Here's my proposal, let me know what you think:

SET ROW SECURITY FOR { ALL COMMANDS | {[SELECT,INSERT,UPDATE,DELETE}+}

in other words, you specify either:

SET ROW SECURITY FOR ALL COMMANDS

or a command-list like:

SET ROW SECURITY FOR INSERT OR UPDATE OR DELETE

(Intentionally the same as CREATE TRIGGER ... FOR INSERT OR UPDATE OR
DELETE ...)

The subtlety here is that the "SELECT" clause applies to the *read part*
of an UPDATE or DELETE too, just like the current implementation. That
protects us against leaks via RETURNING, and ensures that the
row-security policy is consistent. The "INSERT", "UPDATE" or "DELETE"
part of the policy would *only* be used by write checks that verify that
a new tuple being written meets the row-security criteria.

For INSERT, that's obvious: check the insert policy and see if the tuple
should be allowed; if not, raise permission denied. The SELECT predicate
doesn't matter since it's not reading from the target table (except
possibly via join/subquery, where it is already applied).

For UPDATE, we only ever try to update tuples the select policy allows
us to see. Row security already does this thanks to Kohei KaiGai's great
work. The write side check (just a trigger) only needs to make sure the
new tuple meets the UPDATE predicate.

For DELETE, the predicate controls whether the user can delete the
tuple, so it's possible to have row-security policies that let users
read but not delete some tuples.

If the catalog fields and syntax for setting them are included in the
patch the first time around then users can use that information in their
own triggers, and we can provide canned ones in the documentation if we
run out of time to write C triggers that are automatically created like
FK checks are.

That keeps the patch smaller, since it separates the write and read
row-security.

Opinions?

I'm cooking up an adjustment to Kohei KaiGai's RLS patch with this
change now. Meanwhile I'm attaching my most recent update of his patch,
which includes:

* Rebase on top of head

* Documentation updates and rewording

* Fixes some missed renaming of "rls" and variants to "rowsecurity"

* Additional regression tests demonstrating the problems with
handling of portals (cursors and SECURITY DEFINER functions
returning refcursor). These tests intentionally fail as their
expected file contains what _should_ happen not what does.

* Additional regression tests demonstrating that foreign key
enforcement is affected inconsistently by RLS, including the
broken example with superuser I posted previously. Again
it intentionally fails with expected containing what I think
should happen.

* Test cases to demonstrate that RS leaks information via
UNIQUE constraints and that this is expected.

* Documentation on use of pg_get_expr for decoding the
rowsecurity expressions in pg_rowsecurity into readable
SQL expressions usable via EXECUTE

The current tree is here, rebased on top of today's master:

https://github.com/ringerc/postgres/tree/rls-9.4

(this branch is rebased regularly!)

I've attached an updated squashed patch against today's master/head for
anyone who wants to give it a go or take a look. "make check" is
supposed to fail, since what should happen isn't yet what the code
actually does.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 1, 2013 at 3:52 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> I've been looking some more into write-side checks in row-security and
> have a suggestion.
>
> Even though write-side checks are actually fairly separate to read
> checks, and can be done as another step, I'd like to think about them
> before the catalog format and syntax are settled. I think we need fields
> for write operations in pg_rowsecurity and the syntax to set them so
> that the catalog information can be used by triggers to enforce write
> checks. Even if, for the first cut, they're not supported by built-in
> auto-created triggers.
>
> Here's my proposal, let me know what you think:
>
> SET ROW SECURITY FOR { ALL COMMANDS | {[SELECT,INSERT,UPDATE,DELETE}+}
>
> in other words, you specify either:
>
> SET ROW SECURITY FOR ALL COMMANDS

I continue to think that this syntax is misguided. For SELECT and
DELETE there is only read-side security, and for INSERT there is only
write-side security, so that's OK as far as it goes, but for UPDATE
both read-side security and write-side security are possible, and
there ought to be a way to get one without the other. This syntax
won't support that cleanly.

I wonder whether it's worth thinking about the relationship between
the write-side security contemplated for this feature iand the WITH
CHECK OPTION syntax that we have for auto-updateable views, which
serves more or less the same purpose. I'm not sure that syntax is any
great shakes, but it's existing precedent of some form and could
perhaps at least be looked at as a source of inspiration.

I would generally expect that most people would want either "read side
security for all commands" or "read and write side security for all
commands". I think whatever syntax we come up with this feature ought
to make each of those things straightforward to get.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/04/2013 09:55 PM, Robert Haas wrote:
> I continue to think that this syntax is misguided. For SELECT and
> DELETE there is only read-side security, and for INSERT there is only
> write-side security, so that's OK as far as it goes, but for UPDATE
> both read-side security and write-side security are possible, and
> there ought to be a way to get one without the other. This syntax
> won't support that cleanly.

That's what I was thinking earlier too - separate "FOR READ" and "FOR
WRITE" instead.

The reason I came back to insert/update/delete was that it's entirely
reasonable to want to prohibit deletes but permit updates to the same
tuple. Both are writes; both set xmax, it's just that one _replaces_ the
tuple, the other doesn't.

So really, there are four cases:

READ
WRITE INSERT
WRITE UPDATE
WRITE DELETE

> I wonder whether it's worth thinking about the relationship between
> the write-side security contemplated for this feature iand the WITH
> CHECK OPTION syntax that we have for auto-updateable views, which
> serves more or less the same purpose. I'm not sure that syntax is any
> great shakes, but it's existing precedent of some form and could
> perhaps at least be looked at as a source of inspiration.

I've been thinking about the overlap with WITH CHECK OPTION as well.

> I would generally expect that most people would want either "read side
> security for all commands" or "read and write side security for all
> commands". I think whatever syntax we come up with this feature ought
> to make each of those things straightforward to get.

but sometimes with different predicates for read and write, i.e. you can
see rows you can't modify or can insert rows / update rows that you
can't see after the change.

Similarly, saying you can update but not delete seems quite reasonable
to me.

On the other hand, we might choose to say "if you want to do things with
that granularity use your own triggers to enforce it" and provide only
READ and WRITE for RLS.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Nov 4, 2013 at 8:00 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 11/04/2013 09:55 PM, Robert Haas wrote:
>> I continue to think that this syntax is misguided. For SELECT and
>> DELETE there is only read-side security, and for INSERT there is only
>> write-side security, so that's OK as far as it goes, but for UPDATE
>> both read-side security and write-side security are possible, and
>> there ought to be a way to get one without the other. This syntax
>> won't support that cleanly.
>
> That's what I was thinking earlier too - separate "FOR READ" and "FOR
> WRITE" instead.
>
> The reason I came back to insert/update/delete was that it's entirely
> reasonable to want to prohibit deletes but permit updates to the same
> tuple. Both are writes; both set xmax, it's just that one _replaces_ the
> tuple, the other doesn't.
>
> So really, there are four cases:
>
> READ
> WRITE INSERT
> WRITE UPDATE
> WRITE DELETE

Isn't READ similarly divisible into READ SELECT, READ UPDATE, and READ DELETE?

>> I would generally expect that most people would want either "read side
>> security for all commands" or "read and write side security for all
>> commands". I think whatever syntax we come up with this feature ought
>> to make each of those things straightforward to get.
>
> but sometimes with different predicates for read and write, i.e. you can
> see rows you can't modify or can insert rows / update rows that you
> can't see after the change.

Yes, that's possible.

> Similarly, saying you can update but not delete seems quite reasonable
> to me.

If you simply want to allow UPDATE but not DELETE, you can refrain
from granting the table-level privilege. The situation in which you
need things separate is when you want to allow both UPDATE and DELETE
but with different RLS quals for each.

> On the other hand, we might choose to say "if you want to do things with
> that granularity use your own triggers to enforce it" and provide only
> READ and WRITE for RLS.

The funny thing about this whole feature is that it's just syntax
support for doing things that you can already do in other ways. If
you want read-side security, create a security_barrier view and select
from that instead of hitting the table directly. If you want
write-side security, enforce it using triggers. So essentially what
this is, I think, is an attempt to invent nicer syntax around
something that we already have, and provide a one-stop-shopping
experience rather than a roll-your-own experience for people who want
row-level security.

Now maybe that's fine. But given that, I think it's pretty important
that we get the syntax right. Because if you're adding a feature
primarily to add a more convenient syntax, then the syntax had better
actually be convenient.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Now maybe that's fine. But given that, I think it's pretty important
> that we get the syntax right. Because if you're adding a feature
> primarily to add a more convenient syntax, then the syntax had better
> actually be convenient.

I agree that we want to get the syntax correct, but also very clear as
it's security related and we don't want anyone surprised by what happens
when they use it. The idea, as has been discussed in the past, is to
then allow tying RLS in with SELinux and provide MAC.

Thanks,

Stephen

On Tue, Nov 5, 2013 at 9:01 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Now maybe that's fine. But given that, I think it's pretty important
>> that we get the syntax right. Because if you're adding a feature
>> primarily to add a more convenient syntax, then the syntax had better
>> actually be convenient.
>
> I agree that we want to get the syntax correct, but also very clear as
> it's security related and we don't want anyone surprised by what happens
> when they use it. The idea, as has been discussed in the past, is to
> then allow tying RLS in with SELinux and provide MAC.

No argument. I think "convenient" and "unsurprising" are closely-aligned goals.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/05/2013 09:30 PM, Robert Haas wrote:
>> So really, there are four cases:
>>
>> READ
>> WRITE INSERT
>> WRITE UPDATE
>> WRITE DELETE
>
> Isn't READ similarly divisible into READ SELECT, READ UPDATE, and READ DELETE?

Not in my opinion. No matter what the command, the read side is all
about having some way to obtain the contents of the tuple.

Separate "READ DELETE" etc would only be interesting if we wanted to let
someone DELETE rows they cannot SELECT. Since we have DELETE ...
RETURNING, and since users can write a predicate function for DELETE
that leaks the information even if we didn't, in practice if you give
the user any READ right you've given them all of them. So I don't think
we can support that (except maybe by column RLS down the track).

By contrast on the write side it seems routine to need different rules
for different operations. The traditional heriachical mandatory access
model as implemented by Teradata Row Level Security, Oracle Label Based
Security, etc, can have different rules for each operation. Here's a
synopsis of the example described in the Teradata docs as a typical policy:

- SELECT: Current session security label must be >= the row label

- INSERT: Current session security label must be = the new row label

- UPDATE: Session label must be >= row label to update the row.
New row must be = session security label.

- DELETE: Only permitted for rows with the lowest label set, ensuring
row is reviewed and declassified before deletion.

Except for the DELETE, this is actually just two policies, one for reads
(session label => row label) and one for writes (session label = new row
label). So this might be an acceptable constraint if necessary, but it'd
be really good to support per-command rules, and we certainly need
asymmetric read- and write- rules.

I'm looking into use cases and existing examples to put this in a more
concerete context, as much of the RLS discussion has been a hypothetical
one that doesn't look at concrete user problems much.

>> Similarly, saying you can update but not delete seems quite reasonable
>> to me.
>
> If you simply want to allow UPDATE but not DELETE, you can refrain
> from granting the table-level privilege. The situation in which you
> need things separate is when you want to allow both UPDATE and DELETE
> but with different RLS quals for each.

That's what I was getting at, yes. Like the example above; the set of
rows you can update might be different to the set of rows you can delete.

>> On the other hand, we might choose to say "if you want to do things with
>> that granularity use your own triggers to enforce it" and provide only
>> READ and WRITE for RLS.
>
> The funny thing about this whole feature is that it's just syntax
> support for doing things that you can already do in other ways. If
> you want read-side security, create a security_barrier view and select
> from that instead of hitting the table directly. If you want
> write-side security, enforce it using triggers.

Right now you can't have both together, though; an UPDATE on the raw
table can observe rows that wouldn't be visible via the view and can
send them to the client via RAISE NOTICE or whatever.

Support for automatically updatable security barrier views would take
care of this issue, at which point I'd agree: RLS becomes mostly
cosmetic syntactical sugar over existing capabilities. FKs would ignore
RLS, much like the would if you use explicit SECURITY BARRIER views and
have FKs between the base tables.

One big difference still remains though: when you add an RLS policy on a
table, all procedures and views referring to that table automatically
use the transparent security barrier view over the table instead. That's
*not* the case when you use views manually; you have to re-create views
that point to the table so they instead point to a security barrier view
over the table. Again it's nothing you can't do with updatable security
barrier views, but it's automatic and transparent with RLS.

> Now maybe that's fine. But given that, I think it's pretty important
> that we get the syntax right. Because if you're adding a feature
> primarily to add a more convenient syntax, then the syntax had better
> actually be convenient.

I completely agree with that. I don't have a strong opinion on the
current syntax.

I've looked at how some other vendors do it, and I can't say their
approaches are pretty.

Oracle VPD has you create a PL/SQL procedure to generate the SQL text of
the desired RLS predicate. It then wants you to create a POLICY (via a
PL/SQL call to a built-in package) that associates the predicate
generation function with a table and sets some options controlling what
statement types it applies to, when the predicate is re-generated, etc.
It also has policy groups, which bundle policies together and control
whether or not they're applied for a given session. The predicates of
different policies are ANDed together.

So to create a single RLS policy on a single table you have to write a
PL/SQL stored procedure that generates an SQL predicate and then create
a RLS policy to apply that procedure to the table.

ALTER TABLE ... SET ROW SECURITY is the equivalent of adding the policy;
Pg RLS doesn't have an equivalent of the predicate generating function,
instead only supporting static predicates. (There might be patent issues
around using a predicate-generator function; I haven't looked, but think
it was mentioned in earlier discussion).

Teradata instead eschews general-purpose row level security. So it's not
really within the scope of the current RLS feature, comparing instead to
whatever we build on top of it (using SEPostgreSQL or otherwise). It
implements label based security directly, with two kinds of labels. You
can create a CONSTRAINT with either a single hirachical compartment
(think "secret", "top secret", etc) or a CONSTRAINT with set of discrete
and isolated security compartment labels. The CONSTRAINT has functions
written in C associated with it to do enforcement, one for each
statement type. The C functions must not execute SQL. A CONSTRAINT can
be assigned to tables and to users in an m:n manner. A user can have
multiple constraints, as can a table. They're ANDed. A user with
non-heirachical "country" constraint set to "uk" can only see rows with
country label "uk"; a user with heirachical "classification" constraint
set to "unclassified (default), classified" can see rows labeled
"unclassified" and, if they elevate their session, "classified", but
cannot see higher rows.

Both of these have a concept that Pg RLS doesn't seem to have: multiple
RLS policies. I think that's actually quite important to consider,
because we'll need that anyway to support RLS on a subset of columns.
Both also have the concept of turning particular RLS policies on and off
on a per-user basis or per-session using privileged on-login triggers,
so that application A and application B can apply different RLS rules on
the same data.

I don't think it's important to cover these from the start, but it'd be
a good idea not to foreclose these possibilities in whatever gets into Pg.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/05/2013 10:01 PM, Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Now maybe that's fine. But given that, I think it's pretty
>> important that we get the syntax right. Because if you're adding
>> a feature primarily to add a more convenient syntax, then the
>> syntax had better actually be convenient.
>
> I agree that we want to get the syntax correct, but also very clear
> as it's security related and we don't want anyone surprised by what
> happens when they use it. The idea, as has been discussed in the
> past, is to then allow tying RLS in with SELinux and provide MAC.

That was my impression also.

To help get closer to that point, since you were involved in the work
on auto-updatable views: any hints on what might be needed to tackle
making security barrier views updatable?

There's a fun little wrinkle with MAC, by the way: functional indexes.
We can't allow the creation of a functional index, even by the table
owner, if it uses any non-LEAKPROOF operators and functions. Otherwise
the user can write a function to leak the rows, then create an index
using that function.

That's not a problem for the current phase of RLS because the table
owner is allowed to remove the RLS constraint directly. They can also
add triggers that might leak rows via CASCADEs, etc. When MAC comes
into the picture we'll need to impose limits on triggers and
functional indexes added to rows.

- --
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSefGAAAoJELBXNkqjr+S2W6EH+wc3fM3GGoYjnietLfGiiFmA
4ea7sIcio9kdDap3dNpgnMW2NfEHu/OLxSptFGBjl3w4RfA1KSQaKcwupjmanPGa
har7MylI4SKDRHB5LWZEgYrK1A3n/PTJUap3DFGhLJxAdCMM3AtQfcyHBoj/LXfZ
9o9KkpXfzFW2e4yuPR714rZMzfAgO+Jyij9WkcayNASw/0jnCuhCdBtg8mKU6mhz
lC4KA0WGxXqCGDdKxPwVRSJTMoT8kBeUBf4lznSEeGspxCHb4GafMCFvhHarQ9WU
+aBY1mw3ELFXqfPurLC5RZVQGYsygWfzrREJ+oHUJ3khgPR2djj0EAemK3lwO6M=
=HYU7
-----END PGP SIGNATURE-----

On Wed, Nov 6, 2013 at 2:27 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Separate "READ DELETE" etc would only be interesting if we wanted to let
> someone DELETE rows they cannot SELECT. Since we have DELETE ...
> RETURNING, and since users can write a predicate function for DELETE
> that leaks the information even if we didn't, in practice if you give
> the user any READ right you've given them all of them. So I don't think
> we can support that (except maybe by column RLS down the track).

Well, we could require SELECT privilege when a a RETURNING clause is present...

> Except for the DELETE, this is actually just two policies, one for reads
> (session label => row label) and one for writes (session label = new row
> label). So this might be an acceptable constraint if necessary, but it'd
> be really good to support per-command rules, and we certainly need
> asymmetric read- and write- rules.

OK.

> Support for automatically updatable security barrier views would take
> care of this issue, at which point I'd agree: RLS becomes mostly
> cosmetic syntactical sugar over existing capabilities. FKs would ignore
> RLS, much like the would if you use explicit SECURITY BARRIER views and
> have FKs between the base tables.
>
> One big difference still remains though: when you add an RLS policy on a
> table, all procedures and views referring to that table automatically
> use the transparent security barrier view over the table instead. That's
> *not* the case when you use views manually; you have to re-create views
> that point to the table so they instead point to a security barrier view
> over the table. Again it's nothing you can't do with updatable security
> barrier views, but it's automatic and transparent with RLS.

That's true, but it's that automatic transparent part that also
introduces a lot of pain, because what do you do when you need to
really get at the real data (e.g. to back it up)? The ad-hoc rule
"superusers are exempt" solves the problem at one level, but it
doesn't do a lot for e.g. database owners.

> I've looked at how some other vendors do it, and I can't say their
> approaches are pretty.

Did you look at Trusted RUBIX?

> Both of these have a concept that Pg RLS doesn't seem to have: multiple
> RLS policies. I think that's actually quite important to consider,
> because we'll need that anyway to support RLS on a subset of columns.
> Both also have the concept of turning particular RLS policies on and off
> on a per-user basis or per-session using privileged on-login triggers,
> so that application A and application B can apply different RLS rules on
> the same data.
>
> I don't think it's important to cover these from the start, but it'd be
> a good idea not to foreclose these possibilities in whatever gets into Pg.

I agree, and I'm not sure we're there yet. Frankly, switching from a
single security policy per table to multiple policies per table
doesn't sound like a good candidate for a follow-on commit; it's
likely to have fundamental ramifications for the syntax, and I'm not
eager to see us implement one syntax now only to overhaul it in the
next release.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/08/2013 11:03 PM, Robert Haas wrote:
>> > Separate "READ DELETE" etc would only be interesting if we wanted to let
>> > someone DELETE rows they cannot SELECT. Since we have DELETE ...
>> > RETURNING, and since users can write a predicate function for DELETE
>> > that leaks the information even if we didn't, in practice if you give
>> > the user any READ right you've given them all of them. So I don't think
>> > we can support that (except maybe by column RLS down the track).
>
> Well, we could require SELECT privilege when a a RETURNING clause is present...

Absolutely could. Wouldn't stop them grabbing the data via a predicate
function on the update/delete, though, and we can't sanely (IMO) require
SELECT rights if they want to use non-LEAKPROOF functions/operators either.

I do think this needs looking at further, but I suspect it's an area
where Pg's flexibility will make life harder.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 9, 2013 at 10:01 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 11/08/2013 11:03 PM, Robert Haas wrote:
>>> > Separate "READ DELETE" etc would only be interesting if we wanted to let
>>> > someone DELETE rows they cannot SELECT. Since we have DELETE ...
>>> > RETURNING, and since users can write a predicate function for DELETE
>>> > that leaks the information even if we didn't, in practice if you give
>>> > the user any READ right you've given them all of them. So I don't think
>>> > we can support that (except maybe by column RLS down the track).
>>
>> Well, we could require SELECT privilege when a a RETURNING clause is present...
>
> Absolutely could. Wouldn't stop them grabbing the data via a predicate
> function on the update/delete, though, and we can't sanely (IMO) require
> SELECT rights if they want to use non-LEAKPROOF functions/operators either.

Hmm, good point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company