Description Alvaro Munoz 2020-04-14 13:58:13 EDT

Hi,

The GitHub Security Labs (https://securitylab.github.com) team has identified potential security vulnerabilities in the Expression Language 3.0 RI (https://github.com/eclipse-ee4j/el-ri) project.

We are committed to working with you to help resolve these issues. In this report you will find everything you need to effectively coordinate a resolution of these issues with the GSL team.

If at any point you have concerns or questions about this process, please do not hesitate to reach out to us at `securitylab@github.com` (please include your `GSL-ID`).

If you are _NOT_ the correct point of contact for this report, please let us know!

Cheers,
A

# GitHub Security Labs (GSL) Vulnerability Report: `GSL-2020-021`

## Overview

A bug in the [ELParserTokenManager](https://github.com/apache/tomcat/blob/master/java/org/apache/el/parser/ELParserTokenManager.java) enables invalid EL expressions to be evaluated as if they were valid. For example, the following message will evaluate an invalid EL expression and the interpolated message will be `1+1 = 2`:

```
1+1 = $\#{1+1}
```
(Note: EL expression delimiter is escaped and therefore it should be treated as a literal expression and not be evaluated)

This bug enables attackers to bypass input sanitation (escaping, stripping) controls that developers may have put in place when handling user-controlled data in error messages.

## Product
Eclipse-EE4J Expression Language Reference Implementation
 
## Tested Version
3.0.3

## Details

### Incorrect EL expression tokenization

EL expressions are used in many places. One of them is in [Bean Validation constraint error messages](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-message-interpolation) where it can take user-controlled data. As specified in [Hibernate Validator documentation](https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/?v=6.1#_the_code_constraintvalidatorcontext_code):

> Note that the custom message template is passed directly to the Expression Language engine.
> Thus, you should be very careful when integrating user input in a custom message template as it will be interpreted by the Expression Language engine, which is usually not the behavior you expect and could allow malicious users to leak sensitive data.
> If you need to integrate user input, you should:
> - either escape it by using the Jakarta Bean Validation message interpolation escaping rules;
> - or, even better, pass it as message parameters or expression variables by unwrapping the context to HibernateConstraintValidatorContext.

Several applications attempt to prevent such EL injections by replacing the EL opening delimiter `${` with just `{`. e.g.:

```
    public String replaceElDelimiter(final String value) {
        if (value != null) {
            return value.replaceAll("\\$+\\{", "{");
        }
        return null;
    }
```

This is seemingly a secure way to prevent injection attacks since all occurrences of `${` will be replaced with `{`, and since the regex matches repeating `$` it will also fix more intricate injection attempts that send e.g. `$${` in an attempt to arrive at the `${` delimiter to achieve EL execution.

A way of bypassing this control is trying to use deferred expressions instead (`#{expr}`) since they are not protected by this function. However, the bean validation specs do not allow the use of deferred expressions and Bean Validation implementations such as Apache BVal enforce this restriction by escaping them (`#{expr}` -> `\#{expr}`). [For example](https://github.com/apache/bval/blob/master/bval-jsr/src/main/java/org/apache/bval/el/ELFacade.java#L75):

``` java
// Java Bean Validation does not support EL expressions that look like JSP "deferred" expressions
return expressionFactory.createValueExpression(context,
    EvaluationType.DEFERRED.regex.matcher(message).replaceAll("\\$0"), String.class).getValue(context)
    .toString();
```

This way the underlying EL processor should ignore them and treat them as literal expressions.

However, a bug in the Eclipse EE4J EL implementation allows attackers to bypass this protection with a payload such as `FOO $\#{payload}`. This should be considered a literal expression and not be evaluated, however this is not the case.

The bug seems to be in the [parser's grammar](https://github.com/eclipse-ee4j/el-ri/blob/master/impl/src/main/java/com/sun/el/parser/ELParser.jjt). Specifically in:

```
<DEFAULT> TOKEN :
{
  < LITERAL_EXPRESSION:
    ((~["\\", "$", "#"])
      | ("\\" ("\\" | "$" | "#"))
      | ("$" ~["{", "$", "#"])
      | ("#" ~["{", "$", "#"])
    )+
    | "$"
    | "#"
  >
|
  < START_DYNAMIC_EXPRESSION: "${" > {stack.push(DEFAULT);}: IN_EXPRESSION
|
  < START_DEFERRED_EXPRESSION: "#{" > {stack.push(DEFAULT);}: IN_EXPRESSION
}
```

A `$` or `#` followed by a character that is not `{`, `$` or `#` will be treated as a literal expression. The interesting case is where the character following the `$` or `#` chars is a backslash. The parser will consume the backslash as part of the literal expression and will leave the character that follows it unescaped.

#### Remediation
Looking for the same bug in different EL implementations we found that the Tomcat EL implementation is not vulnerable. When studying [their grammar](https://github.com/apache/tomcat/blob/master/java/org/apache/el/parser/ELParser.jjt) we note that they account for backslashes as well:

```
<DEFAULT> TOKEN :
{
  /*
   * The following definition uses + rather than * in two places to prevent
   * LITERAL_EXPRESSION matching the empty string that could result in the
   * Parser entering an infinite loop.
   */
  < LITERAL_EXPRESSION:
    (   (~["$", "#", "\\"])* "\\" (["$", "#"])?
      | (~["$", "#"])* (["$", "#"] ~["{", "$", "#", "\\"])
      | (~["$", "#"])+
    )+
    | "$"
    | "#"
  >
|
  < START_DYNAMIC_EXPRESSION: "${" > {deque.push(DEFAULT);}: IN_EXPRESSION
|
  < START_DEFERRED_EXPRESSION: "#{" > {deque.push(DEFAULT);}: IN_EXPRESSION
}
```

Git history reveals that this issue was found accidentally while fixing a different bug. The bug-fix commit can be found [here](https://github.com/apache/tomcat/commit/7bd0e358ba5a07df1acb69829eec1ddad9461d09). A similar patch should fix the Jakarta EL implementation as well.

#### Impact

This issue may lead to mitigation bypasses that allow for remote code execution in affected applications.

## GitHub Security Advisories

We recommend you create a private [GitHub Security Advisory](https://help.github.com/en/github/managing-security-vulnerabilities/creating-a-security-advisory) for these findings. This also allows you to invite the GSL team to collaborate and further discuss these findings in private before they are [published](https://help.github.com/en/github/managing-security-vulnerabilities/publishing-a-security-advisory).

## Credit

This issue was discovered and reported by GSL team member [@pwntester (Alvaro Muñoz)](https://github.com/pwntester).

## Contact

You can contact the GSL team at `securitylab@github.com`, please include the `GSL-ID` in any communication regarding this issue.

## Disclosure Policy

This report is subject to our [coordinated disclosure policy](https://securitylab.github.com/disclosures#policy).