matrixone
matrixone copied to clipboard
matrixorigin
[Bug]: potential SQL injection
Is there an existing issue for the same bug?
- [X] I have checked the existing issues.
Environment
- Version or commit-id (e.g. v0.1.0 or 8b23a93):
- Hardware parameters:
- OS type:
- Others:
Actual Behavior
- start mo-service
- execute this SQL statement:
create role 'foo"; drop table foo; select "';
result:
ERROR 1105 (HY000): Only CREATE of DDL is supported in transactions
The injected drop statement is executed.
Expected Behavior
No SQL injection
Steps to Reproduce
No response
Additional information
No response
what the value of select @@autocommit; in the case.
As designed in v0.6, DROP can not be in an uncommitted transaction.
what the value of
select @@autocommit;in the case. As designed in v0.6, DROP can not be in an uncommitted transaction.
The problem is the injection, not the drop statement. The injected statement should not be executed at all.
@reusee could u pls test it at the latest commit and update the issue status, thank u.
@sukki37 I checked the discussions in https://github.com/matrixorigin/matrixone/pull/5684. I believe the injection problem is not fixed yet.
@daviszhen assigned to you. This issue has a broad issue. We need to examine ALL sql statement constructed with fmt.Sprintf and escape all db/table/view/column/user names.
This is a pretty big task, I think it is better set aside in 0.7 with a sr. engineer, or two. It needs to be planned.
Postpone to 0.8.
As I understand, we want to make sure we never construct SQL from what a user typed in. Just switch all execution to use param binding. So this should not be a big or risky change, but yes, we want to be thorough.
after discuss with @reusee @domingozhang @heni02 @qingxinhome what will do in this issue are: 1、find the case which rewrite some input to SQL and execute. (eg: we will rewrite 'show xx' to 'select xxx from xx', 'create xx' to 'select xx from yy where cc !=xx' & 'insert into yy values xx'); 2、make sure the rewrite action is safe. have not SQL injection
1、SQL injection in MO, now: you run these sql, will drop table foo.
drop user `foo"; drop table foo; select "1`;
create role 'foo"; drop table foo; select "';
why? because we do some sql with fmt.Printf in background. eg: when we run 【drop user usr1】， we will run this sql in background to check if user exists
// user is a string, value is 'usr1'
getPasswordOfUserFormat := `select user_id,authentication_string,default_role from mo_catalog.mo_user where user_name = "%s";`
bh.Exec(ctx, fmt.Sprintf(getPasswordOfUserFormat, user))
then if we run sql 【drop user foo"; drop table foo; select "1】，we actually run
select user_id,authentication_string,default_role from mo_catalog.mo_user where user_name = "foo"; drop table foo; select "1"
// SQL injection happen!
2、we can found SQL injection in these case to use fmt.Sprinf get running SQL:
str := "select * from t1 where str like `%s`" //use "" to define string. with %s to get string input
sql := fmt.Sprintf(str, "a`;drop database db1;select 1 from t1 where a=`a") //injection happen！ db1 was droped
str = `select * from t1 where str like "%s"` //use `` to define string, with %s to get string input
sql = fmt.Sprintf(str, "a\";drop database db1;select 1 from t1 where a=\"a") //injection happen！ db1 was droped
str = "select * from t1 where int > %d" //use "" to define string. with %d to get numeric input
// this sql will output like : select * from t1 where int > %!!(MISSING)d(string=%!a(MISSING);drop database db1;select (1)
sql = fmt.Sprintf(str, "%a;drop database db1;select(1") //injection happen！ db1 was droped
how to fixed it？ solution 1 try to make every input have a rule（like : user_name can only use alphabet&integer）, and check it before fmt.Sprintf.
solution 2 try to add a common function to check the input (maybe we need the orgin SQL and input, because we need the arg is string or numeric), make sure input can not let SQL injection happen.
solution 3 because : 3.1 all SQL injection will change the origin SQL to multi SQLs, and do some evil in one of the statements. 3.2 the SQL(rewrite by fmt.Sprintf) was executed by BackgroundHandler.Exec method so maybe we can: check if SQL is one statement in Exec() method. if SQL was multi statements, seems SQL injection happen.
Yes, I will take solution 3. because solution 1&2, we need to do lot of work and can not sure we do it all.
fix sql injection like: 1、change one execute sql to multi sql 2、add some expr after sql, make the where clause incorrect
we also have some fmt.Sprintf to rewrite sql in Task system. but i found that's background execute. no input. so I do nothing for that. main change in authenticate.
commit id:5265933d7a097a964fb4cfc126fe034789f3f6bf
mysql> drop user foo"; drop table foo; select "1;
ERROR 20101 (HY000): internal error: invalid input
mysql> create role 'foo"; drop table foo; select "';
ERROR 20101 (HY000): internal error: invalid input
mysql> drop account sys";drop database system; select ";
ERROR 20101 (HY000): internal error: invalid input
@ouyuanning plz confirm are these correct？
mysql> drop database system";drop table system.statement_info;select ";
ERROR 1008 (HY000): Can't drop database 'system";drop table system.statement_info;select "'; database doesn't exist
mysql> create database sys";drop database system; select ";
Query OK, 1 row affected (0.06 sec)
mysql> create user sys";drop database system; select ";
ERROR 1064 (HY000): SQL parser error: You have an error in your SQL syntax; check the manual that corresponds to your MatrixOne server version for the right syntax to use. syntax error at line 1 column 49 near "";
that's not SQL injection. but, i think we can check user name & database name at first
