Skip to content

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

Merged
ti-srebot merged 3 commits intopingcap:release-4.0from
ti-srebot:release-4.0-57eef1333bf1
Jan 27, 2021
Merged

server, sessionctx: add multi statement workaround (#22351)#22468
ti-srebot merged 3 commits intopingcap:release-4.0from
ti-srebot:release-4.0-57eef1333bf1

Conversation

@ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Jan 21, 2021

cherry-pick #22351 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/22468

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/22468:release-4.0-57eef1333bf1

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.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@morgo please accept the invitation then you can push to the cherry-pick pull requests.
https://github.com/ti-srebot/tidb/invitations

@morgo
Copy link
Contributor

morgo commented Jan 21, 2021

This depends on #22451 merging. Done.

@morgo
Copy link
Contributor

morgo commented Jan 26, 2021

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 27, 2021
@bb7133
Copy link
Member

bb7133 commented Jan 27, 2021

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 27, 2021
@bb7133
Copy link
Member

bb7133 commented Jan 27, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 27, 2021
@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 22531

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit 8d893eb into pingcap:release-4.0 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/4.0-cherry-pick

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants