Misleading ExpressionLanguage Documentation

[dbohannon]

The ExpressionLanguage component page (https://symfony.com/doc/current/components/expression_language.html) states:

“Expressions can be seen as a very restricted PHP sandbox and are immune to external injections as you must explicitly declare which variables are available in an expression.”

However, this statement is a bit misleading as expression injection is very possible when mapping tainted data to expression variables in the $values array. Even though an injected expression is limited to a predefined set of variables and operators, an attacker can still modify the expression and alter application logic. The documentation also states that the ExpressionLanguage component may be used for validation which further increases the likelihood that a developer will pass tainted data into the expression and create an expression injection vulnerability. I suggest updating the documentation at https://github.com/symfony/symfony-docs/blob/master/components/expression_language.rst to explicitly state that tainted data should not be passed to the $values array (or appended directly to the expression string).

To demonstrate the issue, consider the example code block below:

$user = $_GET['user'];
if(!$language->evaluate('user["username"] matches "/[a-zA-Z0-9]$/"', array("user" => $user))){
 return new Response("Validation failed!");
}

Passing the following user parameter will alter the expression and bypass the validation:
<script>alert('XSS_SUCCESS!')</script>" > 0 | "a

I will try to update the documentation and submit a pull request as time permits.

[javiereguiluz]

@dbohannon this is an interesting proposal. Thanks for the insights!

However, instead of adding more explanations or complicating docs, what if we just remove this controversial paragraph entirely?

[dbohannon]

I still think the documentation should explicitly state that tainted data should not be passed to the $values array, especially since other documentation pages talk about using ExpressionLanguage for validation. If a developer attempts to validate user-controlled data then they will be open to expression injection.

[weaverryan]

Yea, we do need a warning somewhere about this - it's not obvious (at least to me) that a dynamic value has the ability to affect the underlying logic like this.

@dbohannon what other examples are there where this might be used? I can see with your example that you could "fool" the value being passed to matches. I suppose you could also do with pretty much any operand?

$price = $_GET['price'];
if(!$language->evaluate('price < 0', array("price" => $price))){
 return new Response("Validation failed!");
}

being passed the following (string) value:

-100 + 200

Is that another example? I'm trying to see how generic/specific the problem is to determine the best wording. Also, is there a generic way to escape the input values, or not? My instinct is no - it's not a matter of escaping certain characters... it depends on how you're using the value, correct?

Thanks!

[dbohannon]

You're correct that you can use pretty much any built-in operand to alter the expression logic. The operands used will vary depending on the implementation and how you're trying to exploit it. I believe that you would have to sanitize or escape all operands listed at http://symfony.com/doc/current/components/expression_language/syntax.html as well as ', ", etc. Or, maybe just white-list alphanumeric characters. So maybe the recommendation should be "Avoid passing untrusted data into the $values array. If untrusted data must be used within the expression, sanitize non-alphanumeric characters to prevent malicious users from injecting control characters and altering the expression".

Another possibility is to rework the ExpressionLanguage implementation so that it parameterizes the expression and then inserts data from the $values array (similar to a parameterized SQL query), but I'm not sure how feasible this is since it would probably require a bit of work and be a breaking change.

[javiereguiluz]

Closing as fixed by #13688.