sql: prevent arbitrary writes to system.comments

Previously, the GRANT, UPDATE, DELETE and INSERT privileges
were granted to `public`, i.e. everyone, on `system.comments`.

This was unintended - only users with permissions on an object
should be able to modify that object's comments.

This patch fixes it.

Release note (security update): Any user could previously modify any
database/table/view/index comment via direct SQL updates to
`system.comments`. This was unintended and a form of privilege
escalation, and is now prevented. The privileges required for the
COMMENT statement and `pg_description`, `col_description()`,
`obj_description()` and `shobj_description()` are operating as in
PostgreSQL and unaffected by this change: all users can *view* any
comments on any object (bypassing other privileges), but modifying
comments require write privilege on the target object.

pkg/sql/alter_table.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -1395,10 +1396,11 @@ func injectTableStats(
 func (p *planner) removeColumnComment(
 	ctx context.Context, tableID sqlbase.ID, columnID sqlbase.ColumnID,
 ) error {
-	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"delete-column-comment",
 		p.txn,
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3",
 		keys.ColumnCommentType,
 		tableID,

pkg/sql/comment_on_column.go

@@ -14,8 +14,10 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 )
 
 type commentOnColumnNode struct {
@@ -49,10 +51,11 @@ func (n *commentOnColumnNode) startExec(params runParams) error {
 	}
 
 	if n.n.Comment != nil {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"set-column-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"UPSERT INTO system.comments VALUES ($1, $2, $3, $4)",
 			keys.ColumnCommentType,
 			n.tableDesc.ID,
@@ -62,10 +65,11 @@ func (n *commentOnColumnNode) startExec(params runParams) error {
 			return err
 		}
 	} else {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"delete-column-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3",
 			keys.ColumnCommentType,
 			n.tableDesc.ID,

pkg/sql/comment_on_database.go

@@ -14,6 +14,7 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
@@ -44,10 +45,11 @@ func (p *planner) CommentOnDatabase(
 
 func (n *commentOnDatabaseNode) startExec(params runParams) error {
 	if n.n.Comment != nil {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"set-db-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"UPSERT INTO system.comments VALUES ($1, $2, 0, $3)",
 			keys.DatabaseCommentType,
 			n.dbDesc.ID,
@@ -56,10 +58,11 @@ func (n *commentOnDatabaseNode) startExec(params runParams) error {
 			return err
 		}
 	} else {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"delete-db-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0",
 			keys.DatabaseCommentType,
 			n.dbDesc.ID)

pkg/sql/comment_on_index.go

@@ -14,6 +14,7 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
@@ -77,10 +78,11 @@ func (n *commentOnIndexNode) startExec(params runParams) error {
 func (p *planner) upsertIndexComment(
 	ctx context.Context, tableID sqlbase.ID, indexID sqlbase.IndexID, comment string,
 ) error {
-	_, err := p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+	_, err := p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"set-index-comment",
 		p.Txn(),
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"UPSERT INTO system.comments VALUES ($1, $2, $3, $4)",
 		keys.IndexCommentType,
 		tableID,
@@ -93,10 +95,11 @@ func (p *planner) upsertIndexComment(
 func (p *planner) removeIndexComment(
 	ctx context.Context, tableID sqlbase.ID, indexID sqlbase.IndexID,
 ) error {
-	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"delete-index-comment",
 		p.txn,
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3",
 		keys.IndexCommentType,
 		tableID,

pkg/sql/comment_on_table.go

@@ -14,8 +14,10 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 )
 
 type commentOnTableNode struct {
@@ -42,10 +44,11 @@ func (p *planner) CommentOnTable(ctx context.Context, n *tree.CommentOnTable) (p
 
 func (n *commentOnTableNode) startExec(params runParams) error {
 	if n.n.Comment != nil {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"set-table-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"UPSERT INTO system.comments VALUES ($1, $2, 0, $3)",
 			keys.TableCommentType,
 			n.tableDesc.ID,
@@ -54,10 +57,11 @@ func (n *commentOnTableNode) startExec(params runParams) error {
 			return err
 		}
 	} else {
-		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
+		_, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
 			params.ctx,
 			"delete-table-comment",
 			params.p.Txn(),
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0",
 			keys.TableCommentType,
 			n.tableDesc.ID)

pkg/sql/drop_database.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/internal/client"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -259,10 +260,11 @@ func (p *planner) accumulateDependentTables(
 }
 
 func (p *planner) removeDbComment(ctx context.Context, dbID sqlbase.ID) error {
-	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"delete-db-comment",
 		p.txn,
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0",
 		keys.DatabaseCommentType,
 		dbID)

pkg/sql/drop_table.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -594,21 +595,23 @@ func removeMatchingReferences(
 func (p *planner) removeTableComment(
 	ctx context.Context, tableDesc *sqlbase.MutableTableDescriptor,
 ) error {
-	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+	_, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"delete-table-comment",
 		p.txn,
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0",
 		keys.TableCommentType,
 		tableDesc.ID)
 	if err != nil {
 		return err
 	}
 
-	_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+	_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 		ctx,
 		"delete-comment",
 		p.txn,
+		sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 		"DELETE FROM system.comments WHERE type=$1 AND object_id=$2",
 		keys.ColumnCommentType,
 		tableDesc.ID)

pkg/sql/logictest/testdata/logic_test/privileges_comments

@@ -0,0 +1,82 @@
+# Disable automatic stats to avoid flakiness.
+statement ok
+SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false
+
+subtest regression45707
+
+user root
+
+statement ok
+CREATE DATABASE d45707; CREATE TABLE d45707.t45707(x INT);
+  GRANT SELECT ON DATABASE d45707 TO testuser;
+  GRANT SELECT ON d45707.t45707 TO testuser
+
+statement ok
+COMMENT ON DATABASE d45707 IS 'd45707';
+COMMENT ON TABLE d45707.t45707 IS 't45707';
+COMMENT ON COLUMN d45707.t45707.x IS 'x45707';
+COMMENT ON INDEX d45707.t45707@primary IS 'p45707'
+
+user testuser
+
+statement ok
+SET DATABASE = d45707
+
+# Verify the user cannot modify the comments
+
+statement error user testuser does not have CREATE privilege on database d45707
+COMMENT ON DATABASE d45707 IS 'd45707'
+
+statement error user testuser does not have CREATE privilege on relation t45707
+COMMENT ON TABLE d45707.t45707 IS 't45707'
+
+statement error user testuser does not have CREATE privilege on relation t45707
+COMMENT ON COLUMN d45707.t45707.x IS 'x45707'
+
+statement error user testuser does not have CREATE privilege on relation t45707
+COMMENT ON INDEX d45707.t45707@primary IS 'p45707'
+
+# Verify that the user can view the comments
+
+query T
+SELECT shobj_description(oid, 'pg_database')
+  FROM pg_database
+ WHERE datname = 'd45707'
+----
+d45707
+
+query T
+SELECT col_description(attrelid, attnum)
+  FROM pg_attribute
+ WHERE attrelid = 't45707'::regclass AND attname = 'x'
+----
+x45707
+
+query T
+SELECT obj_description('t45707'::REGCLASS)
+----
+t45707
+
+query T
+SELECT obj_description(indexrelid)
+  FROM pg_index
+ WHERE indrelid = 't45707'::REGCLASS
+   AND indisprimary
+----
+p45707
+
+# Verify that the user can modify the comments.
+
+user root
+
+statement ok
+GRANT ALL ON DATABASE d45707 TO testuser;
+  GRANT ALL ON TABLE d45707.t45707 TO testuser
+
+user testuser
+
+statement ok
+COMMENT ON DATABASE d45707 IS 'd45707_2';
+COMMENT ON TABLE d45707.t45707 IS 't45707_2';
+COMMENT ON COLUMN d45707.t45707.x IS 'x45707_2';
+COMMENT ON INDEX d45707.t45707@primary IS 'p45707_2'

pkg/sql/sem/builtins/pg_builtins.go

@@ -837,9 +837,17 @@ var pgBuiltins = map[string]builtinDefinition{
 					// below to pick up the table comment by accident.
 					return tree.DNull, nil
 				}
+				// Note: the following is equivalent to:
+				//
+				// SELECT description FROM pg_catalog.pg_description
+				//  WHERE objoid=$1 AND objsubid=$2 LIMIT 1
+				//
+				// TODO(jordanlewis): Really we'd like to query this directly
+				// on pg_description and let predicate push-down do its job.
 				r, err := ctx.InternalExecutor.QueryRow(
 					ctx.Ctx(), "pg_get_coldesc",
-					ctx.Txn, `
+					ctx.Txn,
+					`
 SELECT COALESCE(c.comment, pc.comment) FROM system.comments c
 FULL OUTER JOIN crdb_internal.predefined_comments pc
 ON pc.object_id=c.object_id AND pc.sub_id=c.sub_id AND pc.type = c.type

pkg/sql/sqlbase/system.go

@@ -1554,6 +1554,7 @@ func IsReservedID(id ID) bool {
 
 // newCommentPrivilegeDescriptor returns a privilege descriptor for comment table
 func newCommentPrivilegeDescriptor(priv privilege.List) *PrivilegeDescriptor {
+	selectPriv := privilege.List{privilege.SELECT}
 	return &PrivilegeDescriptor{
 		Users: []UserPrivileges{
 			{
@@ -1562,7 +1563,7 @@ func newCommentPrivilegeDescriptor(priv privilege.List) *PrivilegeDescriptor {
 			},
 			{
 				User:       PublicRole,
-				Privileges: priv.ToBitField(),
+				Privileges: selectPriv.ToBitField(),
 			},
 			{
 				User:       security.RootUser,

pkg/sql/truncate.go

@@ -466,10 +466,11 @@ func reassignColumnComment(
 	}
 
 	if comment != nil {
-		_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+		_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 			ctx,
 			"set-column-comment",
 			p.txn,
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"UPSERT INTO system.comments VALUES ($1, $2, $3, $4)",
 			keys.ColumnCommentType,
 			newID,
@@ -479,10 +480,11 @@ func reassignColumnComment(
 			return err
 		}
 
-		_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec(
+		_, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx(
 			ctx,
 			"delete-column-comment",
 			p.txn,
+			sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
 			"DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3",
 			keys.ColumnCommentType,
 			oldID,