Skip to content

Commit

Permalink
FIX Security fixes (filter onload js, less verbose error message in
Browse files Browse the repository at this point in the history 
download and viewimage, show info to encourage dolibarr_main_prod=1)
  • Loading branch information
@eldy
eldy committed Sep 6, 2017
1 parent 9cc344f commit d26b2a6
Show file tree
Hide file tree
Showing 7 changed files with 215 additions and 211 deletions.
216 changes: 108 additions & 108 deletions htdocs/admin/company.php
View file

Large diffs are not rendered by default.

103 changes: 51 additions & 52 deletions htdocs/admin/menus/edit.php
View file
Expand Up @@ -83,27 +83,27 @@
}
}
}

$menu = new Menubase($db);
$result=$menu->fetch($_POST['menuId']);
$result=$menu->fetch(GETPOST('menuId', 'int'));
if ($result > 0)
{
$menu->titre=$_POST['titre'];
$menu->leftmenu=$_POST['leftmenu'];
$menu->url=$_POST['url'];
$menu->langs=$_POST['langs'];
$menu->position=$_POST['position'];
$menu->enabled=$_POST['enabled'];
$menu->perms=$_POST['perms'];
$menu->target=$_POST['target'];
$menu->user=$_POST['user'];
if (is_numeric($_POST['menuIdParent']))
$menu->titre=GETPOST('titre', 'alpha');
$menu->leftmenu=GETPOST('leftmenu', 'alpha');
$menu->url=GETPOST('url','alpha');
$menu->langs=GETPOST('langs','alpha');
$menu->position=GETPOST('position','int');
$menu->enabled=GETPOST('enabled','alpha');
$menu->perms=GETPOST('perms','alpha');
$menu->target=GETPOST('target','alpha');
$menu->user=GETPOST('user','alpha');
if (is_numeric(GETPOST('menuIdParent','alpha')))
{
$menu->fk_menu=$_POST['menuIdParent'];
$menu->fk_menu=GETPOST('menuIdParent','alpha');
}
else
{
if ($_POST['type'] == 'top') $menu->fk_menu=0;
if (GETPOST('type','alpha') == 'top') $menu->fk_menu=0;
else $menu->fk_menu=-1;
$menu->fk_mainmenu=$mainmenu;
$menu->fk_leftmenu=$leftmenu;
Expand All @@ -123,7 +123,6 @@
{
setEventMessages($menu->error, $menu->errors, 'errors');
}
$_GET["menuId"] = $_POST['menuId'];
$action = "edit";
}
else
Expand All @@ -148,9 +147,9 @@
}

