Skip to content

Commit

Permalink
Merge #45712 #45729
Browse files Browse the repository at this point in the history 
45712: sql: prevent arbitrary writes to system.comments r=RichardJCai a=knz

Fixes #45707.

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.

45729: roachtest: fix copy/bank roachtest on 19.1 r=nvanbenschoten a=ajwerner

In 19.2 we changed the name of the column from
SHOW ZONE CONFIGURATION from `config_sql` to `raw_config_sql`.
I relied on this column name in #45451.

This change updates the test to fall back to the old defaults if the column
does not exist.

Fixes #45489.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
  • Loading branch information
@knz @ajwerner
3 people committed Mar 5, 2020
3 parents 0000921 + b7f8a96 + 35b2e99 commit 4610ad2
Show file tree
Hide file tree
Showing 16 changed files with 171 additions and 45 deletions.
4 changes: 4 additions & 0 deletions pkg/cmd/roachtest/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,5 +164,9 @@ func getDefaultRangeSize(
AS range_max_bytes
FROM
[SHOW ZONE CONFIGURATION FOR RANGE default];`).Scan(&rangeMinBytes, &rangeMaxBytes)
// Older cluster versions do not contain this column. Use the old default.
if err != nil && strings.Contains(err.Error(), `column "raw_config_sql" does not exist`) {
rangeMinBytes, rangeMaxBytes, err = 32<<20 /* 32MB */, 64<<20 /* 64MB */, nil
}
return rangeMinBytes, rangeMaxBytes, err
}
4 changes: 3 additions & 1 deletion pkg/sql/alter_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1428,10 +1429,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,
Expand Down
8 changes: 6 additions & 2 deletions pkg/sql/comment_on_column.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
7 changes: 5 additions & 2 deletions pkg/sql/comment_on_database.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions pkg/sql/comment_on_index.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
8 changes: 6 additions & 2 deletions pkg/sql/comment_on_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion pkg/sql/drop_database.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions pkg/sql/drop_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
25 changes: 11 additions & 14 deletions pkg/sql/logictest/testdata/logic_test/grant_table
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ SET DATABASE = ''
query TTTTT colnames,rowsort
SELECT * FROM [SHOW GRANTS]
WHERE schema_name NOT IN ('crdb_internal', 'pg_catalog', 'information_schema')
ORDER BY 1,2,3
----
database_name schema_name table_name grantee privilege_type
system public namespace_deprecated admin GRANT
Expand Down Expand Up @@ -296,11 +297,7 @@ system public comments admin GRANT
system public comments admin INSERT
system public comments admin SELECT
system public comments admin UPDATE
system public comments public DELETE
system public comments public GRANT
system public comments public INSERT
system public comments public SELECT
system public comments public UPDATE
system public comments root DELETE
system public comments root GRANT
system public comments root INSERT
Expand Down Expand Up @@ -358,6 +355,16 @@ system public protected_ts_records admin GRANT
system public protected_ts_records admin SELECT
system public protected_ts_records root GRANT
system public protected_ts_records root SELECT
system public role_options admin DELETE
system public role_options admin GRANT
system public role_options admin INSERT
system public role_options admin SELECT
system public role_options admin UPDATE
system public role_options root DELETE
system public role_options root GRANT
system public role_options root INSERT
system public role_options root SELECT
system public role_options root UPDATE
system public statement_bundle_chunks admin DELETE
system public statement_bundle_chunks admin GRANT
system public statement_bundle_chunks admin INSERT
Expand Down Expand Up @@ -388,16 +395,6 @@ system public statement_diagnostics root GRANT
system public statement_diagnostics root INSERT
system public statement_diagnostics root SELECT
system public statement_diagnostics root UPDATE
system public role_options admin DELETE
system public role_options admin GRANT
system public role_options admin INSERT
system public role_options admin SELECT
system public role_options admin UPDATE
system public role_options root DELETE
system public role_options root GRANT
system public role_options root INSERT
system public role_options root SELECT
system public role_options root UPDATE
a public NULL admin ALL
a public NULL readwrite ALL
a public NULL root ALL
Expand Down
14 changes: 4 additions & 10 deletions pkg/sql/logictest/testdata/logic_test/information_schema
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ system public web_sessions BASE TABLE
system public table_statistics BASE TABLE YES 1
system public locations BASE TABLE YES 1
system public role_members BASE TABLE YES 1
system public comments BASE TABLE YES 1
system public comments BASE TABLE YES 5
system public replication_constraint_stats BASE TABLE YES 1
system public replication_critical_localities BASE TABLE YES 1
system public replication_stats BASE TABLE YES 1
Expand All @@ -636,7 +636,9 @@ statement ok
ALTER TABLE other_db.xyz ADD COLUMN j INT

query TTI colnames
SELECT TABLE_CATALOG, TABLE_NAME, VERSION FROM "".information_schema.tables WHERE version > 1 AND TABLE_SCHEMA = 'public' ORDER BY 1,2
SELECT table_catalog, table_name, version
FROM "".information_schema.tables
WHERE table_catalog != 'system' AND version > 1 AND table_schema = 'public' ORDER BY 1,2
----
table_catalog table_name version
other_db xyz 6
Expand Down Expand Up @@ -1603,11 +1605,7 @@ NULL admin system public comments
NULL admin system public comments INSERT NULL NO
NULL admin system public comments SELECT NULL YES
NULL admin system public comments UPDATE NULL NO
NULL public system public comments DELETE NULL NO
NULL public system public comments GRANT NULL NO
NULL public system public comments INSERT NULL NO
NULL public system public comments SELECT NULL YES
NULL public system public comments UPDATE NULL NO
NULL root system public comments DELETE NULL NO
NULL root system public comments GRANT NULL NO
NULL root system public comments INSERT NULL NO
Expand Down Expand Up @@ -2076,11 +2074,7 @@ NULL admin system public comments
NULL admin system public comments INSERT NULL NO
NULL admin system public comments SELECT NULL YES
NULL admin system public comments UPDATE NULL NO
NULL public system public comments DELETE NULL NO
NULL public system public comments GRANT NULL NO
NULL public system public comments INSERT NULL NO
NULL public system public comments SELECT NULL YES
NULL public system public comments UPDATE NULL NO
NULL root system public comments DELETE NULL NO
NULL root system public comments GRANT NULL NO
NULL root system public comments INSERT NULL NO
Expand Down
Loading

0 comments on commit 4610ad2

Please sign in to comment.