From 83a01b6e5c5f9ddf3da73409de62aed558dd3e4e Mon Sep 17 00:00:00 2001 From: richardjcai Date: Wed, 3 Mar 2021 11:27:35 -0500 Subject: [PATCH] sql: disallow GRANT/REVOKE on system tables Disallow GRANT/REVOKE operations on system objects to avoid potential deadlocks related to version bumps. Release justification: bug fix Release note (sql change): Disallow `GRANT/REVOKE` operations on system tables. Release note (bug fix): Fix a bug where `GRANT/REVOKE` on the `system.lease` table would result in a deadlock. --- pkg/sql/grant_revoke.go | 12 ++++ .../logictest/testdata/logic_test/grant_table | 8 +++ pkg/sql/logictest/testdata/logic_test/system | 68 +++++++++---------- pkg/sql/logictest/testdata/logic_test/user | 7 +- 4 files changed, 58 insertions(+), 37 deletions(-) diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index 63528be6ffc6..2f6d57ff442c 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -14,6 +14,7 @@ import ( "context" "fmt" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" @@ -21,6 +22,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" @@ -185,6 +188,15 @@ func (n *changePrivilegesNode) startExec(params runParams) error { // we update them in KV below. b := p.txn.NewBatch() for _, descriptor := range descriptors { + // Disallow privilege changes on system objects. For more context, see #43842. + op := "REVOKE" + if n.isGrant { + op = "GRANT" + } + if descriptor.GetID() < keys.MinUserDescID { + return pgerror.Newf(pgcode.InsufficientPrivilege, "cannot %s on system object", op) + } + if err := p.CheckPrivilege(ctx, descriptor, privilege.GRANT); err != nil { return err } diff --git a/pkg/sql/logictest/testdata/logic_test/grant_table b/pkg/sql/logictest/testdata/logic_test/grant_table index 684d9175eb97..0455ae381bed 100644 --- a/pkg/sql/logictest/testdata/logic_test/grant_table +++ b/pkg/sql/logictest/testdata/logic_test/grant_table @@ -1922,3 +1922,11 @@ b public t2 root ALL statement error pq: invalid privilege type USAGE for table GRANT USAGE ON t TO testuser + +# Grant / Revoke should not work on system tables. + +statement error pq: cannot GRANT on system object +GRANT SELECT ON system.lease TO testuser + +statement error pq: cannot REVOKE on system object +REVOKE SELECT ON system.lease FROM testuser diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system index dbd2f8adcb78..85321ca55eb8 100644 --- a/pkg/sql/logictest/testdata/logic_test/system +++ b/pkg/sql/logictest/testdata/logic_test/system @@ -468,110 +468,110 @@ DROP DATABASE system statement error user root does not have DROP privilege on relation users DROP TABLE system.users -statement error user root does not have ALL privilege on database system +statement error pq: cannot GRANT on system object GRANT ALL ON DATABASE system TO testuser -statement error user root does not have INSERT privilege on database system +statement error pq: cannot GRANT on system object GRANT GRANT, SELECT, INSERT ON DATABASE system TO testuser -statement ok +statement error pq: cannot GRANT on system object GRANT GRANT, SELECT ON DATABASE system TO testuser -statement error user root does not have ALL privilege on relation namespace +statement error pq: cannot GRANT on system object GRANT ALL ON system.namespace TO testuser -statement error user root does not have INSERT privilege on relation namespace +statement error pq: cannot GRANT on system object GRANT GRANT, SELECT, INSERT ON system.namespace TO testuser -statement ok +statement error pq: cannot GRANT on system object GRANT GRANT, SELECT ON system.namespace TO testuser -statement ok +statement error pq: cannot GRANT on system object GRANT SELECT ON system.descriptor TO testuser # Superusers must have exactly the allowed privileges. -statement error user root does not have ALL privilege on database system +statement error pq: cannot GRANT on system object GRANT ALL ON DATABASE system TO root -statement error user root does not have DELETE privilege on database system +statement error pq: cannot GRANT on system object GRANT DELETE, INSERT ON DATABASE system TO root -statement error user root does not have ALL privilege on relation namespace +statement error pq: cannot GRANT on system object GRANT ALL ON system.namespace TO root -statement error user root does not have DELETE privilege on relation descriptor +statement error pq: cannot GRANT on system object GRANT DELETE, INSERT ON system.descriptor TO root -statement error user root does not have ALL privilege on relation descriptor +statement error pq: cannot GRANT on system object GRANT ALL ON system.descriptor TO root -statement error user root must have exactly GRANT, SELECT privileges on system database with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON DATABASE system FROM root -statement error user root must have exactly GRANT, SELECT privileges on system table with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON system.namespace FROM root -statement error user root does not have ALL privilege on relation namespace +statement error pq: cannot REVOKE on system object REVOKE ALL ON system.namespace FROM root -statement error user root does not have privileges over system table with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT,SELECT ON system.namespace FROM root -statement error user root does not have ALL privilege on database system +statement error pq: cannot GRANT on system object GRANT ALL ON DATABASE system TO admin -statement error user root does not have DELETE privilege on database system +statement error pq: cannot GRANT on system object GRANT DELETE, INSERT ON DATABASE system TO admin -statement error user admin must have exactly GRANT, SELECT privileges on system database with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON DATABASE system FROM admin -statement error user root does not have ALL privilege on relation namespace +statement error pq: cannot GRANT on system object GRANT ALL ON system.namespace TO admin -statement error user root does not have DELETE privilege on relation descriptor +statement error pq: cannot GRANT on system object GRANT DELETE, INSERT ON system.descriptor TO admin -statement error user root does not have ALL privilege on relation descriptor +statement error pq: cannot GRANT on system object GRANT ALL ON system.descriptor TO admin -statement error user admin must have exactly GRANT, SELECT privileges on system table with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON system.descriptor FROM admin -statement error user admin must have exactly GRANT, SELECT privileges on system database with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON DATABASE system FROM admin -statement error user admin must have exactly GRANT, SELECT privileges on system table with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT ON system.namespace FROM admin -statement error user root does not have ALL privilege on relation namespace +statement error pq: cannot REVOKE on system object REVOKE ALL ON system.namespace FROM admin -statement error user admin does not have privileges over system table with ID=.* +statement error pq: cannot REVOKE on system object REVOKE GRANT,SELECT ON system.namespace FROM admin # Some tables (we test system.lease here) used to allow multiple privilege sets for # backwards compatibility, and superusers were allowed very wide privileges. # We make sure this is no longer the case. -statement error user root does not have ALL privilege on relation lease +statement error pq: cannot GRANT on system object GRANT ALL ON system.lease TO testuser -statement error user root does not have CREATE privilege on relation lease +statement error pq: cannot GRANT on system object GRANT CREATE on system.lease to root -statement error user root does not have CREATE privilege on relation lease +statement error pq: cannot GRANT on system object GRANT CREATE on system.lease to admin -statement error user root does not have CREATE privilege on relation lease +statement error pq: cannot GRANT on system object GRANT CREATE on system.lease to testuser -statement error user root does not have ALL privilege on relation lease +statement error pq: cannot GRANT on system object GRANT ALL ON system.lease TO root -statement error user root does not have ALL privilege on relation lease +statement error pq: cannot GRANT on system object GRANT ALL ON system.lease TO admin -statement error user root does not have ALL privilege on relation lease +statement error pq: cannot GRANT on system object GRANT ALL ON system.lease TO testuser # NB: the "order by" is necessary or this test is flaky under DistSQL. diff --git a/pkg/sql/logictest/testdata/logic_test/user b/pkg/sql/logictest/testdata/logic_test/user index bd0283c10555..0351355f0128 100644 --- a/pkg/sql/logictest/testdata/logic_test/user +++ b/pkg/sql/logictest/testdata/logic_test/user @@ -137,9 +137,6 @@ root statement ok ALTER USER testuser CREATEROLE -statement ok -GRANT SELECT ON system.role_options to testuser - user testuser statement ok @@ -148,6 +145,8 @@ CREATE ROLE user4 CREATEROLE statement ok CREATE USER user5 NOLOGIN +user root + query TTT SELECT * FROM system.role_options ---- @@ -156,6 +155,8 @@ user4 CREATEROLE NULL user4 NOLOGIN NULL user5 NOLOGIN NULL +user testuser + statement ok DROP ROLE user4