Thread: 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
Attachment
On Fri, Nov 1, 2013 at 3:52 AM, Craig Ringer <craig@2ndquadrant.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@2ndquadrant.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@gmail.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@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.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@gmail.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@2ndquadrant.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@2ndquadrant.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