*: refactor the RestrictedSQLExecutor interface by tiancaiamao · Pull Request #22579 · pingcap/tidb

tiancaiamao · 2021-01-27T13:02:29Z

What problem does this PR solve?

Problem Summary:

Improve security for our code.

What is changed and how it works?

The initial definition of RestrictedSQLExecutor looks like this:

type RestrictedSQLExecutor interface {
	// ExecRestrictedSQL run sql statement in ctx with some restriction.
	ExecRestrictedSQL(sql string) ([]chunk.Row, []*ast.ResultField, error)
}

It's not good for security reasons, the usage pattern looks like this:

sql  = fmt.Sprintf('select xxx from ..%s ', varFromOutside)
stmt, err = ExecRestrictedSQL(sql) 
...

Later on, we add more methods to the interface which IMO, ugly:

	// ExecRestrictedSQLWithContext run sql statement in ctx with some restriction.
	ExecRestrictedSQLWithContext(ctx context.Context, sql string, opts ...OptionFuncAlias) ([]chunk.Row, []*ast.ResultField, error)
	// ExecRestrictedSQLWithSnapshot run sql statement in ctx with some restriction and with snapshot.
	// If current session sets the snapshot timestamp, then execute with this snapshot timestamp.
	// Otherwise, execute with the current transaction start timestamp if the transaction is valid.
	ExecRestrictedSQLWithSnapshot(sql string) ([]chunk.Row, []*ast.ResultField, error)

I propose to use this one as its new definition:

type RestrictedSQLExecutor interface {
	ParseWithParams(ctx context.Context, sql string, args ...interface{}) (ast.StmtNode, error)
	ExecRestrictedStmt(ctx context.Context, stmt ast.StmtNode, opts ...OptionFuncAlias) ([]chunk.Row, []*ast.ResultField, error)
}

This interface is secure, and easy to use:

stmt  = exec.ParseWithParams('select xxx from ..%? ', varFromOutside)
res, err = ExecRestrictedSQL(stmt) 
...

What's Changed:

ParseWithParams() now returns ast.StmtNode rather than []ast.StmtNode, multiple statements are not allowed here.

Update the "util", "store", and "domain" packages, ensure they use the new (secure) API.

How it Works:

Related changes

Need to cherry-pick to the release branch

Check List

Tests

No code

Side effects

Breaking backward compatibility
API change

Release note

No release note

xhebox · 2021-01-27T15:22:13Z

session/session.go

-	// Attention: it does not prevent you from doing parse("select '%?", ";SQL injection!;") => "select '';SQL injection!;'".
-	// One argument should be a standalone entity. It should not "concat" with other placeholders and characters.
-	// This function only saves you from processing potentially unsafe parameters.
-	ParseWithParams(ctx context.Context, sql string, args ...interface{}) ([]ast.StmtNode, error)


What is the reason to remove it in the interface?

The external SQL all use ExecuteStmt or ExecutePrepared ...

And we have the ExecuteInternal for internal SQL already, and also the RestrictedSQLExecutor, there is no need to include this method in the interface.

Do we know that we can convert most uses of Execute to ExecuteInternal safely? My understanding is that ExecuteInternal is similar to Restricted, which is that it does not binary log, runs in the server permissions etc.

If we expect that COM_QUERY is the only valid case of "Execute", then that's fine. It is already switched over to ExecuteStmt (as you mention).

OK, but you still leave it in the RestrictedSQLExecutor interface. I would like both-existing or both-none.

And you can completely extract SQL execution methods into another subinterface... Session could embed all sql executor interfaces,

We can clean up the code after the refactor finish:

Step1: introduce the new API for RestrictedSQLExecutor
Step2: use the new API to replace the old one
Step3: remove the old API

We can embed RestrictedSQLExecutor into the Session interface like this:

type Session interface { RestrictedSQLExecutor ... }

Keep each interface small and composable.... after we clean the usage of the old ones.

morgo · 2021-01-27T15:50:38Z

I like it - much cleaner. Please fix CI:

[2021-01-27T13:04:04.687Z] util/admin/admin.go:302:15:	stmt.Restore(format.NewRestoreCtx(format.DefaultRestoreFlags, &sb))

session/session.go

xhebox

ParseWithParams with handle quotes for you. Adding quotes will cause problems.

util/gcutil/gcutil.go

util/admin/admin.go

store/tikv/gcworker/gc_worker.go

xhebox

Rest LGTM, but you need to fix the CI error.

util/admin/admin.go

xhebox

LGTM

morgo · 2021-01-28T16:49:42Z

LGTM

ti-srebot · 2021-01-28T16:49:45Z

@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).

morgo · 2021-01-28T20:06:53Z

session/session.go

 	// Parse is deprecated, use ParseWithParams() instead.
 	Parse(ctx context.Context, sql string) ([]ast.StmtNode, error)


If we remove ParseWithParams, this comment is now incorrect.

Btw: I discovered in one of my PRs that Parse can not be deprecated. It is important to have a Parse function which does not use placeholders so that a statement like this one can be parsed and then executed by ExecuteStmt:

mysql> select count(*) from t1 where formatted like '%ndan%'; | ERROR 1105 (HY000): missing arguments, need 1-th arg, but only got 0 args

