From c4722e36940eec65ba0b6df5d6fdaf211fd9b69a Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Wed, 10 May 2017 11:47:34 +0200 Subject: [PATCH 1/4] FIX XSS --- htdocs/core/lib/functions.lib.php | 8 +++++--- htdocs/index.php | 8 ++++---- htdocs/langs/en_US/agenda.lang | 1 + test/phpunit/SecurityTest.php | 26 +++++++++++++++++++++++++- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index e0beb3a8a8e..6b0b3112307 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -302,7 +302,7 @@ function GETPOST($paramname,$check='',$method=0,$filter=NULL,$options=NULL) break; case 'aZ09': $out=trim($out); - if (preg_match('/[^a-z0-9_]+/i',$out)) $out=''; + if (preg_match('/[^a-z0-9_\-]+/i',$out)) $out=''; break; case 'array': if (! is_array($out) || empty($out)) $out=array(); @@ -2937,7 +2937,7 @@ function dol_print_error($db='',$error='',$errors=null) if ($_SERVER['DOCUMENT_ROOT']) // Mode web { $out.="".$langs->trans("DatabaseTypeManager").": ".$db->type."
\n"; - $out.="".$langs->trans("RequestLastAccessInError").": ".($db->lastqueryerror()?$db->lastqueryerror():$langs->trans("ErrorNoRequestInError"))."
\n"; + $out.="".$langs->trans("RequestLastAccessInError").": ".($db->lastqueryerror()?dol_escape_htmltag($db->lastqueryerror()):$langs->trans("ErrorNoRequestInError"))."
\n"; $out.="".$langs->trans("ReturnCodeLastAccessInError").": ".($db->lasterrno()?$db->lasterrno():$langs->trans("ErrorNoRequestInError"))."
\n"; $out.="".$langs->trans("InformationLastAccessInError").": ".($db->lasterror()?$db->lasterror():$langs->trans("ErrorNoRequestInError"))."
\n"; $out.="
\n"; @@ -2945,7 +2945,9 @@ function dol_print_error($db='',$error='',$errors=null) else // Mode CLI { $out.='> '.$langs->transnoentities("DatabaseTypeManager").":\n".$db->type."\n"; - $out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?$db->lastqueryerror():$langs->trans("ErrorNoRequestInError"))."\n"; + $out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?dol_escape_htmltag($db->lastqueryerror()):$langs->trans("ErrorNoRequestInError"))."\n"; + // To make detection of xss vulnerabilities or sql injection easier with a scanner, replace line with this one: + //$out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?$db->lastqueryerror:$langs->trans("ErrorNoRequestInError"))."\n"; $out.='> '.$langs->transnoentities("ReturnCodeLastAccessInError").":\n".($db->lasterrno()?$db->lasterrno():$langs->trans("ErrorNoRequestInError"))."\n"; $out.='> '.$langs->transnoentities("InformationLastAccessInError").":\n".($db->lasterror()?$db->lasterror():$langs->trans("ErrorNoRequestInError"))."\n"; diff --git a/htdocs/index.php b/htdocs/index.php index 5430fcf7d24..007145083cf 100644 --- a/htdocs/index.php +++ b/htdocs/index.php @@ -55,10 +55,10 @@ if (count($conf->modules) <= (empty($conf->global->MAIN_MIN_NB_ENABLED_MODULE_FO if (GETPOST('addbox')) // Add box (when submit is done from a form when ajax disabled) { require_once DOL_DOCUMENT_ROOT.'/core/class/infobox.class.php'; - $zone=GETPOST('areacode'); - $userid=GETPOST('userid'); - $boxorder=GETPOST('boxorder'); - $boxorder.=GETPOST('boxcombo'); + $zone=GETPOST('areacode', 'aZ09'); + $userid=GETPOST('userid', 'int'); + $boxorder=GETPOST('boxorder', 'aZ09'); + $boxorder.=GETPOST('boxcombo', 'aZ09'); $result=InfoBox::saveboxorder($db,$zone,$boxorder,$userid); } diff --git a/htdocs/langs/en_US/agenda.lang b/htdocs/langs/en_US/agenda.lang index d0f3456987d..6bfa9fd0406 100644 --- a/htdocs/langs/en_US/agenda.lang +++ b/htdocs/langs/en_US/agenda.lang @@ -75,6 +75,7 @@ ProposalDeleted=Proposal deleted OrderDeleted=Order deleted InvoiceDeleted=Invoice deleted ##### End agenda events ##### +AgendaModelModule=Document templates for event DateActionStart=Start date DateActionEnd=End date AgendaUrlOptions1=You can also add following parameters to filter output: diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 00714e7bfab..288a15c317e 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -147,7 +147,9 @@ class SecurityTest extends PHPUnit_Framework_TestCase $_GET["param2"]='a/b#e(pr)qq-rr\cc'; $_GET["param3"]='"a/b#e(pr)qq-rr\cc'; // Same than param2 + " $_GET["param4"]='../dir'; - + $_GET["param5"]="a_1-b"; + + // Test int $result=GETPOST('id','int'); // Must return nothing print __METHOD__." result=".$result."\n"; $this->assertEquals($result,''); @@ -160,6 +162,7 @@ class SecurityTest extends PHPUnit_Framework_TestCase print __METHOD__." result=".$result."\n"; $this->assertEquals($result,333); + // Test alpha $result=GETPOST("param2",'alpha'); print __METHOD__." result=".$result."\n"; $this->assertEquals($result,$_GET["param2"]); @@ -172,6 +175,27 @@ class SecurityTest extends PHPUnit_Framework_TestCase print __METHOD__." result=".$result."\n"; $this->assertEquals($result,''); + // Test aZ09 + $result=GETPOST("param1",'aZ09'); // Must return '' as there is a forbidden char ../ + print __METHOD__." result=".$result."\n"; + $this->assertEquals($result,$_GET["param1"]); + + $result=GETPOST("param2",'aZ09'); // Must return '' as there is a forbidden char ../ + print __METHOD__." result=".$result."\n"; + $this->assertEquals($result,''); + + $result=GETPOST("param3",'aZ09'); // Must return '' as there is a forbidden char ../ + print __METHOD__." result=".$result."\n"; + $this->assertEquals($result,''); + + $result=GETPOST("param4",'aZ09'); // Must return '' as there is a forbidden char ../ + print __METHOD__." result=".$result."\n"; + $this->assertEquals($result,''); + + $result=GETPOST("param5",'aZ09'); + print __METHOD__." result=".$result."\n"; + $this->assertEquals($result,$_GET["param5"]); + return $result; } From 383dfc0c96c0c4e77110d87f91f09a8f5bf7b4da Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Wed, 10 May 2017 11:51:59 +0200 Subject: [PATCH 2/4] FIX Pagination of invoices --- htdocs/compta/facture/list.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/compta/facture/list.php b/htdocs/compta/facture/list.php index cce94646616..693a6f39f97 100644 --- a/htdocs/compta/facture/list.php +++ b/htdocs/compta/facture/list.php @@ -409,7 +409,7 @@ if (empty($conf->global->MAIN_DISABLE_FULL_SCANLIST)) $nbtotalofrecords = $db->num_rows($result); } -$sql.= $db->plimit($limit,$offset); +$sql.= $db->plimit($limit+1,$offset); //print $sql; $resql = $db->query($sql); From 3e0fcf5ec2ecda622736e8d5142e31c592167af2 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Wed, 10 May 2017 11:57:59 +0200 Subject: [PATCH 3/4] Fix label "total for this page" on last page. --- dev/skeletons/skeleton_list.php | 2 +- htdocs/comm/propal/list.php | 2 +- htdocs/commande/list.php | 2 +- htdocs/compta/bank/bankentries.php | 2 +- htdocs/compta/bank/index.php | 2 +- htdocs/compta/facture/list.php | 2 +- htdocs/compta/sociales/index.php | 2 +- htdocs/expensereport/list.php | 2 +- htdocs/fichinter/list.php | 2 +- htdocs/fourn/facture/list.php | 2 +- htdocs/product/stock/productlot_list.php | 2 +- htdocs/projet/list.php | 2 +- htdocs/projet/tasks/list.php | 2 +- htdocs/projet/tasks/time.php | 2 +- 14 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dev/skeletons/skeleton_list.php b/dev/skeletons/skeleton_list.php index 77485c6d638..4453f2d8302 100644 --- a/dev/skeletons/skeleton_list.php +++ b/dev/skeletons/skeleton_list.php @@ -523,7 +523,7 @@ if (isset($totalarray['totalhtfield'])) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/comm/propal/list.php b/htdocs/comm/propal/list.php index d85448397de..efb72afb7b7 100644 --- a/htdocs/comm/propal/list.php +++ b/htdocs/comm/propal/list.php @@ -991,7 +991,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/commande/list.php b/htdocs/commande/list.php index 80601ec9727..e62ae11d5cd 100644 --- a/htdocs/commande/list.php +++ b/htdocs/commande/list.php @@ -1412,7 +1412,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/compta/bank/bankentries.php b/htdocs/compta/bank/bankentries.php index 72e82dd0dd7..4845012a3e0 100644 --- a/htdocs/compta/bank/bankentries.php +++ b/htdocs/compta/bank/bankentries.php @@ -1307,7 +1307,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totaldebfield'] == $i) print ''.price(-1 * $totalarray['totaldeb']).''; diff --git a/htdocs/compta/bank/index.php b/htdocs/compta/bank/index.php index 012dfe42175..5f6e30016d4 100644 --- a/htdocs/compta/bank/index.php +++ b/htdocs/compta/bank/index.php @@ -607,7 +607,7 @@ if (isset($totalarray['totalbalancefield']) && $lastcurrencycode != 'various') / $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalbalancefield'] == $i) print ''.price($totalarray['totalbalance'], 0, $langs, 0, 0, -1, $lastcurrencycode).''; diff --git a/htdocs/compta/facture/list.php b/htdocs/compta/facture/list.php index 693a6f39f97..c5fd9b80482 100644 --- a/htdocs/compta/facture/list.php +++ b/htdocs/compta/facture/list.php @@ -1117,7 +1117,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/compta/sociales/index.php b/htdocs/compta/sociales/index.php index 66d6a65dd56..e3f4a0b9b4d 100644 --- a/htdocs/compta/sociales/index.php +++ b/htdocs/compta/sociales/index.php @@ -274,7 +274,7 @@ if ($resql) if (isset($totalarray['totalttcfield'])) { print ''; - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; print ''; print ''; diff --git a/htdocs/expensereport/list.php b/htdocs/expensereport/list.php index 58541330c83..ca7a52ad293 100644 --- a/htdocs/expensereport/list.php +++ b/htdocs/expensereport/list.php @@ -694,7 +694,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/fichinter/list.php b/htdocs/fichinter/list.php index 9ef7d22ad10..bafafb40dd1 100644 --- a/htdocs/fichinter/list.php +++ b/htdocs/fichinter/list.php @@ -518,7 +518,7 @@ if ($result) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totaldurationfield'] == $i) print ''.convertSecondToTime($totalarray['totalduration'], 'allhourmin').''; diff --git a/htdocs/fourn/facture/list.php b/htdocs/fourn/facture/list.php index 396d5ffd8a5..77064191d24 100644 --- a/htdocs/fourn/facture/list.php +++ b/htdocs/fourn/facture/list.php @@ -970,7 +970,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/product/stock/productlot_list.php b/htdocs/product/stock/productlot_list.php index 2910103e380..3d89aee0013 100644 --- a/htdocs/product/stock/productlot_list.php +++ b/htdocs/product/stock/productlot_list.php @@ -570,7 +570,7 @@ if ($resql) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalhtfield'] == $i) print ''.price($totalarray['totalht']).''; diff --git a/htdocs/projet/list.php b/htdocs/projet/list.php index 51f260cac6f..408031a9ed2 100644 --- a/htdocs/projet/list.php +++ b/htdocs/projet/list.php @@ -854,7 +854,7 @@ if (isset($totalarray['totaloppfield']) || isset($totalarray['totalbudgetfield'] $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totaloppfield'] == $i) print ''.price($totalarray['totalopp']).''; diff --git a/htdocs/projet/tasks/list.php b/htdocs/projet/tasks/list.php index 30a89109115..482c7b6f446 100644 --- a/htdocs/projet/tasks/list.php +++ b/htdocs/projet/tasks/list.php @@ -811,7 +811,7 @@ if (isset($totalarray['totaldurationeffectivefield']) || isset($totalarray['tota $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totalplannedworkloadfield'] == $i) print ''.convertSecondToTime($totalarray['totalplannedworkload'],$plannedworkloadoutputformat).''; diff --git a/htdocs/projet/tasks/time.php b/htdocs/projet/tasks/time.php index 6cb52588a89..4c5d58dab7d 100644 --- a/htdocs/projet/tasks/time.php +++ b/htdocs/projet/tasks/time.php @@ -971,7 +971,7 @@ if (($id > 0 || ! empty($ref)) || $projectidforalltimes > 0) $i++; if ($i == 1) { - if ($num < $limit) print ''.$langs->trans("Total").''; + if ($num < $limit && empty($offset)) print ''.$langs->trans("Total").''; else print ''.$langs->trans("Totalforthispage").''; } elseif ($totalarray['totaldurationfield'] == $i) print ''.convertSecondToTime($totalarray['totalduration'],'allhourmin').''; From fe053c86037d31a4592385e41fd5df46acb1603d Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Wed, 10 May 2017 12:02:41 +0200 Subject: [PATCH 4/4] Better escaping of error message. --- htdocs/core/lib/functions.lib.php | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index 6b0b3112307..facc685d234 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -2938,18 +2938,17 @@ function dol_print_error($db='',$error='',$errors=null) { $out.="".$langs->trans("DatabaseTypeManager").": ".$db->type."
\n"; $out.="".$langs->trans("RequestLastAccessInError").": ".($db->lastqueryerror()?dol_escape_htmltag($db->lastqueryerror()):$langs->trans("ErrorNoRequestInError"))."
\n"; - $out.="".$langs->trans("ReturnCodeLastAccessInError").": ".($db->lasterrno()?$db->lasterrno():$langs->trans("ErrorNoRequestInError"))."
\n"; - $out.="".$langs->trans("InformationLastAccessInError").": ".($db->lasterror()?$db->lasterror():$langs->trans("ErrorNoRequestInError"))."
\n"; + $out.="".$langs->trans("ReturnCodeLastAccessInError").": ".($db->lasterrno()?dol_escape_htmltag($db->lasterrno()):$langs->trans("ErrorNoRequestInError"))."
\n"; + $out.="".$langs->trans("InformationLastAccessInError").": ".($db->lasterror()?dol_escape_htmltag($db->lasterror()):$langs->trans("ErrorNoRequestInError"))."
\n"; $out.="
\n"; } else // Mode CLI { - $out.='> '.$langs->transnoentities("DatabaseTypeManager").":\n".$db->type."\n"; - $out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?dol_escape_htmltag($db->lastqueryerror()):$langs->trans("ErrorNoRequestInError"))."\n"; - // To make detection of xss vulnerabilities or sql injection easier with a scanner, replace line with this one: - //$out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?$db->lastqueryerror:$langs->trans("ErrorNoRequestInError"))."\n"; - $out.='> '.$langs->transnoentities("ReturnCodeLastAccessInError").":\n".($db->lasterrno()?$db->lasterrno():$langs->trans("ErrorNoRequestInError"))."\n"; - $out.='> '.$langs->transnoentities("InformationLastAccessInError").":\n".($db->lasterror()?$db->lasterror():$langs->trans("ErrorNoRequestInError"))."\n"; + // No dol_escape_htmltag for output, we are in CLI mode + $out.='> '.$langs->transnoentities("DatabaseTypeManager").":\n".$db->type."\n"; + $out.='> '.$langs->transnoentities("RequestLastAccessInError").":\n".($db->lastqueryerror()?$db->lastqueryerror():$langs->transnoentities("ErrorNoRequestInError"))."\n"; + $out.='> '.$langs->transnoentities("ReturnCodeLastAccessInError").":\n".($db->lasterrno()?$db->lasterrno():$langs->transnoentities("ErrorNoRequestInError"))."\n"; + $out.='> '.$langs->transnoentities("InformationLastAccessInError").":\n".($db->lasterror()?$db->lasterror():$langs->transnoentities("ErrorNoRequestInError"))."\n"; } $syslog.=", sql=".$db->lastquery();