$leftmenu=''; $mainmenu='';
if (! empty($_POST['menuId']) && ! is_numeric($_POST['menuId']))
if (GETPOST('menuId','int') && ! is_numeric(GETPOST('menuId','int')))
{
$tmp=explode('&',$_POST['menuId']);
$tmp=explode('&',GETPOST('menuId','int'));
foreach($tmp as $s)
{
if (preg_match('/fk_mainmenu=/',$s))
Expand Down Expand Up @@ -197,7 +196,7 @@
$action = 'create';
$error++;
}
if (! $error && empty($_POST['menuId']) && $_POST['type'] == 'left')
if (! $error && ! $_POST['menuId'] && $_POST['type'] == 'left')
{
setEventMessages($langs->trans("ErrorLeftMenuMustHaveAParentId"), null, 'errors');
$action = 'create';
Expand All @@ -207,23 +206,23 @@
if (! $error)
{
$menu = new Menubase($db);
$menu->menu_handler=preg_replace('/_menu$/','',$_POST['menu_handler']);
$menu->type=$_POST['type'];
$menu->titre=$_POST['titre'];
$menu->url=$_POST['url'];
$menu->langs=$_POST['langs'];
$menu->position=$_POST['position'];
$menu->enabled=$_POST['enabled'];
$menu->perms=$_POST['perms'];
$menu->target=$_POST['target'];
$menu->user=$_POST['user'];
if (is_numeric($_POST['menuId']))
$menu->menu_handler=preg_replace('/_menu$/','',GETPOST('menu_handler','aZ09'));
$menu->type=GETPOST('type','alpha');
$menu->titre=GETPOST('titre','alpha');
$menu->url=GETPOST('url','alpha');
$menu->langs=GETPOST('langs','alpha');
$menu->position=GETPOST('position','int');
$menu->enabled=GETPOST('enabled','alpha');
$menu->perms=GETPOST('perms','alpha');
$menu->target=GETPOST('target','alpha');
$menu->user=GETPOST('user','alpha');
if (is_numeric(GETPOST('menuId','int')))
{
$menu->fk_menu=$_POST['menuId'];
$menu->fk_menu=GETPOST('menuId','int');
}
else
{
if ($_POST['type'] == 'top') $menu->fk_menu=0;
if (GETPOST('type','alpha') == 'top') $menu->fk_menu=0;
else $menu->fk_menu=-1;
$menu->fk_mainmenu=$mainmenu;
$menu->fk_leftmenu=$leftmenu;
Expand All @@ -232,7 +231,7 @@
$result=$menu->create($user);
if ($result > 0)
{
header("Location: ".DOL_URL_ROOT."/admin/menus/index.php?menu_handler=".$_POST['menu_handler']);
header("Location: ".DOL_URL_ROOT."/admin/menus/index.php?menu_handler=".GETPOST('menu_handler','aZ09'));
exit;
}
else
Expand Down Expand Up @@ -305,17 +304,17 @@ function init_topleft()
</script>';

print load_fiche_titre($langs->trans("NewMenu"),'','title_setup');
print '<form action="./edit.php?action=add&menuId='.$_GET['menuId'].'" method="post" name="formmenucreate">';

print '<form action="./edit.php?action=add&menuId='.GETPOST('menuId', 'int').'" method="post" name="formmenucreate">';
print '<input type="hidden" name="token" value="'.$_SESSION['newtoken'].'">';

dol_fiche_head();

print '<table class="border" width="100%">';

// Id
$parent_rowid = $_GET['menuId'];
if ($_GET['menuId'])
$parent_rowid = GETPOST('menuId', 'int');
if (GETPOST('menuId', 'int'))
{
$sql = "SELECT m.rowid, m.mainmenu, m.leftmenu, m.level, m.langs FROM ".MAIN_DB_PREFIX."menu as m WHERE m.rowid = ".GETPOST('menuId', 'int');
$res = $db->query($sql);
Expand Down Expand Up @@ -375,40 +374,40 @@ function init_topleft()
}
else
{
print '<td><input type="text" size="48" id="menuId" name="menuId" value="'.($_POST["menuId"]?$_POST["menuId"]:'').'"></td>';
print '<td><input type="text" size="48" id="menuId" name="menuId" value="'.(GETPOST("menuId", 'int')?GETPOST("menuId", 'int'):'').'"></td>';
}
print '<td>'.$langs->trans('DetailMenuIdParent');
print ', '.$langs->trans("Example").': fk_mainmenu=abc&fk_leftmenu=def';
print '</td></tr>';

// Title
print '<tr><td class="fieldrequired">'.$langs->trans('Title').'</td><td><input type="text" size="30" name="titre" value="'.$_POST["titre"].'"></td><td>'.$langs->trans('DetailTitre').'</td></tr>';
print '<tr><td class="fieldrequired">'.$langs->trans('Title').'</td><td><input type="text" size="30" name="titre" value="'.dol_escape_htmltag(GETPOST("titre",'alpha')).'"></td><td>'.$langs->trans('DetailTitre').'</td></tr>';

// URL
print '<tr><td class="fieldrequired">'.$langs->trans('URL').'</td><td><input type="text" size="60" name="url" value="'.$_POST["url"].'"></td><td>'.$langs->trans('DetailUrl').'</td></tr>';
print '<tr><td class="fieldrequired">'.$langs->trans('URL').'</td><td><input type="text" size="60" name="url" value="'.GETPOST("url",'alpha').'"></td><td>'.$langs->trans('DetailUrl').'</td></tr>';

// Langs
print '<tr><td>'.$langs->trans('LangFile').'</td><td><input type="text" size="30" name="langs" value="'.$parent_langs.'"></td><td>'.$langs->trans('DetailLangs').'</td></tr>';

// Position
print '<tr><td>'.$langs->trans('Position').'</td><td><input type="text" size="5" name="position" value="'.(isset($_POST["position"])?$_POST["position"]:100).'"></td><td>'.$langs->trans('DetailPosition').'</td></tr>';
print '<tr><td>'.$langs->trans('Position').'</td><td><input type="text" size="5" name="position" value="'.dol_escape_htmltag(isset($_POST["position"])?$_POST["position"]:100).'"></td><td>'.$langs->trans('DetailPosition').'</td></tr>';

// Target
print '<tr><td>'.$langs->trans('Target').'</td><td><select class="flat" name="target">';
print '<option value=""'.($menu->target==""?' selected':'').'>'.$langs->trans('').'</option>';
print '<option value=""'.($menu->target==""?' selected':'').'>&nbsp;</option>';
print '<option value="_blank"'.($menu->target=="_blank"?' selected':'').'>'.$langs->trans('_blank').'</option>';
print '</select></td></td><td>'.$langs->trans('DetailTarget').'</td></tr>';

// Enabled
print '<tr><td>'.$langs->trans('Enabled').'</td><td><input type="text" size="60" name="enabled" value="'.$_POST["enabled"].'"></td><td>'.$langs->trans('DetailEnabled').'</td></tr>';
print '<tr><td>'.$langs->trans('Enabled').'</td><td><input type="text" size="60" name="enabled" value="'.GETPOST("enabled",'alpha').'"></td><td>'.$langs->trans('DetailEnabled').'</td></tr>';

// Perms
print '<tr><td>'.$langs->trans('Rights').'</td><td><input type="text" size="60" name="perms" value="'.$_POST["perms"].'"></td><td>'.$langs->trans('DetailRight').'</td></tr>';
print '<tr><td>'.$langs->trans('Rights').'</td><td><input type="text" size="60" name="perms" value="'.GETPOST('perms','alpha').'"></td><td>'.$langs->trans('DetailRight').'</td></tr>';

print '</table>';

dol_fiche_end();

// Boutons
print '<div class="center">';
print '<input type="submit" class="button" name="save" value="'.$langs->trans("Save").'">';
Expand All @@ -426,14 +425,14 @@ function init_topleft()
print '<form action="./edit.php?action=update" method="POST" name="formmenuedit">';
print '<input type="hidden" name="token" value="'.$_SESSION['newtoken'].'">';
print '<input type="hidden" name="handler_origine" value="'.$menu_handler.'">';
print '<input type="hidden" name="menuId" value="'.$_GET['menuId'].'">';
print '<input type="hidden" name="menuId" value="'.GETPOST('menuId', 'int').'">';

dol_fiche_head();

print '<table class="border" width="100%">';

$menu = new Menubase($db);
$result=$menu->fetch($_GET['menuId']);
$result=$menu->fetch(GETPOST('menuId', 'int'));
//var_dump($menu);

// Id
Expand Down Expand Up @@ -472,20 +471,20 @@ function init_topleft()
//print '<tr><td>'.$langs->trans('Level').'</td><td>'.$menu->level.'</td><td>'.$langs->trans('DetailLevel').'</td></tr>';

// Title
print '<tr><td class="fieldrequired">'.$langs->trans('Title').'</td><td><input type="text" size="30" name="titre" value="'.$menu->titre.'"></td><td>'.$langs->trans('DetailTitre').'</td></tr>';
print '<tr><td class="fieldrequired">'.$langs->trans('Title').'</td><td><input type="text" size="30" name="titre" value="'.dol_escape_htmltag($menu->titre).'"></td><td>'.$langs->trans('DetailTitre').'</td></tr>';

// Url
print '<tr><td class="fieldrequired">'.$langs->trans('URL').'</td><td><input type="text" class="quatrevingtpercent" name="url" value="'.$menu->url.'"></td><td>'.$langs->trans('DetailUrl').'</td></tr>';

// Langs
print '<tr><td>'.$langs->trans('LangFile').'</td><td><input type="text" size="30" name="langs" value="'.$menu->langs.'"></td><td>'.$langs->trans('DetailLangs').'</td></tr>';
print '<tr><td>'.$langs->trans('LangFile').'</td><td><input type="text" size="30" name="langs" value="'.dol_escape_htmltag($menu->langs).'"></td><td>'.$langs->trans('DetailLangs').'</td></tr>';

// Position
print '<tr><td>'.$langs->trans('Position').'</td><td><input type="text" size="5" name="position" value="'.$menu->position.'"></td><td>'.$langs->trans('DetailPosition').'</td></tr>';

// Target
print '<tr><td>'.$langs->trans('Target').'</td><td><select class="flat" name="target">';
print '<option value=""'.($menu->target==""?' selected':'').'>'.$langs->trans('').'</option>';
print '<option value=""'.($menu->target==""?' selected':'').'>&nbsp;</option>';
print '<option value="_blank"'.($menu->target=="_blank"?' selected':'').'>'.$langs->trans('_blank').'</option>';
print '</select></td><td>'.$langs->trans('DetailTarget').'</td></tr>';

Expand All @@ -502,7 +501,7 @@ function init_topleft()
print '</table>';

dol_fiche_end();

// Bouton
print '<div class="center">';
print '<input type="submit" class="button" name="save" value="'.$langs->trans("Save").'">';
Expand Down
7 changes: 6 additions & 1 deletion htdocs/core/lib/functions.lib.php
View file
Expand Up @@ -3251,7 +3251,12 @@ function dol_print_error($db='',$error='',$errors=null)
}

if (empty($dolibarr_main_prod)) print $out;
else define("MAIN_CORE_ERROR", 1);
else
{
print $langs->trans("DolibarrHasDetectedError").'. ';
print $langs->trans("YouCanSetOptionDolibarrMainProdToZero");
define("MAIN_CORE_ERROR", 1);
}
//else print 'Sorry, an error occured but the parameter $dolibarr_main_prod is defined in conf file so no message is reported to your browser. Please read the log file for error message.';
dol_syslog("Error ".$syslog, LOG_ERR);
}
Expand Down
29 changes: 14 additions & 15 deletions htdocs/document.php
View file
Expand Up @@ -110,7 +110,7 @@ function llxFooter() { }
$check_access = dol_check_secure_access_document($modulepart, $original_file, $entity, $refname);
$accessallowed = $check_access['accessallowed'];
$sqlprotectagainstexternals = $check_access['sqlprotectagainstexternals'];
$original_file = $check_access['original_file']; // original_file is now a full path name
$fullpath_original_file = $check_access['original_file']; // $fullpath_original_file is now a full path name

// Basic protection (against external users only)
if ($user->societe_id > 0)
Expand All @@ -137,36 +137,35 @@ function llxFooter() { }
}

// Security:
// Limite acces si droits non corrects
// Limit access if permissions are wrong
if (! $accessallowed)
{
accessforbidden();
}

// Security:
// On interdit les remontees de repertoire ainsi que les pipe dans
// les noms de fichiers.
if (preg_match('/\.\./',$original_file) || preg_match('/[<>|]/',$original_file))
// On interdit les remontees de repertoire ainsi que les pipe dans les noms de fichiers.
if (preg_match('/\.\./',$fullpath_original_file) || preg_match('/[<>|]/',$fullpath_original_file))
{
dol_syslog("Refused to deliver file ".$original_file);
$file=basename($original_file); // Do no show plain path of original_file in shown error message
dol_print_error(0,$langs->trans("ErrorFileNameInvalid",$file));
dol_syslog("Refused to deliver file ".$fullpath_original_file);
print "ErrorFileNameInvalid: ".$original_file;
exit;
}


clearstatcache();

$filename = basename($original_file);
$filename = basename($fullpath_original_file);

// Output file on browser
dol_syslog("document.php download $original_file $filename content-type=$type");
$original_file_osencoded=dol_osencode($original_file); // New file name encoded in OS encoding charset
dol_syslog("document.php download $fullpath_original_file filename=$filename content-type=$type");
$fullpath_original_file_osencoded=dol_osencode($fullpath_original_file); // New file name encoded in OS encoding charset

// This test if file exists should be useless. We keep it to find bug more easily
if (! file_exists($original_file_osencoded))
if (! file_exists($fullpath_original_file_osencoded))
{
dol_print_error(0,$langs->trans("ErrorFileDoesNotExists",$original_file));
dol_syslog("ErrorFileDoesNotExists: ".$fullpath_original_file);
print "ErrorFileDoesNotExists: ".$original_file;
exit;
}

Expand All @@ -177,14 +176,14 @@ function llxFooter() { }
// Add MIME Content-Disposition from RFC 2183 (inline=automatically displayed, atachment=need user action to open)
if ($attachment) header('Content-Disposition: attachment; filename="'.$filename.'"');
else header('Content-Disposition: inline; filename="'.$filename.'"');
header('Content-Length: ' . dol_filesize($original_file));
header('Content-Length: ' . dol_filesize($fullpath_original_file));
// Ajout directives pour resoudre bug IE
header('Cache-Control: Public, must-revalidate');
header('Pragma: public');

//ob_clean();
//flush();

readfile($original_file_osencoded);
readfile($fullpath_original_file_osencoded);

if (is_object($db)) $db->close();
3 changes: 2 additions & 1 deletion htdocs/langs/en_US/main.lang
View file
Expand Up @@ -103,7 +103,8 @@ RequestLastAccessInError=Latest database access request error
ReturnCodeLastAccessInError=Return code for latest database access request error
InformationLastAccessInError=Information for latest database access request error
DolibarrHasDetectedError=Dolibarr has detected a technical error
InformationToHelpDiagnose=This information can be useful for diagnostic purposes
YouCanSetOptionDolibarrMainProdToZero=You can read log file or set option $dolibarr_main_prod to '0' in your config file to get more information.
InformationToHelpDiagnose=This information can be useful for diagnostic purposes (you can set option $dolibarr_main_prod to '1' to remove such notices)
MoreInformation=More information
TechnicalInformation=Technical information
TechnicalID=Technical ID
Expand Down
41 changes: 21 additions & 20 deletions htdocs/main.inc.php
View file
Expand Up @@ -77,38 +77,39 @@ function stripslashes_deep($value)
*/
function test_sql_and_script_inject($val, $type)
{
$sql_inj = 0;
$inj = 0;
// For SQL Injection (only GET and POST are used to be included into bad escaped SQL requests)
if ($type != 2)
{
$sql_inj += preg_match('/delete\s+from/i', $val);
$sql_inj += preg_match('/create\s+table/i', $val);
$sql_inj += preg_match('/update.+set.+=/i', $val);
$sql_inj += preg_match('/insert\s+into/i', $val);
$sql_inj += preg_match('/select.+from/i', $val);
$sql_inj += preg_match('/union.+select/i', $val);
$sql_inj += preg_match('/into\s+(outfile|dumpfile)/i', $val);
$sql_inj += preg_match('/(\.\.%2f)+/i', $val);
$inj += preg_match('/delete\s+from/i', $val);
$inj += preg_match('/create\s+table/i', $val);
$inj += preg_match('/update.+set.+=/i', $val);
$inj += preg_match('/insert\s+into/i', $val);
$inj += preg_match('/select.+from/i', $val);
$inj += preg_match('/union.+select/i', $val);
$inj += preg_match('/into\s+(outfile|dumpfile)/i', $val);
$inj += preg_match('/(\.\.%2f)+/i', $val);
}
// For XSS Injection done by adding javascript with script
// This is all cases a browser consider text is javascript:
// When it found '<script', 'javascript:', '<style', 'onload\s=' on body tag, '="&' on a tag size with old browsers
// All examples on page: http://ha.ckers.org/xss.html#XSScalc
$sql_inj += preg_match('/<script/i', $val);
if (! defined('NOSTYLECHECK')) $sql_inj += preg_match('/<style/i', $val);
$sql_inj += preg_match('/base[\s]+href/si', $val);
$sql_inj += preg_match('/<.*onmouse/si', $val); // onmousexxx can be set on img or any html tag like <img title='...' onmouseover=alert(1)>
$sql_inj += preg_match('/onerror\s*=/i', $val); // onerror can be set on img or any html tag like <img title='...' onerror = alert(1)>
$sql_inj += preg_match('/onfocus\s*=/i', $val); // onfocus can be set on input text html tag like <input type='text' value='...' onfocus = alert(1)>
$inj += preg_match('/<script/i', $val);
if (! defined('NOSTYLECHECK')) $inj += preg_match('/<style/i', $val);
$inj += preg_match('/base[\s]+href/si', $val);
$inj += preg_match('/<.*onmouse/si', $val); // onmousexxx can be set on img or any html tag like <img title='...' onmouseover=alert(1)>
$inj += preg_match('/onerror\s*=/i', $val); // onerror can be set on img or any html tag like <img title='...' onerror = alert(1)>
$inj += preg_match('/onfocus\s*=/i', $val); // onfocus can be set on input text html tag like <input type='text' value='...' onfocus = alert(1)>
$inj += preg_match('/onload\s*=/i', $val); // onload can be set on input text html tag like <input type='text' value='...' onfocus = alert(1)>
if ($type == 1)
{
$sql_inj += preg_match('/javascript:/i', $val);
$sql_inj += preg_match('/vbscript:/i', $val);
$inj += preg_match('/javascript:/i', $val);
$inj += preg_match('/vbscript:/i', $val);
}
// For XSS Injection done by adding javascript closing html tags like with onmousemove, etc... (closing a src or href tag with not cleaned param)
if ($type == 1) $sql_inj += preg_match('/"/i', $val); // We refused " in GET parameters value
if ($type == 2) $sql_inj += preg_match('/[;"]/', $val); // PHP_SELF is a file system path. It can contains spaces.
return $sql_inj;
if ($type == 1) $inj += preg_match('/"/i', $val); // We refused " in GET parameters value
if ($type == 2) $inj += preg_match('/[;"]/', $val); // PHP_SELF is a file system path. It can contains spaces.
return $inj;
}

/**
Expand Down

1 comment on commit d26b2a6

@eldy
Copy link
Member Author

@eldy eldy commented on d26b2a6 Sep 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIX SQL injection on admin/menus/edit.php parameter menuId. Discovered by ADLab of Venustech
FIX Multiple cross-site scripting (XSS) on admin/company.php parameter CompanyName,CompanyAddress,CompanyZip,CompanyTown,Phone.... Discovered by ADLab of Venustech
FIX cross-site scripting (XSS) on admin/menus/edit.php parameter Title Discovered by ADLab of Venustech
FIX Sensitive information disclosure on document.php parameter file Discovered by ADLab of Venustech

Please sign in to comment.