From 83f14bf7ff45590b9b62b27b6c35a0b7b02ac7fe Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 4 May 2021 13:21:05 -0600 Subject: [PATCH 1/3] *: Add security enhanced mode part 3 --- docs/design/2021-03-09-dynamic-privileges.md | 4 +- .../2021-03-09-security-enhanced-mode.md | 4 +- executor/show.go | 15 +++++- planner/core/expression_rewriter.go | 8 +++ planner/core/planbuilder.go | 4 ++ privilege/privileges/privileges.go | 5 +- privilege/privileges/privileges_test.go | 49 +++++++++++++++++++ sessionctx/variable/sysvar.go | 2 +- sessionctx/variable/tidb_vars.go | 2 - tidb-server/main.go | 3 ++ util/sem/sem.go | 32 ++++++++++++ util/sem/sem_test.go | 25 ++++++++++ 12 files changed, 142 insertions(+), 11 deletions(-) diff --git a/docs/design/2021-03-09-dynamic-privileges.md b/docs/design/2021-03-09-dynamic-privileges.md index 7ad0d59d2c54e..c85c0dc0c5305 100644 --- a/docs/design/2021-03-09-dynamic-privileges.md +++ b/docs/design/2021-03-09-dynamic-privileges.md @@ -1,7 +1,7 @@ # Proposal: - Author(s): [morgo](https://github.com/morgo) -- Last updated: April 25, 2021 +- Last updated: May 04, 2021 - Discussion at: N/A ## Table of Contents @@ -238,7 +238,7 @@ No change | Privilege Name | Description | Notes | | --------------- | --------------- | --------------- | -| `RESTRICTED_SYSTEM_VARIABLES_ADMIN` | Allows changing a restricted `GLOBAL` system variable. | Currently in SEM all high risk variables are unloaded. TBD, it might be required in future that they are only visible/settable to those with this privilege and not SUPER. | +| `RESTRICTED_VARIABLES_ADMIN` | Allows changing a restricted `GLOBAL` system variable. | Currently in SEM all high risk variables are unloaded. TBD, it might be required in future that they are only visible/settable to those with this privilege and not SUPER. | | `RESTRICTED_STATUS_ADMIN` | Allows observing restricted status variables. | i.e. `SHOW GLOBAL STATUS` by default hides some statistics when `SEM` is enabled. | | `RESTRICTED_CONNECTION_ADMIN` | A special privilege to say that their connections, etc. can’t be killed by SUPER users AND they can kill connections by all other users. Affects `KILL`, `KILL TIDB` commands. | It is intended for the CloudAdmin user in DBaaS. | | `RESTRICTED_USER_ADMIN` | A special privilege to say that their access can’t be changed by `SUPER` users. Statements `DROP USER`, `SET PASSWORD`, `ALTER USER`, `REVOKE` are all limited. | It is intended for the CloudAdmin user in DbaaS. | diff --git a/docs/design/2021-03-09-security-enhanced-mode.md b/docs/design/2021-03-09-security-enhanced-mode.md index e939fec67c154..efc5b79f499e4 100644 --- a/docs/design/2021-03-09-security-enhanced-mode.md +++ b/docs/design/2021-03-09-security-enhanced-mode.md @@ -1,7 +1,7 @@ # Proposal: - Author(s): [morgo](https://github.com/morgo) -- Last updated: April 25, 2021 +- Last updated: May 04, 2021 - Discussion at: N/A ## Table of Contents @@ -49,7 +49,7 @@ A boolean option called `EnableEnhancedSecurity` (default `FALSE`) will be added ### System Variables -The following system variables will be hidden unless the user has the `RESTRICTED_SYSTEM_VARIABLES_ADMIN` privilege: +The following system variables will be hidden unless the user has the `RESTRICTED_VARIABLES_ADMIN` privilege: * variable.TiDBDDLSlowOprThreshold, * variable.TiDBAllowRemoveAutoInc, diff --git a/executor/show.go b/executor/show.go index 725c0e0e28ec2..dd614ee9c993d 100644 --- a/executor/show.go +++ b/executor/show.go @@ -661,6 +661,17 @@ func (e *ShowExec) fetchShowMasterStatus() error { return nil } +func (e *ShowExec) sysVarHiddenForSem(sysVarNameInLower string) bool { + if !sem.IsEnabled() || !sem.IsInvisibleSysVar(sysVarNameInLower) { + return false + } + checker := privilege.GetPrivilegeManager(e.ctx) + if checker == nil || checker.RequestDynamicVerification(e.ctx.GetSessionVars().ActiveRoles, "RESTRICTED_VARIABLES_ADMIN", false) { + return false + } + return true +} + func (e *ShowExec) fetchShowVariables() (err error) { var ( value string @@ -673,7 +684,7 @@ func (e *ShowExec) fetchShowVariables() (err error) { // otherwise, fetch the value from table `mysql.Global_Variables`. for _, v := range variable.GetSysVars() { if v.Scope != variable.ScopeSession { - if v.Hidden { + if v.Hidden || e.sysVarHiddenForSem(v.Name) { continue } value, err = variable.GetGlobalSystemVar(sessionVars, v.Name) @@ -690,7 +701,7 @@ func (e *ShowExec) fetchShowVariables() (err error) { // If it is a session only variable, use the default value defined in code, // otherwise, fetch the value from table `mysql.Global_Variables`. for _, v := range variable.GetSysVars() { - if v.Hidden { + if v.Hidden || e.sysVarHiddenForSem(v.Name) { continue } value, err = variable.GetSessionSystemVar(sessionVars, v.Name) diff --git a/planner/core/expression_rewriter.go b/planner/core/expression_rewriter.go index 5d6a11ad982a9..0a174b52ea560 100644 --- a/planner/core/expression_rewriter.go +++ b/planner/core/expression_rewriter.go @@ -31,6 +31,7 @@ import ( "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/expression/aggregation" "github.com/pingcap/tidb/infoschema" + "github.com/pingcap/tidb/privilege" "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/table" @@ -42,6 +43,7 @@ import ( "github.com/pingcap/tidb/util/codec" "github.com/pingcap/tidb/util/collate" "github.com/pingcap/tidb/util/hint" + "github.com/pingcap/tidb/util/sem" "github.com/pingcap/tidb/util/stringutil" ) @@ -1228,6 +1230,12 @@ func (er *expressionRewriter) rewriteVariable(v *ast.VariableExpr) { er.err = variable.ErrUnknownSystemVar.GenWithStackByArgs(name) return } + if sem.IsEnabled() && sem.IsInvisibleSysVar(sysVar.Name) { + checker := privilege.GetPrivilegeManager(er.b.ctx) + if checker == nil || !checker.RequestDynamicVerification(sessionVars.ActiveRoles, "RESTRICTED_VARIABLES_ADMIN", false) { + er.err = ErrSpecificAccessDenied.GenWithStackByArgs("RESTRICTED_VARIABLES_ADMIN") + } + } // Variable is @@gobal.variable_name or variable is only global scope variable. if v.IsGlobal || sysVar.Scope == variable.ScopeGlobal { val, err = variable.GetGlobalSystemVar(sessionVars, name) diff --git a/planner/core/planbuilder.go b/planner/core/planbuilder.go index 6fc98bc522508..0423923fc2be1 100644 --- a/planner/core/planbuilder.go +++ b/planner/core/planbuilder.go @@ -720,6 +720,10 @@ func (b *PlanBuilder) buildSet(ctx context.Context, v *ast.SetStmt) (Plan, error err := ErrSpecificAccessDenied.GenWithStackByArgs("SUPER or SYSTEM_VARIABLES_ADMIN") b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "SYSTEM_VARIABLES_ADMIN", false, err) } + if sem.IsEnabled() && sem.IsInvisibleSysVar(strings.ToLower(vars.Name)) { + err := ErrSpecificAccessDenied.GenWithStackByArgs("RESTRICTED_VARIABLES_ADMIN") + b.visitInfo = appendDynamicVisitInfo(b.visitInfo, "RESTRICTED_VARIABLES_ADMIN", false, err) + } assign := &expression.VarAssignment{ Name: vars.Name, IsGlobal: vars.IsGlobal, diff --git a/privilege/privileges/privileges.go b/privilege/privileges/privileges.go index 5b7917e802aac..5446396bd9b48 100644 --- a/privilege/privileges/privileges.go +++ b/privilege/privileges/privileges.go @@ -43,8 +43,9 @@ var dynamicPrivs = []string{ "SYSTEM_VARIABLES_ADMIN", "ROLE_ADMIN", "CONNECTION_ADMIN", - "RESTRICTED_TABLES_ADMIN", // Can see system tables when SEM is enabled - "RESTRICTED_STATUS_ADMIN", // Can see all status vars when SEM is enabled. + "RESTRICTED_TABLES_ADMIN", // Can see system tables when SEM is enabled + "RESTRICTED_STATUS_ADMIN", // Can see all status vars when SEM is enabled. + "RESTRICTED_VARIABLES_ADMIN", // Can see all variables when SEM is enabled } var dynamicPrivLock sync.Mutex diff --git a/privilege/privileges/privileges_test.go b/privilege/privileges/privileges_test.go index 816fe5a59d0bd..6604d816cc6f0 100644 --- a/privilege/privileges/privileges_test.go +++ b/privilege/privileges/privileges_test.go @@ -1393,3 +1393,52 @@ func (s *testPrivilegeSuite) TestSecurityEnhancedModeStatusVars(c *C) { AuthHostname: "%", }, nil, nil) } + +func (s *testPrivilegeSuite) TestSecurityEnhancedModeSysVars(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("CREATE USER svroot1, svroot2") + tk.MustExec("GRANT SUPER ON *.* to svroot1 WITH GRANT OPTION") + tk.MustExec("SET tidb_enable_dynamic_privileges=1") + tk.MustExec("GRANT SUPER, RESTRICTED_VARIABLES_ADMIN ON *.* to svroot2") + + sem.Enable() + defer sem.Disable() + + // svroot1 has SUPER but in SEM will be restricted + tk.Se.Auth(&auth.UserIdentity{ + Username: "svroot1", + Hostname: "localhost", + AuthUsername: "uroot", + AuthHostname: "%", + }, nil, nil) + + tk.MustQuery(`SHOW VARIABLES LIKE 'tidb_force_priority'`).Check(testkit.Rows()) + tk.MustQuery(`SHOW GLOBAL VARIABLES LIKE 'tidb_enable_telemetry'`).Check(testkit.Rows()) + + _, err := tk.Exec("SET tidb_force_priority = 'NO_PRIORITY'") + c.Assert(err.Error(), Equals, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation") + _, err = tk.Exec("SET GLOBAL tidb_enable_telemetry = OFF") + c.Assert(err.Error(), Equals, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation") + + _, err = tk.Exec("SELECT @@session.tidb_force_priority") + c.Assert(err.Error(), Equals, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation") + _, err = tk.Exec("SELECT @@global.tidb_enable_telemetry") + c.Assert(err.Error(), Equals, "[planner:1227]Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation") + + tk.Se.Auth(&auth.UserIdentity{ + Username: "svroot2", + Hostname: "localhost", + AuthUsername: "uroot", + AuthHostname: "%", + }, nil, nil) + + tk.MustQuery(`SHOW VARIABLES LIKE 'tidb_force_priority'`).Check(testkit.Rows("tidb_force_priority NO_PRIORITY")) + tk.MustQuery(`SHOW GLOBAL VARIABLES LIKE 'tidb_enable_telemetry'`).Check(testkit.Rows("tidb_enable_telemetry ON")) + + // should not actually make any change. + tk.MustExec("SET tidb_force_priority = 'NO_PRIORITY'") + tk.MustExec("SET GLOBAL tidb_enable_telemetry = ON") + + tk.MustQuery(`SELECT @@session.tidb_force_priority`).Check(testkit.Rows("NO_PRIORITY")) + tk.MustQuery(`SELECT @@global.tidb_enable_telemetry`).Check(testkit.Rows("1")) +} diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 751f0e70f0469..7bae11e6ef113 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -507,7 +507,7 @@ var defaultSysVars = []*SysVar{ } return normalizedValue, ErrWrongValueForVar.GenWithStackByArgs(ForeignKeyChecks, originalValue) }}, - {Scope: ScopeNone, Name: Hostname, Value: ServerHostname}, + {Scope: ScopeNone, Name: Hostname, Value: DefHostname}, {Scope: ScopeSession, Name: Timestamp, Value: ""}, {Scope: ScopeGlobal | ScopeSession, Name: CharacterSetFilesystem, Value: "binary", Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) { return checkCharacterSet(normalizedValue, CharacterSetFilesystem) diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index 30d52ac54f386..5e65bcb9199a9 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/sessionctx/variable/tidb_vars.go @@ -15,7 +15,6 @@ package variable import ( "math" - "os" "github.com/pingcap/parser/mysql" "github.com/pingcap/tidb/config" @@ -693,7 +692,6 @@ var ( // DDLSlowOprThreshold is the threshold for ddl slow operations, uint is millisecond. DDLSlowOprThreshold uint32 = DefTiDBDDLSlowOprThreshold ForcePriority = int32(DefTiDBForcePriority) - ServerHostname, _ = os.Hostname() MaxOfMaxAllowedPacket uint64 = 1073741824 ExpensiveQueryTimeThreshold uint64 = DefTiDBExpensiveQueryTimeThreshold MinExpensiveQueryTimeThreshold uint64 = 10 // 10s diff --git a/tidb-server/main.go b/tidb-server/main.go index f070d2eeec48d..2e6ce34ca1301 100644 --- a/tidb-server/main.go +++ b/tidb-server/main.go @@ -537,6 +537,9 @@ func setGlobalVars() { variable.SetSysVar(variable.TiDBSlowQueryFile, cfg.Log.SlowQueryFile) variable.SetSysVar(variable.TiDBIsolationReadEngines, strings.Join(cfg.IsolationRead.Engines, ", ")) variable.MemoryUsageAlarmRatio.Store(cfg.Performance.MemoryUsageAlarmRatio) + if hostname, err := os.Hostname(); err != nil { + variable.SetSysVar(variable.Hostname, hostname) + } if cfg.Security.EnableSEM { sem.Enable() diff --git a/util/sem/sem.go b/util/sem/sem.go index 8c3d2b456d991..d29d29b601559 100644 --- a/util/sem/sem.go +++ b/util/sem/sem.go @@ -14,6 +14,7 @@ package sem import ( + "os" "strings" "sync/atomic" @@ -70,6 +71,7 @@ var ( func Enable() { atomic.StoreInt32(&semEnabled, 1) variable.SetSysVar(variable.TiDBEnableEnhancedSecurity, variable.On) + variable.SetSysVar(variable.Hostname, variable.DefHostname) // write to log so users understand why some operations are weird. logutil.BgLogger().Info("tidb-server is operating with security enhanced mode (SEM) enabled") } @@ -79,6 +81,9 @@ func Enable() { func Disable() { atomic.StoreInt32(&semEnabled, 0) variable.SetSysVar(variable.TiDBEnableEnhancedSecurity, variable.Off) + if hostname, err := os.Hostname(); err != nil { + variable.SetSysVar(variable.Hostname, hostname) + } } // IsEnabled checks if Security Enhanced Mode (SEM) is enabled @@ -125,6 +130,33 @@ func IsInvisibleStatusVar(varName string) bool { return varName == tidbGCLeaderDesc } +// IsInvisibleSysVar returns true if the sysvar needs to be hidden +func IsInvisibleSysVar(varNameInLower string) bool { + switch varNameInLower { + case variable.TiDBDDLSlowOprThreshold, // ddl_slow_threshold + variable.TiDBAllowRemoveAutoInc, + variable.TiDBCheckMb4ValueInUTF8, + variable.TiDBConfig, + variable.TiDBEnableSlowLog, + variable.TiDBExpensiveQueryTimeThreshold, + variable.TiDBForcePriority, + variable.TiDBGeneralLog, + variable.TiDBMetricSchemaRangeDuration, + variable.TiDBMetricSchemaStep, + variable.TiDBOptWriteRowID, + variable.TiDBPProfSQLCPU, + variable.TiDBRecordPlanInSlowLog, + variable.TiDBSlowQueryFile, + variable.TiDBSlowLogThreshold, + variable.TiDBEnableCollectExecutionInfo, + variable.TiDBMemoryUsageAlarmRatio, + variable.TiDBEnableTelemetry, + variable.TiDBRowFormatVersion: + return true + } + return false +} + // IsRestrictedPrivilege returns true if the privilege shuld not be satisfied by SUPER // As most dynamic privileges are. func IsRestrictedPrivilege(privNameInUpper string) bool { diff --git a/util/sem/sem_test.go b/util/sem/sem_test.go index c303d2195c7f4..073a195139c37 100644 --- a/util/sem/sem_test.go +++ b/util/sem/sem_test.go @@ -17,6 +17,7 @@ import ( "testing" "github.com/pingcap/parser/mysql" + "github.com/pingcap/tidb/sessionctx/variable" . "github.com/pingcap/check" ) @@ -74,3 +75,27 @@ func (s *testSecurity) TestIsInvisibleStatusVar(c *C) { c.Assert(IsInvisibleStatusVar("ddl_schema_version"), IsFalse) c.Assert(IsInvisibleStatusVar("Ssl_version"), IsFalse) } + +func (s *testSecurity) TestIsInvisibleSysVar(c *C) { + c.Assert(IsInvisibleSysVar(variable.Hostname), IsFalse) // changes the value to default, but is not invisible + c.Assert(IsInvisibleSysVar(variable.TiDBEnableEnhancedSecurity), IsFalse) // should be able to see the mode is on. + + c.Assert(IsInvisibleSysVar(variable.TiDBAllowRemoveAutoInc), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBCheckMb4ValueInUTF8), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBConfig), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBEnableSlowLog), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBExpensiveQueryTimeThreshold), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBForcePriority), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBGeneralLog), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBMetricSchemaRangeDuration), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBMetricSchemaStep), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBOptWriteRowID), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBPProfSQLCPU), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBRecordPlanInSlowLog), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBSlowQueryFile), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBSlowLogThreshold), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBEnableCollectExecutionInfo), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBMemoryUsageAlarmRatio), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBEnableTelemetry), IsTrue) + c.Assert(IsInvisibleSysVar(variable.TiDBRowFormatVersion), IsTrue) +} From 7e95d2417538ee933c5f8b94ff576c307dd6684c Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 4 May 2021 14:24:45 -0600 Subject: [PATCH 2/3] Change to usei visitInfo --- planner/core/expression_rewriter.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/planner/core/expression_rewriter.go b/planner/core/expression_rewriter.go index 0a174b52ea560..cbc8a2db4d222 100644 --- a/planner/core/expression_rewriter.go +++ b/planner/core/expression_rewriter.go @@ -31,7 +31,6 @@ import ( "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/expression/aggregation" "github.com/pingcap/tidb/infoschema" - "github.com/pingcap/tidb/privilege" "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/table" @@ -1231,10 +1230,8 @@ func (er *expressionRewriter) rewriteVariable(v *ast.VariableExpr) { return } if sem.IsEnabled() && sem.IsInvisibleSysVar(sysVar.Name) { - checker := privilege.GetPrivilegeManager(er.b.ctx) - if checker == nil || !checker.RequestDynamicVerification(sessionVars.ActiveRoles, "RESTRICTED_VARIABLES_ADMIN", false) { - er.err = ErrSpecificAccessDenied.GenWithStackByArgs("RESTRICTED_VARIABLES_ADMIN") - } + err := ErrSpecificAccessDenied.GenWithStackByArgs("RESTRICTED_VARIABLES_ADMIN") + er.b.visitInfo = appendDynamicVisitInfo(er.b.visitInfo, "RESTRICTED_VARIABLES_ADMIN", false, err) } // Variable is @@gobal.variable_name or variable is only global scope variable. if v.IsGlobal || sysVar.Scope == variable.ScopeGlobal { From 580274bf8c5af2de8a24feb0e48d6298ae011d96 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Sun, 16 May 2021 20:39:47 -0600 Subject: [PATCH 3/3] fix merge --- executor/executor_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/executor/executor_test.go b/executor/executor_test.go index d67bb6b48b8f4..d5d0537b548a0 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -357,6 +357,7 @@ func (s *testSuiteP1) TestShow(c *C) { "CONNECTION_ADMIN Server Admin ", "RESTRICTED_TABLES_ADMIN Server Admin ", "RESTRICTED_STATUS_ADMIN Server Admin ", + "RESTRICTED_VARIABLES_ADMIN Server Admin ", )) c.Assert(len(tk.MustQuery("show table status").Rows()), Equals, 1) }