Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smarty 4.0.2 Warning: math: illegal characters #702

Closed
ghost opened this issue Jan 10, 2022 · 12 comments · Fixed by #708
Closed

Smarty 4.0.2 Warning: math: illegal characters #702

ghost opened this issue Jan 10, 2022 · 12 comments · Fixed by #708

Comments

@ghost
Copy link

ghost commented Jan 10, 2022

Good morning dear Smarty Team,

after updating to PHP 8.1 i get:
Warning: math: illegal characters in vendor/smarty/smarty/libs/plugins/function.math.php on line 76

Here is some sample code when the warning pops up.

{assign var="x" value="1"}
{assign var="y" value="1"}

{math equation="x+y+1" x=$x y=$y assign="yx"}

Smarty 4.0.2 ( independent from warning testet 3.1.* and 4.0.0)
Php 8.1.1

@IRGC
Copy link

IRGC commented Jan 10, 2022 via email

@ghost
Copy link
Author

ghost commented Jan 10, 2022

Well you go a point there gone close it again sry for disturbing ^^

@ghost ghost closed this as completed Jan 10, 2022
@IRGC
Copy link

IRGC commented Jan 10, 2022 via email

@ghost
Copy link
Author

ghost commented Jan 10, 2022

Gone reopen it again. It isnt an PHP 8.1 error. (had some branching problems where composer didnt downgrade smarty)

The problem is comming from line 69-78 in function.math.php


 // Adapted from https://www.php.net/manual/en/function.eval.php#107377
    $number = '(?:\d+(?:[,.]\d+)?|pi|π)'; // What is a number
    $functionsOrVars = '((?:0x[a-fA-F0-9]+)|([a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*))';
    $operators = '[+\/*\^%-]'; // Allowed math operators
    $regexp = '/^(('.$number.'|'.$functionsOrVars.'|('.$functionsOrVars.'\s*\((?1)+\)|\((?1)+\)))(?:'.$operators.'(?2))?)+$/';

    if (!preg_match($regexp, $equation)) {
        trigger_error("math: illegal characters", E_USER_WARNING);
        return;
    }

the regex does not support 2 mathematical operations without parentheses.

So 2+2+2 wouldnt work but (2+2)+2 would.

Dont know if you want to clearify that as a bug but it breaks some stuff on my side

@ghost ghost reopened this Jan 10, 2022
@ghost ghost changed the title PHP 8.1 Warning: math: illegal characters Smarty 4.0.2 Warning: math: illegal characters Jan 10, 2022
@caugner
Copy link
Contributor

caugner commented Jan 17, 2022

We're also affected by this issue:

<span class="item">{math equation="x - y + 1" x=$smarty.section.i.index y=$nav_start}</span>

It is caused by the following check:

// Remove whitespaces
$equation = preg_replace('/\s+/', '', $equation);
// Adapted from https://www.php.net/manual/en/function.eval.php#107377
$number = '(?:\d+(?:[,.]\d+)?|pi|π)'; // What is a number
$functionsOrVars = '((?:0x[a-fA-F0-9]+)|([a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*))';
$operators = '[+\/*\^%-]'; // Allowed math operators
$regexp = '/^(('.$number.'|'.$functionsOrVars.'|('.$functionsOrVars.'\s*\((?1)+\)|\((?1)+\)))(?:'.$operators.'(?2))?)+$/';
if (!preg_match($regexp, $equation)) {
trigger_error("math: illegal characters", E_USER_WARNING);
return;
}

This check was introduced by @wisskid in 4.0.2 (215d81a) to address CVE-2021-29454.

@caugner
Copy link
Contributor

caugner commented Jan 17, 2022

@Exzelsio A workaround is to replace {math equation} with an assignment:

{* Before *}
{assign var="x" value="1"}
{assign var="y" value="1"}

{math equation="x+y+1" x=$x y=$y assign="yx"}

{* After *}
{$x = 1}
{$y = 1}
{$yx = $x + $y + 1}

@ghost
Copy link
Author

ghost commented Jan 17, 2022

@caugner
Tanke for your answer, I have also thought about it. However, I/we use the function around 800-900 times throughout our system and since the source code is already over 15 years old, a simple fix of the function would be better from my point of view. But anyway thanks for your contribution :D

@caugner
Copy link
Contributor

caugner commented Jan 17, 2022

@Exzelsio To make sure #708 fixes all your cases, would you be able to share other equation patterns you may use in your code base apart from x+y+1 (i.e. two addition operators)?

E.g. you could run grep --include '*.tpl' -hoRP "(?<=equation=[\"'])([^\"']*)" . | sort -u

@ghost
Copy link
Author

ghost commented Jan 17, 2022

I took some time and inspected all our cases, most would not break. (There were about 20-30 that would break, but I fixed them temporarily). Besides using assigns you can also just use lots of brackets (if you want to stay true to math equalation).

X+Z+Y = (X+Z)+y

You just have to be careful that the equation still works correctly.

wisskid pushed a commit that referenced this issue Jan 17, 2022
wisskid pushed a commit that referenced this issue Jan 17, 2022
* fix(math): fix equation regexp

Fixes #702.
wisskid added a commit that referenced this issue Jan 17, 2022
* fix(math): fix equation regexp

Fixes #702.
@ghost
Copy link
Author

ghost commented Jan 19, 2022

@wisskid
The Comit fixed and qualation like 2 -- 1.
But not something like 2 + 2 + 2. So The bug is still open

@caugner
Copy link
Contributor

caugner commented Jan 20, 2022

@Exzelsio I added your example as a test case locally, and it does not fail:

    public function testMultipleAdditions()
    {
        $this->smarty->disableSecurity();
        $expected = "6";
        $tpl = $this->smarty->createTemplate('eval:{math equation="2 + 2 + 2"}');
        $this->assertEquals($expected, $this->smarty->fetch($tpl));
    }

Please upgrade to Smarty 4.0.4 and test again.

@jvega
Copy link

jvega commented Feb 18, 2022

Hi, I got warnings for the next equations, these equations works fine before the sandbox change

{equation="x*-1" x="120.00000"}

{equation="-x*y" x="120.00000" y="1"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants