Misleading Warning in ExpressionLanguage Documentation

[valepu]

Hi,
I'm writing in reference to this issue: #8259

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.

I replied on the issue because I noticed an error in the regex used as example which led to the belief that variables can alter the expressions used, but I thought it would be best to open a new issue. I'm quoting the relevant part of my answer

The regex "/[a-zA-Z0-9]$/" used in the first post will test if the last character of the string is alphanumeric, which is the case for <script>alert('XSS_SUCCESS!')</script>" > 0 | "a so the expression is returning true because the last character of the string is "a" not because the expression was altered through the variable. You can test it here: https://onlinephp.io?s=s7EvyCjg5eLlSk3OyFcoKEpNj89NLEnO0FDSj07UrXLUjTLQtYxV0VfSUVCyKU4uyiwosUvMSS0q0VCPCA6ODw51dnYNDlZU17TRh8rGKCnYKRgo1CjEKCUqaSrYKyj5ggxMLVZSsFJQcslPLc5TL1EAW6JkDQA%2C&v=7.4.30

I tested this example using this regex "/^[a-zA-Z0-9]+$/" (which will return true only if the whole string is alphanumeric) and this time I got the proper answer (matching <script>alert('XSS_SUCCESS!')</script>" > 0 | "a against "/^[a-zA-Z0-9]+$/" returned false).

I also tried these:

• $expr->evaluate("value", ["value" => '100 - 1']); returned the string "100 - 1"

• $expr->evaluate("value + 1", ["value" => '100 - 1']); gave me error (A non-numeric value encountered)

• $expr->evaluate("value < 0", ["value" => '50 - 100']); returned false

• $expr->evaluate("value > 0", ["value" => '50 - 100']); returned true. But I believe it's converting the string "50 - 100" into "50" (at least that's what intval() returns) just like normal PHP would do

While reminding people to sanitize inputs is always a good thing, the warning written as it is could mislead people into believing the ExpressionLanguage component is highly unsafe (I mean, improper use of the component is always a risk, but not as much as this warning could make you think)

[javiereguiluz]

Closing as fixed in #18897.

[valepu]

To fix this issue this warning should be removed

I didn't do the edit myself because I thought this should get properly investigated on symfony, but from my tests it looks like it's not possible to inject control characters (which makes this warning false)