Re: row_security GUC does not behave as documented - Mailing list pgsql-hackers

Stephen Frost <sfrost@snowman.net> writes:
> On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The fine manual says that when row_security is set to off, "queries fail
>> which would otherwise apply at least one policy".  However, a look at
>> check_enable_rls() says that that is a true statement only when the user
>> is not table owner.  If the user *is* table owner, turning off
>> row_security seems to amount to just silently disabling RLS, even for
>> tables with FORCE ROW LEVEL SECURITY.
>>
>> I am not sure if this is a documentation bug or a code bug, but it
>> sure looks to be one or the other.

> The original reason for changing how row_security works was to avoid a
> change in behavior based on a GUC changing. As such, I'm thinking that has
> to be a code bug, as otherwise it would be a behavior change due to a GUC
> being changed in the FORCE RLS case for table owners.

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases.  See attached.

            regards, tom lane

diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index b6c1d75..4e7b4d1 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 59,67 ****
      Oid            user_id = checkAsUser ? checkAsUser : GetUserId();

      /* Nothing to do for built-in relations */
!     if (relid < FirstNormalObjectId)
          return RLS_NONE;

      tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
      if (!HeapTupleIsValid(tuple))
          return RLS_NONE;
--- 59,68 ----
      Oid            user_id = checkAsUser ? checkAsUser : GetUserId();

      /* Nothing to do for built-in relations */
!     if (relid < (Oid) FirstNormalObjectId)
          return RLS_NONE;

+     /* Fetch relation's relrowsecurity and relforcerowsecurity flags */
      tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
      if (!HeapTupleIsValid(tuple))
          return RLS_NONE;
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 88,96 ****
          return RLS_NONE_ENV;

      /*
!      * Table owners generally bypass RLS, except if row_security=true and the
!      * table has been set (by an owner) to FORCE ROW SECURITY, and this is not
!      * a referential integrity check.
       *
       * Return RLS_NONE_ENV to indicate that this decision depends on the
       * environment (in this case, the user_id).
--- 89,97 ----
          return RLS_NONE_ENV;

      /*
!      * Table owners generally bypass RLS, except if the table has been set (by
!      * an owner) to FORCE ROW SECURITY, and this is not a referential
!      * integrity check.
       *
       * Return RLS_NONE_ENV to indicate that this decision depends on the
       * environment (in this case, the user_id).
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 98,128 ****
      if (pg_class_ownercheck(relid, user_id))
      {
          /*
!          * If row_security=true and FORCE ROW LEVEL SECURITY has been set on
!          * the relation then we return RLS_ENABLED to indicate that RLS should
!          * still be applied.  If we are in a SECURITY_NOFORCE_RLS context or if
!          * row_security=false then we return RLS_NONE_ENV.
           *
!          * The SECURITY_NOFORCE_RLS indicates that we should not apply RLS even
!          * if the table has FORCE RLS set- IF the current user is the owner.
!          * This is specifically to ensure that referential integrity checks are
!          * able to still run correctly.
           *
           * This is intentionally only done after we have checked that the user
           * is the table owner, which should always be the case for referential
           * integrity checks.
           */
!         if (row_security && relforcerowsecurity && !InNoForceRLSOperation())
!             return RLS_ENABLED;
!         else
              return RLS_NONE_ENV;
      }

!     /* row_security GUC says to bypass RLS, but user lacks permission */
      if (!row_security && !noError)
          ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!                  errmsg("insufficient privilege to bypass row-level security")));

      /* RLS should be fully enabled for this relation. */
      return RLS_ENABLED;
--- 99,130 ----
      if (pg_class_ownercheck(relid, user_id))
      {
          /*
!          * If FORCE ROW LEVEL SECURITY has been set on the relation then we
!          * should return RLS_ENABLED to indicate that RLS should be applied.
!          * If not, or if we are in an InNoForceRLSOperation context, we return
!          * RLS_NONE_ENV.
           *
!          * InNoForceRLSOperation indicates that we should not apply RLS even
!          * if the table has FORCE RLS set - IF the current user is the owner.
!          * This is specifically to ensure that referential integrity checks
!          * are able to still run correctly.
           *
           * This is intentionally only done after we have checked that the user
           * is the table owner, which should always be the case for referential
           * integrity checks.
           */
!         if (!relforcerowsecurity || InNoForceRLSOperation())
              return RLS_NONE_ENV;
      }

!     /*
!      * We should apply RLS.  However, the user may turn off the row_security
!      * GUC to get a forced error instead.
!      */
      if (!row_security && !noError)
          ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!              errmsg("insufficient privilege to bypass row-level security")));

      /* RLS should be fully enabled for this relation. */
      return RLS_ENABLED;
*** /home/postgres/pgsql/src/test/regress/expected/rowsecurity.out    Sat Dec 19 16:47:11 2015
--- /home/postgres/pgsql/src/test/regress/results/rowsecurity.out    Sun Jan  3 21:56:18 2016
***************
*** 3217,3244 ****
  SET row_security = off;
  -- Shows all rows
  TABLE r1;
!  a
! ----
!  10
!  20
! (2 rows)
!
  -- Update all rows
  UPDATE r1 SET a = 1;
  TABLE r1;
!  a
! ---
!  1
!  1
! (2 rows)
!
  -- Delete all rows
  DELETE FROM r1;
  TABLE r1;
!  a
! ---
! (0 rows)
!
  DROP TABLE r1;
  --
  -- FORCE ROW LEVEL SECURITY does not break RI
--- 3217,3233 ----
  SET row_security = off;
  -- Shows all rows
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  -- Update all rows
  UPDATE r1 SET a = 1;
+ ERROR:  insufficient privilege to bypass row-level security
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  -- Delete all rows
  DELETE FROM r1;
+ ERROR:  insufficient privilege to bypass row-level security
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  DROP TABLE r1;
  --
  -- FORCE ROW LEVEL SECURITY does not break RI
***************
*** 3351,3362 ****
  SET row_security = off;
  -- Rows shown now
  TABLE r1;
!  a
! ----
!  10
!  20
! (2 rows)
!
  SET row_security = on;
  -- Error
  INSERT INTO r1 VALUES (10), (20) RETURNING *;
--- 3340,3346 ----
  SET row_security = off;
  -- Rows shown now
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  SET row_security = on;
  -- Error
  INSERT INTO r1 VALUES (10), (20) RETURNING *;
***************
*** 3379,3402 ****
  -- Show updated rows
  SET row_security = off;
  TABLE r1;
!  a
! ----
!  30
! (1 row)
!
  -- reset value in r1 for test with RETURNING
  UPDATE r1 SET a = 10;
  -- Verify row reset
  TABLE r1;
!  a
! ----
!  10
! (1 row)
!
  SET row_security = on;
  -- Error
  UPDATE r1 SET a = 30 RETURNING *;
! ERROR:  new row violates row-level security policy for table "r1"
  DROP TABLE r1;
  -- Check dependency handling
  RESET SESSION AUTHORIZATION;
--- 3363,3382 ----
  -- Show updated rows
  SET row_security = off;
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  -- reset value in r1 for test with RETURNING
  UPDATE r1 SET a = 10;
+ ERROR:  insufficient privilege to bypass row-level security
  -- Verify row reset
  TABLE r1;
! ERROR:  insufficient privilege to bypass row-level security
  SET row_security = on;
  -- Error
  UPDATE r1 SET a = 30 RETURNING *;
!  a
! ---
! (0 rows)
!
  DROP TABLE r1;
  -- Check dependency handling
  RESET SESSION AUTHORIZATION;

======================================================================