mybatis / mybatis-3

MyBatis SQL mapper framework for Java
http://mybatis.github.io/mybatis-3/
Apache License 2.0
19.68k stars 12.81k forks source link

add a tag like #{} ${} reopen! i have a new idea! #1941

Open k4n5ha0 opened 4 years ago

k4n5ha0 commented 4 years ago

Hi! i am a web security engineer. With my communicate, my colleagues accepted use #{} is safe. If don't check code very carefully, useing ${} wil be create a sql injection Also i add some rules into sonarqube to check xml can't have ${} But ordey by and group by must use ${} So I thinging if mybatis can add a tag , something like @{} !{} In this new tag,Mybatis can prevent "sql injection char" likes *' " , ( ) { } @** nine chars Or In this new tag only accept letters and char regex: [a-zA-Z] So #{} and new tag canbe safe. Only some situation coder will useing ${}. Thx.

harawata commented 4 years ago

Hello @k4n5ha0 ,

Filtering characters can never be a good strategy to prevent SQL injection as there are many techniques to circumvent those filters. The only reliable protection is to use #{}.

For ORDER BY, there usually are several choices, so you may be able to write as follows to avoid ${}.

ORDER BY
<choose>
  <when test="orderBy == 1">
    id desc
  </when>
  <when test="orderBy == 2">
    date_submitted desc
  </when>
  ...

If your app allows end users to supply arbitrary ORDER BY / GROUP BY clause, your job will be very hard...

k4n5ha0 commented 4 years ago

ok thx very much :-)

k4n5ha0 commented 4 years ago

Hi!I have a new idea! How about build a safemode like fastjson. url: https://github.com/alibaba/fastjson/wiki/fastjson_safemode 1) ParserConfig.getGlobalInstance().setSafeMode(true); 2) fastjson.parser.safeMode=true 3) -Dfastjson.parser.safeMode=true Three ways to open a safemode.

When Mybatis open this mode, mybatis's $ will throw a Exception. So this mode will prevent Sql Injection! And Forcing programmers use # Thx!

harawata commented 4 years ago

${} is an important feature and I am not comfortable with adding an option to disallow its usage. It is not that difficult to use ${} safely, so you should educate your programmers instead of banning it. In case you really have to detect ${}, you may be able to check text nodes in a custom language driver, probably (I do not recommend using it on production for performance reason, though).

k4n5ha0 commented 4 years ago

hi , one of my friend working in a bank , he tells me this bank is using fortify to check mybatis's xml isnt have ${} .i talked with him , he tells me more about mybatis, some company is using "changed mybatis" to reduce sqlinection Possibility, so i wich this feather will be create.secre by deault. thx.

k4n5ha0 commented 1 year ago

Hi,i really thinks create A new tag(for example !{})is a good idea,beacause a tag which: use allowrule regex [a-zA-Z0-9_] or use denyrule in tag string can't include ' " < > % ( ) may help programmers reduce sql injection code.

sql injection with like

Select * from news where title like '%${title}%'

can be

Select * from news where title like '%!{title}%'

sql injection with in

Select * from news where id in (${id})

can be

Select * from news where id in (!{id})

sql injection with order by

Select * from news where title = 'time' order by ${time} asc

can be

Select * from news where title = 'time' order by !{time} asc

this new tag can fix sqlinjection very quickly with lower time.

xiaoqin00 commented 1 year ago

great,We must take security risks more seriously

harawata commented 1 year ago

As I wrote in my first comment, filtering characters can never be a good strategy to prevent SQL injection. Trying to filter characters is the opposite of taking the security risk seriously and we will not provide such feature.