sql: prevent mutation of synthetic privilege cache entries

Previously, when GRANT / REVOKE were modifying synthetic privileges, they
requested a copy of the privilege descriptor from the synthetic privilege
cache, which would make a shallow copy. This meant that returned entries
were not safe for mutations, since slices would be shared between
copies. This could lead to unexpected behavior for concurrent
transactions or schema change retries, since slices could get modified
in unexpected ways. To address this, this patch marks the existing
method as immutable and makes a variant aimed at modification that is
labeled as mutable.

Fixes: cockroachdb#156503
Fixes: cockroachdb#155822
Fixes: cockroachdb#155858
Fixes: cockroachdb#156393

Release note (bug fix): Transactions running concurrently with a GRANT /
REVOKE on virtual tables / external connections could observe
modifications incorrectly.

Author: fqazi

pkg/sql/authorization.go

@@ -156,7 +156,7 @@ func (p *planner) HasPrivilege(
 	// permission check).
 	p.maybeAuditSensitiveTableAccessEvent(privilegeObject, privilegeKind)
 
-	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privs, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return false, err
 	}
@@ -209,7 +209,7 @@ func (p *planner) HasAnyPrivilege(
 		return true, nil
 	}
 
-	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privs, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return false, err
 	}
@@ -373,7 +373,7 @@ func (p *planner) getOwnerOfPrivilegeObject(
 	if d, ok := privilegeObject.(catalog.TableDescriptor); ok && d.IsVirtualTable() {
 		return username.NodeUserName(), nil
 	}
-	privDesc, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return username.SQLUsername{}, err
 	}

pkg/sql/grant_revoke_system.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -48,7 +49,7 @@ func (n *changeNonDescriptorBackedPrivilegesNode) startExec(params runParams) er
 		if err := catprivilege.ValidateSyntheticPrivilegeObject(systemPrivilegeObject); err != nil {
 			return err
 		}
-		syntheticPrivDesc, err := params.p.getPrivilegeDescriptor(params.ctx, systemPrivilegeObject)
+		syntheticPrivDesc, err := params.p.getMutablePrivilegeDescriptor(params.ctx, systemPrivilegeObject)
 		if err != nil {
 			return err
 		}
@@ -255,10 +256,11 @@ func (n *changeNonDescriptorBackedPrivilegesNode) makeSystemPrivilegeObject(
 	}
 }
 
-// getPrivilegeDescriptor returns the privilege descriptor for the
+// getImmutablePrivilegeDescriptor returns the privilege descriptor for the
 // object. Note that for non-descriptor backed objects, we query the
-// system.privileges table to synthesize a PrivilegeDescriptor.
-func (p *planner) getPrivilegeDescriptor(
+// system.privileges table to synthesize a PrivilegeDescriptor. The returned
+// privilege descriptor is not safe for modification.
+func (p *planner) getImmutablePrivilegeDescriptor(
 	ctx context.Context, po privilege.Object,
 ) (*catpb.PrivilegeDescriptor, error) {
 	switch d := po.(type) {
@@ -294,3 +296,26 @@ func (p *planner) getPrivilegeDescriptor(
 	}
 	return nil, errors.AssertionFailedf("unknown privilege.Object type %T", po)
 }
+
+// getMutablePrivilegeDescriptor returns the privilege descriptor for the
+// object. Note that for non-descriptor backed objects, we query the
+// system.privileges table to synthesize a PrivilegeDescriptor. The returned
+// copy can be safely modified.
+func (p *planner) getMutablePrivilegeDescriptor(
+	ctx context.Context, po privilege.Object,
+) (*catpb.PrivilegeDescriptor, error) {
+	pd, err := p.getImmutablePrivilegeDescriptor(ctx, po)
+	if err != nil {
+		return nil, err
+	}
+	// No copy is needed for non-synthetic descriptors.
+	switch d := po.(type) {
+	case catalog.TableDescriptor:
+		if !d.IsVirtualTable() {
+			return pd, nil
+		}
+	case catalog.Descriptor:
+		return pd, nil
+	}
+	return protoutil.Clone(pd).(*catpb.PrivilegeDescriptor), nil
+}

pkg/sql/information_schema.go

@@ -408,7 +408,7 @@ https://www.postgresql.org/docs/9.5/infoschema-column-privileges.html`,
 			dbNameStr := tree.NewDString(db.GetName())
 			scNameStr := tree.NewDString(sc.GetName())
 			columndata := privilege.List{privilege.SELECT, privilege.INSERT, privilege.UPDATE} // privileges for column level granularity
-			privDesc, err := p.getPrivilegeDescriptor(ctx, table)
+			privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 			if err != nil {
 				return err
 			}
@@ -1610,7 +1610,7 @@ func populateTablePrivileges(
 			// TODO(knz): This should filter for the current user, see
 			// https://github.com/cockroachdb/cockroach/issues/35572
 			tableType := table.GetObjectType()
-			desc, err := p.getPrivilegeDescriptor(ctx, table)
+			desc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 			if err != nil {
 				return err
 			}
@@ -1623,7 +1623,7 @@ func populateTablePrivileges(
 				for _, priv := range u.Privileges {
 					// We use this function to check for the grant option so that the
 					// object owner also gets is_grantable=true.
-					privs, err := p.getPrivilegeDescriptor(ctx, table)
+					privs, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 					if err != nil {
 						return err
 					}

pkg/sql/pg_catalog.go

@@ -3246,7 +3246,7 @@ https://www.postgresql.org/docs/9.6/catalog-pg-shdepend.html`,
 		if err = forEachTableDesc(ctx, p, dbContext, opts,
 			func(ctx context.Context, descCtx tableDescContext) error {
 				db, table := descCtx.database, descCtx.table
-				privDesc, err := p.getPrivilegeDescriptor(ctx, table)
+				privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 				if err != nil {
 					return err
 				}

pkg/sql/resolver.go

@@ -136,7 +136,7 @@ func (p *planner) HasAnyPrivilegeForSpecifier(
 		}
 
 		if priv.GrantOption {
-			privDesc, err := p.getPrivilegeDescriptor(ctx, privObject)
+			privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, privObject)
 			if err != nil {
 				return eval.HasNoPrivilege, err
 			}