okay, when I finish the rewrite, I'll clean up some deprecated methods and update the comment here.

morgo · 2021-01-29T03:16:16Z

LGTM

ti-srebot · 2021-01-29T03:16:19Z

@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 · 2021-01-29T04:39:18Z

/merge

@morgo 's review is valid. pingcap/community#398

morgo · 2021-01-29T04:40:34Z

LGTM

morgo · 2021-01-29T04:40:50Z

/merge

ti-srebot · 2021-01-29T04:43:13Z

/run-all-tests

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

ti-srebot · 2021-01-29T04:57:37Z

cherry pick to release-4.0 in PR #22621

xhebox · 2021-02-02T06:48:31Z

Do we backport this to 5.0 and 4.0? If not, we can close the cherry-pick PRs. @tiancaiamao

xhebox · 2021-02-03T04:38:42Z

/label needs-cherry-pick-5.0-rc

xhebox · 2021-02-03T04:38:55Z

/run-cherry-picks

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

ti-srebot · 2021-02-03T04:39:31Z

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

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: tiancaiamao <tiancaiamao@gmail.com>

*: refactor the RestrictedSQLExecutor interface

a113996

tiancaiamao requested a review from a team as a code owner January 27, 2021 13:02

tiancaiamao requested review from XuHuaiyu, morgo and xhebox and removed request for a team and XuHuaiyu January 27, 2021 13:02

tiancaiamao added sig/sql-infra SIG: SQL Infra component/session component/store component/util type/4.0-cherry-pick type/enhancement The issue or PR belongs to an enhancement. labels Jan 27, 2021

github-actions bot added the sig/execution SIG execution label Jan 27, 2021

xhebox reviewed Jan 27, 2021

View reviewed changes

fix build

b55cde6

morgo reviewed Jan 28, 2021

View reviewed changes

session/session.go Outdated Show resolved Hide resolved

tiancaiamao added 2 commits January 28, 2021 10:18

make golint happy

a450c1f

address comment

34c0e7b

xhebox reviewed Jan 28, 2021

View reviewed changes

address comment

9689fa5

xhebox reviewed Jan 28, 2021

View reviewed changes

util/admin/admin.go Outdated Show resolved Hide resolved

address comment

bc6af67

xhebox reviewed Jan 28, 2021

View reviewed changes

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

tiancaiamao added needs-cherry-pick-4.0 and removed type/4.0-cherry-pick labels Jan 28, 2021

morgo reviewed Jan 28, 2021

View reviewed changes

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

ti-srebot approved these changes Jan 29, 2021

View reviewed changes

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

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

Merge branch 'master' into sqlexec

a8fc395

ti-srebot merged commit ea6ccf8 into pingcap:master Jan 29, 2021

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

cherry pick pingcap#22579 to release-4.0

5f0b140

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

ti-srebot mentioned this pull request Jan 29, 2021

*: refactor the RestrictedSQLExecutor interface (#22579) #22621

Merged

tiancaiamao deleted the sqlexec branch January 29, 2021 05:00

xhebox mentioned this pull request Feb 1, 2021

*: adapt new api for the executor package #22644

Merged

ti-srebot added the needs-cherry-pick-5.0-rc label Feb 3, 2021

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

cherry pick pingcap#22579 to release-5.0-rc

156242e

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

ti-srebot mentioned this pull request Feb 3, 2021

*: refactor the RestrictedSQLExecutor interface (#22579) #22687

Merged

tiancaiamao added a commit that referenced this pull request Feb 22, 2021

*: refactor the RestrictedSQLExecutor interface (#22579) (#22687)

2e791d4

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: tiancaiamao <tiancaiamao@gmail.com>

ti-chi-bot pushed a commit that referenced this pull request Mar 3, 2021

*: refactor the RestrictedSQLExecutor interface (#22579) (#22621)

ae010ce

		// Parse is deprecated, use ParseWithParams() instead.
		Parse(ctx context.Context, sql string) ([]ast.StmtNode, error)

Conversation

tiancaiamao commented Jan 27, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

xhebox Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

tiancaiamao Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

morgo Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

tiancaiamao Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

morgo commented Jan 27, 2021

Uh oh!

Uh oh!

xhebox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xhebox left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xhebox left a comment

Choose a reason for hiding this comment

Uh oh!

morgo commented Jan 28, 2021

Uh oh!

ti-srebot commented Jan 28, 2021

Uh oh!

morgo Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

tiancaiamao Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

morgo commented Jan 29, 2021

Uh oh!

ti-srebot commented Jan 29, 2021

Uh oh!

tiancaiamao commented Jan 29, 2021

Uh oh!

morgo commented Jan 29, 2021

Uh oh!

morgo commented Jan 29, 2021

Uh oh!

ti-srebot commented Jan 29, 2021

Uh oh!

ti-srebot commented Jan 29, 2021

Uh oh!

xhebox commented Feb 2, 2021

Uh oh!

xhebox commented Feb 3, 2021

Uh oh!

xhebox commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-srebot commented Feb 3, 2021

Uh oh!

morgo Jan 28, 2021 •

edited

Loading

xhebox left a comment •

edited

Loading

xhebox commented Feb 3, 2021 •

edited

Loading