Make inherited LOCK TABLE perform access permission checks on parent table only.

Previously, LOCK TABLE command through a parent table checked
the permissions on not only the parent table but also the children
tables inherited from it. This was a bug and inherited queries should
perform access permission checks on the parent table only. This
commit fixes LOCK TABLE so that it does not check the permission
on children tables.

This patch is applied only in the master branch. We decided not to
back-patch because it's not hard to imagine that there are some
applications expecting the old behavior and the change breaks their
security.

Author: Amit Langote
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CAHGQGwE+GauyG7POtRfRwwthAGwTjPQYdFHR97+LzA4RHGnJxA@mail.gmail.com

diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 329ab849c0bc4ee8da314fd440aedeae87e193fb..d8cafc42bb546c3c97b3e79ee68f65e8b0eabc9f 100644 (file)
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -28,7 +28,7 @@
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
 static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
                                         Oid oldrelid, void *arg);
@@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt)
        if (get_rel_relkind(reloid) == RELKIND_VIEW)
            LockViewRecurse(reloid, lockstmt->mode, lockstmt->nowait, NIL);
        else if (recurse)
-           LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
+           LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
    }
 }
 
@@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 /*
  * Apply LOCK TABLE recursively over an inheritance tree
  *
- * We use find_inheritance_children not find_all_inheritors to avoid taking
- * locks far in advance of checking privileges.  This means we'll visit
- * multiply-inheriting children more than once, but that's no problem.
+ * This doesn't check permission to perform LOCK TABLE on the child tables,
+ * because getting here means that the user has permission to lock the
+ * parent which is enough.
  */
 static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
 {
    List       *children;
    ListCell   *lc;
 
-   children = find_inheritance_children(reloid, NoLock);
+   children = find_all_inheritors(reloid, NoLock, NULL);
 
    foreach(lc, children)
    {
        Oid         childreloid = lfirst_oid(lc);
-       AclResult   aclresult;
 
-       /* Check permissions before acquiring the lock. */
-       aclresult = LockTableAclCheck(childreloid, lockmode, userid);
-       if (aclresult != ACLCHECK_OK)
-       {
-           char       *relname = get_rel_name(childreloid);
-
-           if (!relname)
-               continue;       /* child concurrently dropped, just skip it */
-           aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(childreloid)), relname);
-       }
+       /* Parent already locked. */
+       if (childreloid == reloid)
+           continue;
 
-       /* We have enough rights to lock the relation; do so. */
        if (!nowait)
            LockRelationOid(childreloid, lockmode);
        else if (!ConditionalLockRelationOid(childreloid, lockmode))
@@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
            UnlockRelationOid(childreloid, lockmode);
            continue;
        }
-
-       LockTableRecurse(childreloid, lockmode, nowait, userid);
    }
 }
 
@@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
            if (relkind == RELKIND_VIEW)
                LockViewRecurse(relid, context->lockmode, context->nowait, context->ancestor_views);
            else if (rte->inh)
-               LockTableRecurse(relid, context->lockmode, context->nowait, context->viewowner);
+               LockTableRecurse(relid, context->lockmode, context->nowait);
        }
 
        return query_tree_walker(query,

diff --git a/src/test/regress/expected/lock.out b/src/test/regress/expected/lock.out
index 185fd2f879b2d31dfd6185de7d36349f502f2f58..2e54688dffe32f73daf4ea608b1393ef99d277c0 100644 (file)
--- a/src/test/regress/expected/lock.out
+++ b/src/test/regress/expected/lock.out
@@ -138,15 +138,19 @@ CREATE TABLE lock_tbl3 () INHERITS (lock_tbl2);
 BEGIN TRANSACTION;
 LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
 ROLLBACK;
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Child tables are locked without granting explicit permission to do so as
+-- long as we have permission to lock the parent.
 GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
 SET ROLE regress_rol_lock1;
+-- fail when child locked directly
 BEGIN;
-LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
+LOCK TABLE lock_tbl2;
 ERROR:  permission denied for table lock_tbl2
 ROLLBACK;
 BEGIN;
+LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
+ROLLBACK;
+BEGIN;
 LOCK TABLE ONLY lock_tbl1;
 ROLLBACK;
 RESET ROLE;

diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 297cedbacf11df1bb8c72f4746b84dff0f8a9c64..c2d037b614197511d44806358856df58290c4050 100644 (file)
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -716,6 +716,13 @@ ERROR:  permission denied for table atestc
 TRUNCATE atestp1; -- ok
 TRUNCATE atestc; -- fail
 ERROR:  permission denied for table atestc
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+ERROR:  permission denied for table atestc
+END;
 -- privileges on functions, languages
 -- switch to superuser
 \c -

diff --git a/src/test/regress/sql/lock.sql b/src/test/regress/sql/lock.sql
index 26a7e59a136f02b27f2aa76714d807125c393a7d..e50cb6f06428c2b28b8fbda356449d7b2b15b3ff 100644 (file)
--- a/src/test/regress/sql/lock.sql
+++ b/src/test/regress/sql/lock.sql
@@ -101,10 +101,14 @@ BEGIN TRANSACTION;
 LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
 ROLLBACK;
 
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Child tables are locked without granting explicit permission to do so as
+-- long as we have permission to lock the parent.
 GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
 SET ROLE regress_rol_lock1;
+-- fail when child locked directly
+BEGIN;
+LOCK TABLE lock_tbl2;
+ROLLBACK;
 BEGIN;
 LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
 ROLLBACK;

diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index dfe2603fe2ebfc70685d6487a0d5f9a4919c2efd..2ba69617dcee22b8c024f1297e716ba21a842883 100644 (file)
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -459,6 +459,12 @@ UPDATE atestp1 SET f1 = 1; -- ok
 UPDATE atestc SET f1 = 1; -- fail
 TRUNCATE atestp1; -- ok
 TRUNCATE atestc; -- fail
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+END;
 
 -- privileges on functions, languages