infoschema: improve the security vunerability of TABLE_CONSTRAINTS infoschema #14037

reafans · 2019-12-12T03:36:04Z

What problem does this PR solve?

What is changed and how it works?

before：new user can get the whole data of information_schema.TABLE_CONSTRAINTS

mysql -u root -h 127.0.0.1 -P 4000
mysql> create user tester;
mysql -u tester -h 127.0.0.1 -P 4000
mysql> use information_schema;
mysql> select * from TABLE_CONSTRAINTS;
+--------------------+-------------------+-------------------------+--------------+----------------------+-----------------+
| CONSTRAINT_CATALOG | CONSTRAINT_SCHEMA | CONSTRAINT_NAME         | TABLE_SCHEMA | TABLE_NAME           | CONSTRAINT_TYPE |
+--------------------+-------------------+-------------------------+--------------+----------------------+-----------------+
| def                | mysql             | PRIMARY                 | mysql        | columns_priv         | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | GLOBAL_VARIABLES     | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | tidb                 | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | help_topic           | PRIMARY KEY     |
| def                | mysql             | name                    | mysql        | help_topic           | UNIQUE          |
| def                | mysql             | tbl                     | mysql        | stats_meta           | UNIQUE          |
| def                | mysql             | tbl                     | mysql        | stats_histograms     | UNIQUE          |
| def                | mysql             | tbl                     | mysql        | stats_buckets        | UNIQUE          |
| def                | mysql             | delete_range_index      | mysql        | gc_delete_range      | UNIQUE          |
| def                | mysql             | delete_range_done_index | mysql        | gc_delete_range_done | UNIQUE          |
| def                | mysql             | PRIMARY                 | mysql        | role_edges           | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | default_roles        | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | user                 | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | db                   | PRIMARY KEY     |
| def                | mysql             | PRIMARY                 | mysql        | tables_priv          | PRIMARY KEY     |
+--------------------+-------------------+-------------------------+--------------+----------------------+-----------------+
15 rows in set (0.00 sec)

after：the new user can only get an empty set.

mysql -u tester -h 127.0.0.1 -P 4000
mysql> use information_schema;
mysql> select * from TABLE_CONSTRAINTS;
Empty set (0.00 sec)

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Side effects

None

Related changes

None

reafans · 2019-12-12T03:40:24Z

/run all tests

codecov · 2019-12-12T03:41:52Z

Codecov Report

Merging #14037 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14037   +/-   ##
===========================================
  Coverage   80.2241%   80.2241%           
===========================================
  Files           482        482           
  Lines        121087     121087           
===========================================
  Hits          97141      97141           
  Misses        16215      16215           
  Partials       7731       7731

reafans · 2019-12-12T03:42:34Z

/run-all-tests

…oschema_constraints

wjhuang2016 · 2019-12-12T04:44:36Z

infoschema/tables_test.go

+	tk.MustExec("GRANT ALL PRIVILEGES ON mysql.gc_delete_range TO r_gc_delete_range;")
+	tk.MustExec("GRANT r_gc_delete_range TO constraints_tester;")
+	constraintsTester.MustExec("set role r_gc_delete_range")
+	constraintsTester.MustQuery("select * from information_schema.TABLE_CONSTRAINTS where TABLE_NAME='gc_delete_range';").Check(testkit.Rows("def mysql delete_range_index mysql gc_delete_range UNIQUE"))


We can check that the result is no empty? The rows may change if we update TABLE_CONSTRAINTS in future.

Addressed, PTAL

wjhuang2016 · 2019-12-12T04:45:44Z

