ezyang / htmlpurifier

Standards compliant HTML filter written in PHP
http://htmlpurifier.org
GNU Lesser General Public License v2.1
3.02k stars 323 forks source link

Unchecked input in HTMLPurifier_UnitConverter round() function #386

Open netdreamer opened 8 months ago

netdreamer commented 8 months ago

Hello, I found an issue in the variable checking in round() function of the HTMLPurifier_UnitConverter.

If, for some reason, round() is called with an invalid value, it crashes. There is no check that the passed value $n is really a number, before trying to do abs($n).

    private function round($n, $sigfigs)
    {
               $new_log = (int)floor(log(abs($n), 10)); // Number of digits left of decimal - 1
    ...

At the moment, I temporary fixed my issue by patching the function with a check:

    private function round($n, $sigfigs)
    {
        if (!is_numeric($n)) { return $n; }
        $new_log = (int)floor(log(abs($n), 10)); // Number of digits left of decimal - 1
    ...

This is the stack trace of the call that generated the issue:

PHP Fatal error:  Uncaught TypeError: abs(): Argument #1 ($num) must be of type int|float, string given in /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/UnitConverter.php:264
Stack trace:
#0 /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/UnitConverter.php(264): abs()
#1 /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/UnitConverter.php(178): HTMLPurifier_UnitConverter->round()
#2 /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/Length.php(153): HTMLPurifier_UnitConverter->convert()
#3 /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/AttrDef/CSS/Length.php(65): HTMLPurifier_Length->compareTo()
#4 /web/admin/vendor/ezyang/htmlpurifier/library/HTMLPurifier/AttrDef/CSS/Composite.php(39): HTMLPurifier_AttrDef_CSS_Length->validate()
ezyang commented 8 months ago

drat! send a PR?

netdreamer commented 8 months ago

I'm preparing it, but It's a bit wider than expected: I found another similar issue (Length.php line 119). $log = (int)floor(log(abs($n), 10));

$n is by definition a STRING, because it's the return value of $length->getN(), that returns a string...

So, every time you use it with as a parameter of an arithmetic function, it must be checked and/or converted to a number before calling abs() or similar functions.

netdreamer commented 8 months ago

Sorry, I'm not very used to fixing code... I found that issue was already fixed in master: https://github.com/ezyang/htmlpurifier/commit/43f49ac9a51b81dfd07d3bc8dcfc5ec5637a5e3b But there are no releases with it.