server, sessionctx: add multi statement workaround #22351

morgo · 2021-01-11T17:00:20Z

What problem does this PR solve?

Problem Summary:

v4.0.9 shipped with a fix for a server protocol vulnerability: #19459

It can be worked around by changing client library settings, but that's not always easy given each client library is different. This provides a server workaround as well, which adjusts from an error to a warning by default.

What is changed and how it works?

A new sysvar is added, called tidb_multi_statement_mode (scope: SESSION or GLOBAL). The type is an ENUM:

OFF: the MySQL compatible/safest behavior. Multi-statement is not permitted unless the client sets the multi-statement attribute. An error is returned.
ON: Multi-statement is permitted without errors or warnings.
WARN (default): Multi-statement is permitted, but returns a warning.

Thus, the "4.0.8 and earlier" behavior can be restored with "ON". The 4.0.9 and 4.0.10 behavior is effectively "OFF".

Both the warning and error message is as follows:

client has multi-statement capability disabled. Run SET GLOBAL tidb_multi_statement_mode='ON' after you understand the security risk

The intention is to change the default from WARN back to OFF in a 4.0-series release in the short future, so users are safe-by-default. In order to do this, SQL client error tracking will have to be added (see #14433 ). This PR ensures that this error uses the unique code of 8030 so that deployment tools can check if a user depends on the unsafe behavior before attempting to upgrade them.

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test

Side effects

Introduces security risk again (but not by default, and not materially different from enabling client multi-statement)

Release note

TiDB 4.0.9 fixed a security issue, where TiDB incorrectly permitted multiple statements to be executed in one COM_QUERY packet, leading to increased risk of SQL injection. To provide backwards compatibility for applications that depend on this behavior, a new option tidb_multi_statement_mode has been added. Assuming you understand the security risks, you can revert to the 4.0.8 by executing SET GLOBAL tidb_multi_statement_mode='ON'. The default behavior of tidb_multi_statement_mode also relaxes the error introduced in 4.0.9 to a warning. It is intended to be changed to an error again in a future release.

haohaoxuex2005 · 2021-01-12T07:34:45Z

SQL injection is a high risk, do we have a better way to ensure that user upgrade it?

morgo · 2021-01-12T14:57:20Z

SQL injection is a high risk, do we have a better way to ensure that user upgrade it?

To clarify: SQL injection is still possible either way. But the SQL injection cases are much riskier with multi-statement, since you can terminate the existing partial statement and then execute an entirely new statement.

The protection is still enabled by default. Clients need to send the multi-statement flag, or (from this PR) SET tidb_allow_multi_statement=1. This provides an easier upgrade path for those that can not modify their client drivers. Edit: it is now a warning by default.

haohaoxuex2005 · 2021-01-12T15:09:44Z

SQL injection is a high risk, do we have a better way to ensure that user upgrade it?

To clarify: SQL injection is still possible either way. But the SQL injection cases are much riskier with multi-statement, since you can terminate the existing partial statement and then execute an entirely new statement.

The protection is still enabled by default. Clients need to send the multi-statement flag, or (from this PR) SET tidb_allow_multi_statement=1. This provides an easier upgrade path for those that can not modify their client drivers.

Yeah, I understand, thank you!

morgo · 2021-01-13T04:43:07Z

/run-all-tests

…rkaround

sessionctx/variable/sysvar.go

xhebox

Rest LGTM

xhebox · 2021-01-21T05:07:31Z

sessionctx/variable/sysvar.go

+	// Warn means return warnings
+	Warn = "WARN"
+	// OffInt is used by TiDBMultiStatementMode
+	OffInt = 0


I would suggest you mark a type to OffInt and the field TiDBMultiStatementMode itself, like type TiDBMultiStatementModeEnum int or something else. Then move it closer to the variable field or the handle function.

OK, I'll move it.

xhebox · 2021-01-21T05:23:26Z

server/conn.go

@@ -1433,7 +1433,19 @@ func (cc *clientConn) handleQuery(ctx context.Context, sql string) (err error) {

 		capabilities := cc.ctx.GetSessionVars().ClientCapability
 		if capabilities&mysql.ClientMultiStatements < 1 {


Is it intended that, if user set multi-statements on, even MultiStatementMode is Warn, the server will not report any error? Or if user set multi-statements on, even MultiStatementMode is Off, the server will continue without errors?

That is, is it intended to put your MultiStatementMode code in the branch of multi-statements disabled
by users/clients?

If it is, is there any way to test cases where multi-statements are enabled by clients?

Is it intended that, if user set multi-statements on, even MultiStatementMode is Warn, the server will not report any error? Or if user set multi-statements on, even MultiStatementMode is Off, the server will continue without errors?

Correct: If multiStatements are ON or WARN, there is no error. The error is caused by OFF, because multiStatement support is "turned off". This is assuming that the client has not set the multi-statement flag correctly, in which case it is permitted and none of this applies.

That is, is it intended to put your MultiStatementMode code in the branch of multi-statements disabled
by users/clients?

Correct. If they enable multi-statements in their client, I have no issue with it. This flag is only for preserving the upgrade story because TiDB previously ignored the client multi-statement flag.

If it is, is there any way to test cases where multi-statements are enabled by clients?

Yes, there are preexisting tests. See runTestMultiStatements in server/server_test.go which started setting this flag correctly from the PR https://github.com/pingcap/tidb/pull/19459/files

Thanks for you detailed explanation :)

it that a conclusion above?
client multi-statements = on will pass through the check, otherwise, TiDB will take internal MultiStatementMode into consideration?

Correct. Ideally clients would always set this, and we don't have the internal MultiStatementMode check. But per the description, this is a workaround because TiDB only started enforcing the multi-statement check in 4.0.9 and it's been causing users problems.

xhebox

LGTM

xhebox · 2021-01-21T06:08:12Z

You may want to request another team member to review.

sessionctx/variable/sysvar.go

tiancaiamao · 2021-01-21T06:52:10Z

LGTM

tiancaiamao · 2021-01-21T06:52:31Z

/merge

ti-srebot · 2021-01-21T06:54:57Z

/run-all-tests

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-01-21T07:19:27Z

cherry pick to release-4.0 in PR #22468

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-01-21T07:22:27Z

cherry pick to release-5.0-rc in PR #22469

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

server, sessionctx: add multi statement workaround

c4122e4

morgo added needs-cherry-pick-4.0 labels Jan 11, 2021

morgo requested review from bb7133 and zz-jason January 11, 2021 17:00

github-actions bot added the sig/sql-infra SIG: SQL Infra label Jan 11, 2021

update based on reviewer discussion

da8aa0b

morgo added 2 commits January 13, 2021 12:54

Merge remote-tracking branch 'upstream/master' into add-multi-stmt-wo…

455fb4f

…rkaround

Fix error code, use error message from standard location

ca019cd

morgo mentioned this pull request Jan 13, 2021

*: add infoschema client errors #22382

Merged

Rename variable from reviewer feedback

9d2d1bc

bb7133 reviewed Jan 15, 2021

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

Address PR feedback

cc366d9

morgo force-pushed the add-multi-stmt-workaround branch from dd8ee3f to cc366d9 Compare January 21, 2021 02:31

morgo requested a review from xhebox January 21, 2021 03:31

xhebox reviewed Jan 21, 2021

View reviewed changes

Address PR feedback

6104ed9

xhebox reviewed Jan 21, 2021

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 21, 2021

tiancaiamao self-requested a review January 21, 2021 06:23

tiancaiamao reviewed Jan 21, 2021

View reviewed changes

sessionctx/variable/sysvar.go Show resolved Hide resolved

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 21, 2021

ti-srebot approved these changes Jan 21, 2021

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 21, 2021

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 21, 2021

Merge branch 'master' into add-multi-stmt-workaround

d7d911b

ti-srebot merged commit 57eef13 into pingcap:master Jan 21, 2021

morgo deleted the add-multi-stmt-workaround branch January 21, 2021 07:15

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 21, 2021

cherry pick pingcap#22351 to release-4.0

f671d23

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jan 21, 2021

server, sessionctx: add multi statement workaround (#22351) #22468

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 21, 2021

cherry pick pingcap#22351 to release-5.0-rc

39a58b1

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jan 21, 2021

server, sessionctx: add multi statement workaround (#22351) #22469

Closed

morgo mentioned this pull request Jan 21, 2021

server, sessionctx: change default for tidb_multi_statement_mode #22484

Merged

ti-srebot added a commit that referenced this pull request Jan 27, 2021

server, sessionctx: add multi statement workaround (#22351) (#22468)

8d893eb

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

morgo mentioned this pull request Jan 27, 2021

faq, sysvars: add tidb_multi_statement_mode pingcap/docs#4741

Merged

10 tasks

This was referenced Mar 1, 2021

faq, sysvars: add tidb_multi_statement_mode (#4741) pingcap/docs#4937

Merged

faq, sysvars: add tidb_multi_statement_mode (#4741) pingcap/docs#4938

Merged

TomShawn mentioned this pull request Mar 11, 2021

faq, sysvars: add tidb_multi_statement_mode pingcap/docs-cn#5716

Merged

10 tasks

morgo mentioned this pull request Apr 22, 2021

variable: merge Bool{On,Off} with {On,Off} #24218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server, sessionctx: add multi statement workaround #22351

server, sessionctx: add multi statement workaround #22351

morgo commented Jan 11, 2021 •

edited

Loading

haohaoxuex2005 commented Jan 12, 2021

morgo commented Jan 12, 2021 •

edited

Loading

haohaoxuex2005 commented Jan 12, 2021

morgo commented Jan 13, 2021

xhebox left a comment

xhebox Jan 21, 2021

morgo Jan 21, 2021

xhebox Jan 21, 2021 •

edited

Loading

morgo Jan 21, 2021

xhebox Jan 21, 2021

AilinKid Jan 21, 2021

morgo Jan 21, 2021

xhebox left a comment

xhebox commented Jan 21, 2021

tiancaiamao commented Jan 21, 2021

tiancaiamao commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

		@@ -1433,7 +1433,19 @@ func (cc *clientConn) handleQuery(ctx context.Context, sql string) (err error) {

		capabilities := cc.ctx.GetSessionVars().ClientCapability
		if capabilities&mysql.ClientMultiStatements < 1 {

server, sessionctx: add multi statement workaround #22351

server, sessionctx: add multi statement workaround #22351

Conversation

morgo commented Jan 11, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

haohaoxuex2005 commented Jan 12, 2021

morgo commented Jan 12, 2021 • edited Loading

haohaoxuex2005 commented Jan 12, 2021

morgo commented Jan 13, 2021

xhebox left a comment

Choose a reason for hiding this comment

xhebox Jan 21, 2021

Choose a reason for hiding this comment

morgo Jan 21, 2021

Choose a reason for hiding this comment

xhebox Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

morgo Jan 21, 2021

Choose a reason for hiding this comment

xhebox Jan 21, 2021

Choose a reason for hiding this comment

AilinKid Jan 21, 2021

Choose a reason for hiding this comment

morgo Jan 21, 2021

Choose a reason for hiding this comment

xhebox left a comment

Choose a reason for hiding this comment

xhebox commented Jan 21, 2021

tiancaiamao commented Jan 21, 2021

tiancaiamao commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

ti-srebot commented Jan 21, 2021

morgo commented Jan 11, 2021 •

edited

Loading

morgo commented Jan 12, 2021 •

edited

Loading

xhebox Jan 21, 2021 •

edited

Loading