Skip to content

*: introduce new API ParseWithParams#22499

Merged
ti-srebot merged 21 commits intopingcap:masterfrom
xhebox:injection
Jan 26, 2021
Merged

*: introduce new API ParseWithParams#22499
ti-srebot merged 21 commits intopingcap:masterfrom
xhebox:injection

Conversation

@xhebox
Copy link
Contributor

@xhebox xhebox commented Jan 22, 2021

What problem does this PR solve?

Problem Summary: This PR adds a new API ParseWithParams to help process unsafe arguments than just fmt.Sprintf. Also a helper API ExecuteInternal that is using ParseWithParams and ExecuteStmt.

I did not use PrepareStmt since it is not possible to use placeholder like select * from t where c in ?. But we do have such requirement.

ExecuteInternal is redefined to use ParseWithParams and always use utf8 charset for safety. But it is still needed to modify cases like ExecuteInternal(fmt.Sprintf(...)).

ExecRestrictedSQL is too large, thus it is both annoying and duplicated to write a new RestrictedSQLExecutor based on ParseWithParams. From the git history, this is a very legacy API that is 4 or 5 years old. It should be removed/refactored in further works. So the current plan is, write like ExecRestrictedSQL(session.EscapeSQL(sql, args...)). And it will goes to the modified ExecuteInternal eventually to use utf8 charset to prevent attacks based on charsets.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

xhebox added 2 commits January 22, 2021 17:45
@xhebox
*: parameterized Parse api to prevent injection
dd15707 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: redefine ExecuteInternal api
caa3843 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox xhebox requested a review from a team as a code owner January 22, 2021 09:59
@xhebox xhebox requested review from wshwsh12 and removed request for a team January 22, 2021 09:59
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jan 22, 2021
xhebox added 2 commits January 22, 2021 19:32
@xhebox
*: change specifier
28ed3bc 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: fix test
47347fb 
Signed-off-by: xhe <xw897002528@gmail.com>
morgo
morgo reviewed Jan 22, 2021
View reviewed changes
xhebox added 2 commits January 23, 2021 10:06
@xhebox
*: use charset constant
0ab0295 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: fix test
dfc828b 
Signed-off-by: xhe <xw897002528@gmail.com>
@morgo
Copy link
Contributor

morgo commented Jan 24, 2021

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 24, 2021

@morgo, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIGs: execution(slack),sql-infra(slack).

@tiancaiamao tiancaiamao self-requested a review January 25, 2021 03:04
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
session/utils.go
buf[pos] = '\\'
buf[pos+1] = '\\'
pos += 2
default:
Copy link
Contributor

@tiancaiamao tiancaiamao Jan 25, 2021

Choose a reason for hiding this comment

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

Is \t a special one?

Copy link
Contributor Author

@xhebox xhebox Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think \t should be OK. It is just another type of whitespace. And the original port did not include that, either.

Copy link
Contributor

@tiancaiamao tiancaiamao Jan 25, 2021

Choose a reason for hiding this comment

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

In the MySQL document, there is a "Table 9.1 Special Character Escape Sequences"
https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

I'm not sure are they the same things.

Copy link
Contributor Author

@xhebox xhebox Jan 25, 2021

Choose a reason for hiding this comment

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

I think that is the reverse of escaping. The table means that mysql will interpret \t as 0x09 in string datum. And it only takes effect if NO_BACKSLASH_ESCAPES is off.

Here what we want is, disallow the parser/lexer treating any character in the string as any special term/token.

tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
tiancaiamao
tiancaiamao reviewed Jan 25, 2021
View reviewed changes
xhebox added 5 commits January 25, 2021 20:06
@xhebox
*: fix tracing
fbe06a2 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: one more test
b52e415 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: do not export internal functions for now
be32b05 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: strip the use of string.Index
a2d16ad 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
*: add microsecond
c0e0930 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
Copy link
Contributor Author

xhebox commented Jan 27, 2021

/label needs-cherry-pick-3.0

@xhebox
Copy link
Contributor Author

xhebox commented Jan 27, 2021

/run-cherry-pick

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 27, 2021
@xhebox @ti-srebot
cherry pick pingcap#22499 to release-5.0-rc
61d978b 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 27, 2021
Merged
@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 27, 2021

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

@ti-srebot ti-srebot mentioned this pull request Jan 27, 2021
Merged
@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 27, 2021

cherry pick to release-4.0 in PR #22548

@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 27, 2021

cherry pick to release-3.1 failed

@ti-srebot ti-srebot mentioned this pull request Jan 27, 2021
Merged
@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 27, 2021

cherry pick to release-3.0 in PR #22549

@xhebox xhebox changed the title *: preventing SQL injection *: introduce new API ParseWithParams Jan 27, 2021
@xhebox xhebox mentioned this pull request Feb 1, 2021
Merged
ti-srebot added a commit that referenced this pull request Feb 2, 2021
@ti-srebot
*: introduce new API ParseWithParams (#22499) (#22547)
0f873db 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: xhe <xw897002528@gmail.com>
This was referenced Feb 3, 2021
Closed
Merged
Merged
@xhebox
Copy link
Contributor Author

xhebox commented Feb 20, 2021

/label needs-cherry-pick-2.1

@ti-srebot
Copy link
Contributor

ti-srebot commented Feb 20, 2021

These labels are not found needs-cherry-pick-2.1.

xhebox added a commit to ti-srebot/tidb that referenced this pull request Feb 20, 2021
@xhebox
*: preventing SQL injection (pingcap#22499)
54a0d46 
Signed-off-by: xhe <xw897002528@gmail.com>
xhebox added a commit to xhebox/tidb that referenced this pull request Feb 20, 2021
@xhebox
*: preventing SQL injection (pingcap#22499)
4b20bef 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox xhebox mentioned this pull request Feb 20, 2021
Merged
ti-srebot added a commit that referenced this pull request Feb 20, 2021
@ti-srebot
*: introduce new API ParseWithParams (#22499) (#22549)
d9a4917 
Signed-off-by: xhe <xw897002528@gmail.com>
@morgo morgo mentioned this pull request Feb 22, 2021
Closed
xhebox added a commit to ti-srebot/tidb that referenced this pull request Feb 25, 2021
@xhebox
cherry pick pingcap#22499 to release-4.0
ceb06b8 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
ti-chi-bot pushed a commit that referenced this pull request Mar 1, 2021
@ti-srebot
*: introduce new API ParseWithParams (#22499) (#22548)
d77d908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@morgo morgo morgo left review comments

@tiancaiamao tiancaiamao tiancaiamao left review comments

@AilinKid AilinKid AilinKid left review comments

@bb7133 bb7133 bb7133 approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees

No one assigned

Labels

require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xhebox @morgo @ti-srebot @tiancaiamao @AilinKid @bb7133