Feature Request: Input Processor/Sanitizer for insert and update statements

[jonl-percsolutions-com]

Considering that sanitizing user input, especially from the internet, is important for preventing javascript, sql and other injections, it would be nice if in defining an "insert" or "update" statement in xml or by annotation, I could specify a processor/sanitizer that would validate/cleanup input values.

It could possibly be specified as follows:

<insert id="insertExample1" sanitizerType="org.example.NoJavaScriptSanitizer">
    insert into table a (col1, col2)
    values (#{col1},#{col2:sanitizer=org.example.SafeHtmlSanitizer}</sanitizer>
</insert>

<insert id="insertExample2" sanitizerMap="insertExample2SanitizerMap">
     insert into table b (col1, col3)
    values (#{col1},#{col2},#{col3})
</insert>
<sanitizerMap id="insertExample2SanitizerMap">
        <sanitizer type="org.example.SafeHtmlSanitizer" inputs="col2,col3"/>
        <sanitizer type="org.example.NoJavaScriptSanitizer"/>
</sanitizerMap>

 @Insert("insert into table a (col1, col2)
    values (#{col1},#{col2:sanitizer=org.example.SafeHtmlSanitizer}", sanitizer=NoJavaScriptSanitizer.class);
  public int insertExample3(@Param("col1") int col1, @Param("col2") String col2 )

The sanitizer would be called when resolving the values of the #{...} tokens before calling setNonNullParameter() for the value type handler.
The sanitizerType/sanitizerMap would work similar to the resultType/resultMap functionality.

[emacarron]

HI. I like the idea but only for SQL inyection, not for XSS. IMHO sanitizing XSS is something that should happen in the web layer, not in the persistence level. Filters like ESAPI or HDIV do the job really well and they protect also from CRSF and other attacks (invalid object reference, invalid redirects...).

But it is true that using ${} expressions opens a door for SQL injection. So for example, to make sure that an app built with MyBatis is secure you should search for ${} expressions what is quite tedious.

What about adding a generic pattern to process all ${} expressions?

It would be just adding a some new parameters to control the filtering of values that will end up in a ${} expression.

Parameter suggestions:
enableInjectionValidation=true|false (defaults to false)
injectionExpression=see below for defaults

For this parameter options are:
Including a blacklist pattern to match SQL control characters. This one is taken from HDIV

(\s|\S)*((%27)|(')|(%3D)|(=)|(/)|(%2F)|(")|((%22)|(-|%2D){2})|(%23)|(%3B)|(;))+(\s|\S)*

Or using a whitelist (letters, numbers, point and underscore)

^[a-zA-Z0-9._]*$

Though in SQL we already know quite well which are the invalid control characters, using a whitelist is a good practice in security so I am for the second.

This way, making MyBatis safe will just be adding the parameter enableInjectionValidation=true

What do you think?

[emacarron]

Lets keep this open for a while to let people review it.

[jonl-percsolutions-com]

This is a good start. This pattern matching could be powerful.

A few points:

I would update the default validation to allow hyphen as people store UUIDs in the database increasingly.

I would also include a couple of examples in the documentation for what might be common usages

The only other thing that might make it better would be to provide different injection patterns for different values or different insert/select/update statements. IE, for select and delete I might want a default pattern that is a whitelist, since searching should generally only be with letters/numbers/etc, but for insert/update, and maybe a special select, I might need a more lenient pattern than what is globally configured

So could you add the enableInjectionValidation/injectionExpression to the insert/select/update/delete declarations as well?

You could even add an "expression" tag that defines a reusable injectionExpression. Then you could have something like the following:

 <expression id="everthingButSQL">
      (\s|\S)*((%27)|(')|(%3D)|(=)|(/)|(%2F)|(")|((%22)|(-|%2D){2})|(%23)|(%3B)|(;))+(\s|\S)*
 </expression>

 <insert id="defaultInerstValidationBasedOnGlobalConfig">
     some insert statement here
 </insert>

 <insert id="safeInsert" enableInjectionValidation="true">
     some insert statement here
 </insert>

 <insert id="unsafeInsert" enableInjectionValidation="false">
     some insert statement here
 </insert>

 <insert id="mixedInjectionValidation1" enableInjectionValidation="true">
       insert into some table (id, value)
       VALUES(#{id}, #{value,insertExpressionId=everthingButSQL})
 </insert>

 <insert id="mixedInjectionValidation2" enableInjectionValidation="true">
       insert into some table (id, value1,value2)
       VALUES(#{id}, #{value1,insertExpressionId=everthingButSQL},#{value2})
 </insert>

[tguzik]

I agree this is a good start. We could also provide the ability to escape special characters, instead of denying to execute the statement. Guava has a couple of escapers that could serve as examples: XmlEscaper, HtmlEscaper

...but of course there should be a way to provide own settings, because hardcoding escaping to, for example, XML entities is questionable at least.

[bwzhang2011]

@emacarron, any update with such issue ? 3.3.0 seems a long story since last snapshot, will be ready to release the final version ?

[emacarron]

Hi @bwzhang2011 I finally removed the change because I cannot see clearly the use case. Let me explain my point of view.

Regarding SQL injection

SQL injection happens when you tell the DB engine to execute user input.

There are some key points:

• This only happens with Strings
• This only happens when not using prepared statements
• This only happens with user inputs

The change I made worked only for ${} expressions. They are always strings and not parameters of a prepared statement. So, this covers points number 1 && 2.

The main problem is that we do not know if the data is a user input or not. You may use ${} expressions for any reason, not only for parameters. So there is no way that we can provide a generic default behaviour.

The rule of thumb is: mark the data as data. In the case of a db the right approach is using prepared statements.

Regarding XSS

The problem here is similar. It is OK from a persistence perspective to insert HTML or Javascript in a database. The problem happens when this data comes from user input and you are building a Web site .

Trying to stop XSS by filtering/validating input is not the right approach given that the problem of injections is not the input itself but passing data to an execution context.

And anyway I would never expect this to be solved by the persistence layer. If someone wants to validate/escape input, it should be done in sync with the error handling code.

The rule of thumb here is almost the same: mark the data as data. In this case you should use the proper escaping according to the execution context (javascript, html, xml...) when the output happens.

[jonl-percsolutions-com]

@emacarron

I can agree with your assessment when it comes to passing data to a db, however, mybatis is not a db, its not the actual execution context, it is a data access layer around the execution context the provides many facilities that would otherwise need to be hand written in order to accomplish, like data object instantion, etc. The point of my original request was to reduce the number of wrappers around the data access layer to hand input sanitizing. IE provide one configuration point via the annotation or xml file that tells the data access layer how to handle a piece of data.

The reason I asked for the additional facility of a new parameter on the definitions of the insert/update xml/annotation was because I did not want to have to go and override the handlers for Strings in all contexts, however, that would be another way to do it if you don't feel providing the "juice is worth the squeeze".

[harawata]

I agree with @emacarron on this.
Validation/sanitization are the task of another layer.

---

I could be missing something, but can't you use a custom type handler to achieve it?
I mean, instead of writing

#{col2:sanitizer=org.example.SafeHtmlSanitizer}

you just write

#{col2, typeHander=org.example.SafeHtmlTypeHandler}

and do what you have to do in the type handler code.

[jonl-percsolutions-com]

@harawata

Thats exactly what I said, it could be done with a custom handler, but I thought it would be cleaner and more flexible as a separate mechanism, not just on the column, but for use on a whole statement.

I think, though, the argument that custom type handlers can be used instead is a stronger case against it than that its not the appropriate layer.