Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 27, 2021

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