From 45579edd43823b3b4930526619e303afd3bcef4d Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Sun, 14 Mar 2021 18:57:18 +0100 Subject: [PATCH] Enhance WAF and dol_sanitizeUrl --- htdocs/core/lib/functions.lib.php | 27 ++++++++++++------- htdocs/main.inc.php | 2 +- .../class/productcustomerprice.class.php | 2 +- test/phpunit/SecurityTest.php | 27 ++++++++++++++++--- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index f798d7cbe72..e9e42d586fb 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -613,7 +613,13 @@ function GETPOST($paramname, $check = 'alphanohtml', $method = 0, $filter = null // Sanitizing for special parameters. There is no reason to allow the backtopage parameter to contains an external URL. if ($paramname == 'backtopage' || $paramname == 'backtolist') { $out = str_replace('\\', '/', $out); - $out = str_replace(array(':', '@'), '', $out); + $out = str_replace(array(':', ';', '@'), '', $out); + + do { + $oldstringtoclean = $out; + $out = str_ireplace(array('javascript', 'vbscript', '&colon', ':'), '', $out); + } while ($oldstringtoclean != $out); + $out = preg_replace(array('/^[a-z]*\/\/+/i'), '', $out); } @@ -1010,20 +1016,20 @@ function dol_sanitizePathName($str, $newstr = '_', $unaccent = 1) function dol_sanitizeUrl($stringtoclean, $type = 1) { // We clean string because some hacks try to obfuscate evil strings by inserting non printable chars. Example: 'java(ascci09)scr(ascii00)ipt' is processed like 'javascript' (whatever is place of evil ascii char) - // We should use dol_string_nounprintableascii but function is not yet loaded/available + // We should use dol_string_nounprintableascii but function may not be yet loaded/available $stringtoclean = preg_replace('/[\x00-\x1F\x7F]/u', '', $stringtoclean); // /u operator makes UTF8 valid characters being ignored so are not included into the replace // We clean html comments because some hacks try to obfuscate evil strings by inserting HTML comments. Example: onerror=alert(1) $stringtoclean = preg_replace('//', '', $stringtoclean); $stringtoclean = str_replace('\\', '/', $stringtoclean); - $stringtoclean = str_replace(array(':', '@'), '', $stringtoclean); + if ($type == 1) { + $stringtoclean = str_replace(array(':', ';', '@'), '', $stringtoclean); + } do { $oldstringtoclean = $stringtoclean; - $stringtoclean = str_replace(array('javascript', 'vbscript'), '', $stringtoclean); - if ($type == 1) { - $stringtoclean = str_replace(array(':', '&colon', ';'), '', $stringtoclean); - } + + $stringtoclean = str_ireplace(array('javascript', 'vbscript', '&colon', ':'), '', $stringtoclean); } while ($oldstringtoclean != $stringtoclean); if ($type == 1) { @@ -6240,10 +6246,11 @@ function dol_string_onlythesehtmltags($stringtoclean, $cleanalsosomestyles = 1, $stringtoclean = str_replace('', '__!DOCTYPE_HTML__', $stringtoclean); // Replace DOCTYPE to avoid to have it removed by the strip_tags $stringtoclean = dol_string_nounprintableascii($stringtoclean, 0); - $stringtoclean = preg_replace('/:/i', ':', $stringtoclean); $stringtoclean = preg_replace('//', '', $stringtoclean); - $stringtoclean = preg_replace('/:|:|:/i', '', $stringtoclean); // refused string ':' encoded (no reason to have it encoded) to lock 'javascript:...' + + $stringtoclean = preg_replace('/:/i', ':', $stringtoclean); + $stringtoclean = preg_replace('/:|�+58|:/i', '', $stringtoclean); // refused string ':' encoded (no reason to have a : encoded like this) to disable 'javascript:...' $stringtoclean = preg_replace('/javascript\s*:/i', '', $stringtoclean); $temp = strip_tags($stringtoclean, $allowed_tags_string); @@ -6256,7 +6263,7 @@ function dol_string_onlythesehtmltags($stringtoclean, $cleanalsosomestyles = 1, } // Remove 'javascript:' that we should not find into a text with - // Warning: This is not reliable to fight against obfuscated javascript, there is a lot of other solution to include js into a common html tag (only filtered by the GETPOST). + // Warning: This is not reliable to fight against obfuscated javascript, there is a lot of other solution to include js into a common html tag (only filtered by a GETPOST(.., powerfullfilter)). if ($cleanalsojavascript) { $temp = preg_replace('/javascript\s*:/i', '', $temp); } diff --git a/htdocs/main.inc.php b/htdocs/main.inc.php index ab33d5cdc3a..5b1324aec49 100644 --- a/htdocs/main.inc.php +++ b/htdocs/main.inc.php @@ -68,7 +68,7 @@ function testSqlAndScriptInject($val, $type) $oldval = $val; $val = html_entity_decode($val, ENT_QUOTES | ENT_HTML5); } while ($oldval != $val); - //print "after decoding $val\n"; + //print "after decoding $val\n"; // We clean string because some hacks try to obfuscate evil strings by inserting non printable chars. Example: 'java(ascci09)scr(ascii00)ipt' is processed like 'javascript' (whatever is place of evil ascii char) // We should use dol_string_nounprintableascii but function is not yet loaded/available diff --git a/htdocs/product/class/productcustomerprice.class.php b/htdocs/product/class/productcustomerprice.class.php index 5d395572f5c..046aed05a10 100644 --- a/htdocs/product/class/productcustomerprice.class.php +++ b/htdocs/product/class/productcustomerprice.class.php @@ -455,7 +455,7 @@ class Productcustomerprice extends CommonObject $line->socname = $obj->socname; $line->prodref = $obj->prodref; - $this->lines [] = $line; + $this->lines[] = $line; } $this->db->free($resql); diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 88afc406018..c3855dd9cba 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -207,7 +207,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase $test = 'javascript&colon;alert(1)'; $result=testSqlAndScriptInject($test, 0); - $this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject expected 1b'); + $this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject expected 1a'); $test=""; $result=testSqlAndScriptInject($test, 0); @@ -327,7 +327,8 @@ class SecurityTest extends PHPUnit\Framework\TestCase $_POST["param10"]='is_object($object) ? ($object->id < 10 ? round($object->id / 2, 2) : (2 * $user->id) * (int) substr($mysoc->zip, 1, 2)) : \'objnotdefined\''; $_POST["param11"]=' Name '; $_POST["param12"]='aaa'; - $_POST["param13"]='javascript%26colon%26%23x3B%3Balert(1)'; + //$_POST["param13"]='javascript%26colon%26%23x3B%3Balert(1)'; + //$_POST["param14"]='javascripT&javascript#x3a alert(1)'; $result=GETPOST('id', 'int'); // Must return nothing print __METHOD__." result=".$result."\n"; @@ -436,6 +437,15 @@ class SecurityTest extends PHPUnit\Framework\TestCase print __METHOD__." result=".$result."\n"; $this->assertEquals(trim($_POST["param12"]), $result, 'Test a string with DOCTYPE and restricthtml'); + /*$result=GETPOST("param13", 'alphanohtml'); + print __METHOD__." result=".$result."\n"; + $this->assertEquals(trim($_POST["param13"]), $result, 'Test a string and alphanohtml'); + + $result=GETPOST("param14", 'alphanohtml'); + print __METHOD__." result=".$result."\n"; + $this->assertEquals(trim($_POST["param14"]), $result, 'Test a string and alphanohtml'); + */ + // Special test for GETPOST of backtopage or backtolist parameter $_POST["backtopage"]='//www.google.com'; @@ -463,6 +473,11 @@ class SecurityTest extends PHPUnit\Framework\TestCase print __METHOD__." result=".$result."\n"; $this->assertEquals('/mydir/mypage.php?aa=a%10a', $result, 'Test for backtopage param'); + $_POST["backtopage"]='javascripT&javascript#javascriptxjavascript3a alert(1)'; + $result=GETPOST("backtopage"); + print __METHOD__." result=".$result."\n"; + $this->assertEquals(' alert(1)', $result, 'Test for backtopage param'); + return $result; } @@ -674,13 +689,17 @@ class SecurityTest extends PHPUnit\Framework\TestCase $langs=$this->savlangs; $db=$this->savdb; + $test = 'javascripT&javascript#x3a alert(1)'; + $result=dol_sanitizeUrl($test); + $this->assertEquals(' alert(1)', $result, 'Test on dol_sanitizeUrl A'); + $test = 'javajavascriptscript&cjavascriptolon;alert(1)'; $result=dol_sanitizeUrl($test); - $this->assertEquals('alert(1)', $result, 'Test on dol_sanitizeUrl'); + $this->assertEquals('alert(1)', $result, 'Test on dol_sanitizeUrl B'); $test = '/javas:cript/google.com'; $result=dol_sanitizeUrl($test); - $this->assertEquals('google.com', $result, 'Test on dol_sanitizeUrl'); + $this->assertEquals('google.com', $result, 'Test on dol_sanitizeUrl C'); } /**