Support preventing and reporting feature for SQL injection

[tokuhirom]

I want the feature that was implemented in #1953.

But it's already closed by this message:

Um...this feature seems to discourage the use of ${}, however, the right way to go is to learn/understand how ${} works and use it correctly.

@harawata said:

We can reconsider at any time if this turns out to be a popular request. :)

I posted the comment on the closed #1953 but there's no reaction. As a result, I create a new issue.

[harawata]

Hello @tokuhirom ,

I still believe this should not be a built-in feature. As mentioned in the linked issue, string substitution is a crucial feature and we should not discourage its usage in any way.

If it is super-important to your team, you may be able to write a simple language driver and use it in the development phase.

import org.apache.ibatis.builder.BuilderException;
import org.apache.ibatis.mapping.SqlSource;
import org.apache.ibatis.parsing.XNode;
import org.apache.ibatis.scripting.xmltags.XMLLanguageDriver;

public class NoobLanguageDriver extends XMLLanguageDriver {
  @Override
  public SqlSource createSqlSource(Configuration configuration, XNode script, Class<?> parameterType) {
    doCheck(script.toString());
    return super.createSqlSource(configuration, script, parameterType);
  }

  @Override
  public SqlSource createSqlSource(Configuration configuration, String script, Class<?> parameterType) {
    doCheck(script);
    return super.createSqlSource(configuration, script, parameterType);
  }

  private void doCheck(String script) {
    if (script.contains("${")) {
      throw new BuilderException("Found ${} : " + script);
    }
  }
}

This approach has some advantages over an out-of-box solution, IMHO.

• It has no impact on production performance whatsoever.
• It should be possible to allow ${} in particular statements. It's highly likely that people want this capability (cf.).

[tokuhirom]

But so, we may use $ {} in some cases.

For example: SELECT * FROM foo ORDER BY ${column} DESC. It's the correct way to use ${}. It's a very common use case.

If all of the developers can care about the value of the $column, there's no problem. But humans make mistakes. I want a way to use injectionFilter. Since I want to allow ${} in some cases, but I want to deny the illegal value, that taken by the variable.

And performance is not a big deal. In most of cases, the regular expression matching cost is negligible compared to the communication cost with the DB server.

[harawata]

As the language driver receives the entire statement, you should be able to allow particular parameter ...

private static final Pattern pattern = Pattern.compile("\\$\\{(.*?)}");

private void doCheck(String script) {
  Matcher matcher = pattern.matcher(script);
  while(matcher.find()) {
    if (!allowedNames.contains(matcher.group(1))) {
      throw new BuilderException("Found ${} : " + script);
    }
  }
}

... or particular statement.

private void doCheck(String script) {
  if (!script.contains("${")) {
    return;
  }
  try {
    String hash = HexFormat.of().formatHex(
        MessageDigest.getInstance("md5").digest(
            script.getBytes(StandardCharsets.UTF_8)));
    if (!allowedHashes.contains(hash)) {
      throw new BuilderException("Found ${} : " + script);
    }
  } catch (NoSuchAlgorithmException e) {
    e.printStackTrace();
  }
}

(these are just for demonstrating ideas and are not tested)

[liudongmiao]

@tokuhirom I think you may need a custom OGNL to scan static XML or Java Annotation.

We have defined a custom ClassLoader to modify the MyBatis' way. In our cases, we are doing more. Analysis the XML without actual parameter to get SQL, then check SQL grammar, including dollar.