infoschema/tables.go

 	var rows [][]types.Datum
 	for _, schema := range schemas {
 		for _, tbl := range schema.Tables {
+			if checker != nil && !checker.RequestVerification(ctx.GetSessionVars().ActiveRoles, schema.Name.L, tbl.Name.L, "", mysql.AllPrivMask) {


Use .O instead of .L?

it doesn't matter, both can lead to the same thing, the schema.Name.L, tbl.Name.L will be converted to capital letters in this func.

reafans · 2019-12-12T05:59:57Z

/run-all-tests

reafans · 2019-12-12T06:23:54Z

/run-all-tests

djshow832

LGTM

djshow832 · 2019-12-12T06:42:32Z

infoschema/tables_test.go

+	tk.MustExec("GRANT ALL PRIVILEGES ON mysql.gc_delete_range TO r_gc_delete_range;")
+	tk.MustExec("GRANT r_gc_delete_range TO constraints_tester;")
+	constraintsTester.MustExec("set role r_gc_delete_range")
+	c.Assert(len(constraintsTester.MustQuery("select * from information_schema.TABLE_CONSTRAINTS where TABLE_NAME='gc_delete_range';").Rows()), Greater, 0)


What's the exact count of rows? Is it unpredictable?

I think the count of rows doesn't matter here. We want to check that we can read something here but don't care what it is.

it can be predicted, but may be changed in the future, and what we want is whether the data can be query, not what it is.

reafans · 2019-12-13T05:58:43Z

/run-all-tests

wjhuang2016

lgtm

reafans · 2019-12-13T06:12:55Z

/run-all-tests

…foschema (pingcap#14037)

reafans · 2020-01-02T04:08:19Z

/run-all-tests tidb-test=pr/973

add check for infoschema.table_constrains

71c58db

reafans and others added 4 commits December 12, 2019 11:48

Merge branch 'master' into infoschema_constraints

f300baf

Merge branch 'master' into infoschema_constraints

3afba83

add some test

6154ac1

Merge remote-tracking branch 'origin/infoschema_constraints' into inf…

1a489ab

…oschema_constraints

wjhuang2016 reviewed Dec 12, 2019

View reviewed changes

improve the test

9b54fc6

reafans added component/privilege status/all tests passed type/enhancement The issue or PR belongs to an enhancement. labels Dec 12, 2019

improve the test

d2a4546

djshow832 reviewed Dec 12, 2019

View reviewed changes

djshow832 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 12, 2019

djshow832 reviewed Dec 12, 2019

View reviewed changes

djshow832 removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 12, 2019

Merge branch 'master' into infoschema_constraints

2435b21

wjhuang2016 reviewed Dec 13, 2019

View reviewed changes

Merge branch 'master' into infoschema_constraints

a60758b

reafans added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 13, 2019

wjhuang2016 approved these changes Dec 13, 2019

View reviewed changes

wjhuang2016 merged commit 71a8ef2 into pingcap:master Dec 13, 2019

reafans deleted the infoschema_constraints branch December 13, 2019 06:56

reafans removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 17, 2019

reafans mentioned this pull request Dec 19, 2019

infoschema: improve the security vunerability of key_column_usage and analyze_status infoschema #14142

Merged

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

infoschema: improve the security vunerability of TABLE_CONSTRAINTS in…

c6fa069

…foschema (pingcap#14037)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infoschema: improve the security vunerability of TABLE_CONSTRAINTS infoschema #14037

infoschema: improve the security vunerability of TABLE_CONSTRAINTS infoschema #14037

reafans commented Dec 12, 2019 •

edited

Loading

reafans commented Dec 12, 2019

codecov bot commented Dec 12, 2019 •

edited

Loading

reafans commented Dec 12, 2019

wjhuang2016 Dec 12, 2019

reafans Dec 12, 2019

wjhuang2016 Dec 12, 2019

reafans Dec 12, 2019

reafans commented Dec 12, 2019

reafans commented Dec 12, 2019

djshow832 left a comment

djshow832 Dec 12, 2019

wjhuang2016 Dec 12, 2019

reafans Dec 13, 2019

reafans commented Dec 13, 2019

wjhuang2016 left a comment

reafans commented Dec 13, 2019

reafans commented Jan 2, 2020

infoschema: improve the security vunerability of TABLE_CONSTRAINTS infoschema #14037

infoschema: improve the security vunerability of TABLE_CONSTRAINTS infoschema #14037

Conversation

reafans commented Dec 12, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

reafans commented Dec 12, 2019

codecov bot commented Dec 12, 2019 • edited Loading

Codecov Report

reafans commented Dec 12, 2019

wjhuang2016 Dec 12, 2019

Choose a reason for hiding this comment

reafans Dec 12, 2019

Choose a reason for hiding this comment

wjhuang2016 Dec 12, 2019

Choose a reason for hiding this comment

reafans Dec 12, 2019

Choose a reason for hiding this comment

reafans commented Dec 12, 2019

reafans commented Dec 12, 2019

djshow832 left a comment

Choose a reason for hiding this comment

djshow832 Dec 12, 2019

Choose a reason for hiding this comment

wjhuang2016 Dec 12, 2019

Choose a reason for hiding this comment

reafans Dec 13, 2019

Choose a reason for hiding this comment

reafans commented Dec 13, 2019

wjhuang2016 left a comment

Choose a reason for hiding this comment

reafans commented Dec 13, 2019

reafans commented Jan 2, 2020

reafans commented Dec 12, 2019 •

edited

Loading

codecov bot commented Dec 12, 2019 •

edited

Loading