From a207365bd2f3373405e04ebd292b29529d3b4217 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Wed, 27 May 2020 13:12:18 +0200 Subject: [PATCH] FIX XSS using the renaming of .noexe files - reported by Nolan. --- htdocs/core/class/html.formfile.class.php | 3 +- htdocs/core/lib/files.lib.php | 27 +++++++++-------- htdocs/core/tpl/filemanager.tpl.php | 4 +-- htdocs/ecm/class/ecmfiles.class.php | 36 ++++++++++++----------- htdocs/ecm/file_card.php | 26 ++++++++++++---- htdocs/ecm/index.php | 6 ++-- htdocs/langs/en_US/errors.lang | 1 + 7 files changed, 63 insertions(+), 40 deletions(-) diff --git a/htdocs/core/class/html.formfile.class.php b/htdocs/core/class/html.formfile.class.php index ec9dff5f1a3..97a4bdf1948 100644 --- a/htdocs/core/class/html.formfile.class.php +++ b/htdocs/core/class/html.formfile.class.php @@ -1269,7 +1269,8 @@ class FormFile } else { - print dol_trunc($file['name'], 200); + $filenametoshow = preg_replace('/\.noexe$/', '', $file['name']); + print dol_trunc($filenametoshow, 200); print ''; } // Preview link diff --git a/htdocs/core/lib/files.lib.php b/htdocs/core/lib/files.lib.php index 41d7bf1b294..6614b22e5ed 100644 --- a/htdocs/core/lib/files.lib.php +++ b/htdocs/core/lib/files.lib.php @@ -870,7 +870,7 @@ function dol_move($srcfile, $destfile, $newmask = 0, $overwriteifexists = 1, $te { $rel_filetorenamebefore = preg_replace('/^[\\/]/', '', $rel_filetorenamebefore); $rel_filetorenameafter = preg_replace('/^[\\/]/', '', $rel_filetorenameafter); - //var_dump($rel_filetorenamebefore.' - '.$rel_filetorenameafter); + //var_dump($rel_filetorenamebefore.' - '.$rel_filetorenameafter);exit; dol_syslog("Try to rename also entries in database for full relative path before = ".$rel_filetorenamebefore." after = ".$rel_filetorenameafter, LOG_DEBUG); include_once DOL_DOCUMENT_ROOT.'/ecm/class/ecmfiles.class.php'; @@ -893,6 +893,7 @@ function dol_move($srcfile, $destfile, $newmask = 0, $overwriteifexists = 1, $te $ecmfile->filepath = $rel_dir; $ecmfile->filename = $filename; + $resultecm = $ecmfile->update($user); } elseif ($resultecm == 0) // If no entry were found for src files, create/update target file @@ -995,7 +996,7 @@ function dolCheckVirus($src_file) * @param integer $uploaderrorcode Value of PHP upload error code ($_FILES['field']['error']) * @param int $nohook Disable all hooks * @param string $varfiles _FILES var name - * @return int|string >0 if OK, <0 or string if KO + * @return int|string 1 if OK, 2 if OK and .noexe appended, <0 or string if KO * @see dol_move() */ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disablevirusscan = 0, $uploaderrorcode = 0, $nohook = 0, $varfiles = 'addedfile') @@ -1005,6 +1006,7 @@ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disable $reshook = 0; $file_name = $dest_file; + $successcode = 1; if (empty($nohook)) { @@ -1055,6 +1057,7 @@ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disable if (isAFileWithExecutableContent($dest_file) && empty($conf->global->MAIN_DOCUMENT_IS_OUTSIDE_WEBROOT_SO_NOEXE_NOT_REQUIRED)) { $file_name .= '.noexe'; + $successcode = 2; } // Security: @@ -1109,7 +1112,7 @@ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disable { if (!empty($conf->global->MAIN_UMASK)) @chmod($file_name_osencoded, octdec($conf->global->MAIN_UMASK)); dol_syslog("Files.lib::dol_move_uploaded_file Success to move ".$src_file." to ".$file_name." - Umask=".$conf->global->MAIN_UMASK, LOG_DEBUG); - return 1; // Success + return $successcode; // Success } else { @@ -1118,7 +1121,7 @@ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disable } } - return 1; // Success + return $successcode; // Success } /** @@ -1173,8 +1176,6 @@ function dol_delete_file($file, $disableglob = 0, $nophperrors = 0, $nohook = 0, } else { - $error = 0; - //print "x".$file." ".$disableglob;exit; $file_osencoded = dol_osencode($file); // New filename encoded in OS filesystem encoding charset if (empty($disableglob) && !empty($file_osencoded)) @@ -1197,10 +1198,11 @@ function dol_delete_file($file, $disableglob = 0, $nophperrors = 0, $nohook = 0, $rel_filetodelete = preg_replace('/^'.preg_quote(DOL_DATA_ROOT, '/').'/', '', $filename); if (!preg_match('/(\/temp\/|\/thumbs\/|\.meta$)/', $rel_filetodelete)) // If not a tmp file { - $rel_filetodelete = preg_replace('/^[\\/]/', '', $rel_filetodelete); - if (is_object($db) && $indexdatabase) // $db may not be defined when lib is in a context with define('NOREQUIREDB',1) { + $rel_filetodelete = preg_replace('/^[\\/]/', '', $rel_filetodelete); + $rel_filetodelete = preg_replace('/\.noexe$/', '', $rel_filetodelete); + dol_syslog("Try to remove also entries in database for full relative path = ".$rel_filetodelete, LOG_DEBUG); include_once DOL_DOCUMENT_ROOT.'/ecm/class/ecmfiles.class.php'; $ecmfile = new EcmFiles($db); @@ -1564,6 +1566,7 @@ function dol_add_file_process($upload_dir, $allowoverwrite = 0, $donotupdatesess $destfile = dol_string_nohtmltag($destfile); $destfull = dol_string_nohtmltag($destfull); + // Move file from temp directory to final directory. A .noexe may also be appended on file name. $resupload = dol_move_uploaded_file($TFile['tmp_name'][$i], $destfull, $allowoverwrite, 0, $TFile['error'][$i], 0, $varfiles); if (is_numeric($resupload) && $resupload > 0) // $resupload can be 'ErrorFileAlreadyExists' @@ -1600,10 +1603,10 @@ function dol_add_file_process($upload_dir, $allowoverwrite = 0, $donotupdatesess // Update table of files if ($donotupdatesession == 1) { - $result = addFileIntoDatabaseIndex($upload_dir, basename($destfile), $TFile['name'][$i], 'uploaded', 0); + $result = addFileIntoDatabaseIndex($upload_dir, basename($destfile).($resupload == 2 ? '.noexe' : ''), $TFile['name'][$i], 'uploaded', 0); if ($result < 0) { - setEventMessages('FailedToAddFileIntoDatabaseIndex', '', 'warnings'); + setEventMessages('WarningFailedToAddFileIntoDatabaseIndex', '', 'warnings'); } } @@ -1714,7 +1717,7 @@ function dol_remove_file_process($filenb, $donotupdatesession = 0, $donotdeletef * See also commonGenerateDocument that also add/update database index when a file is generated. * * @param string $dir Directory name (full real path without ending /) - * @param string $file File name + * @param string $file File name (May end with '.noexe') * @param string $fullpathorig Full path of origin for file (can be '') * @param string $mode How file was created ('uploaded', 'generated', ...) * @param int $setsharekey Set also the share key @@ -1730,7 +1733,7 @@ function addFileIntoDatabaseIndex($dir, $file, $fullpathorig = '', $mode = 'uplo if (!preg_match('/[\\/]temp[\\/]|[\\/]thumbs|\.meta$/', $rel_dir)) // If not a tmp dir { - $filename = basename($file); + $filename = basename(preg_replace('/\.noexe$/', '', $file)); $rel_dir = preg_replace('/[\\/]$/', '', $rel_dir); $rel_dir = preg_replace('/^[\\/]/', '', $rel_dir); diff --git a/htdocs/core/tpl/filemanager.tpl.php b/htdocs/core/tpl/filemanager.tpl.php index 3a3395432de..f95286bbcfc 100644 --- a/htdocs/core/tpl/filemanager.tpl.php +++ b/htdocs/core/tpl/filemanager.tpl.php @@ -178,7 +178,7 @@ if (empty($action) || $action == 'editfile' || $action == 'file_manager' || preg print ''; } - else // Show filtree when ajax is disabled (rare) + else // Show file tree when ajax is disabled (rare) { print ''; @@ -212,7 +212,7 @@ if (empty($action) || $action == 'editfile' || $action == 'file_manager' || preg
entity = trim($this->entity); } if (isset($this->filename)) { - $this->filename = trim($this->filename); + $this->filename = preg_replace('/\.noexe$/', '', trim($this->filename)); } if (isset($this->filepath)) { $this->filepath = trim($this->filepath); @@ -346,12 +346,13 @@ class EcmFiles extends CommonObject $sql .= " t.src_object_id"; $sql .= ' FROM ' . MAIN_DB_PREFIX . $this->table_element . ' as t'; $sql.= ' WHERE 1 = 1'; - /* Fetching this table depends on filepath+filename, it must not depends on entity + /* Fetching this table depends on filepath+filename, it must not depends on entity because filesystem on disk does not know what is Dolibarr entities if (! empty($conf->multicompany->enabled)) { $sql .= " AND entity IN (" . getEntity('ecmfiles') . ")"; }*/ if ($relativepath) { - $sql .= " AND t.filepath = '" . $this->db->escape(dirname($relativepath)) . "' AND t.filename = '".$this->db->escape(basename($relativepath))."'"; + $relativepathwithnoexe = preg_replace('/\.noexe$/', '', $relativepath); // We must never have the .noexe into the database + $sql .= " AND t.filepath = '" . $this->db->escape(dirname($relativepath)) . "' AND t.filename = '".$this->db->escape(basename($relativepathwithnoexe))."'"; $sql .= " AND t.entity = ".$conf->entity; // unique key include the entity so each company has its own index } elseif (! empty($ref)) { // hash of file path @@ -552,46 +553,47 @@ class EcmFiles extends CommonObject // Clean parameters if (isset($this->ref)) { - $this->ref = trim($this->ref); + $this->ref = trim($this->ref); } if (isset($this->label)) { - $this->label = trim($this->label); + $this->label = trim($this->label); } if (isset($this->share)) { - $this->share = trim($this->share); + $this->share = trim($this->share); } if (isset($this->entity)) { - $this->entity = trim($this->entity); + $this->entity = trim($this->entity); } if (isset($this->filename)) { - $this->filename = trim($this->filename); + $this->filename = preg_replace('/\.noexe$/', '', trim($this->filename)); } if (isset($this->filepath)) { - $this->filepath = trim($this->filepath); + $this->filepath = trim($this->filepath); + $this->filepath = preg_replace('/[\\/]+$/', '', $this->filepath); // Remove last / } if (isset($this->fullpath_orig)) { - $this->fullpath_orig = trim($this->fullpath_orig); + $this->fullpath_orig = trim($this->fullpath_orig); } if (isset($this->description)) { - $this->description = trim($this->description); + $this->description = trim($this->description); } if (isset($this->keywords)) { - $this->keywords = trim($this->keywords); + $this->keywords = trim($this->keywords); } if (isset($this->cover)) { - $this->cover = trim($this->cover); + $this->cover = trim($this->cover); } if (isset($this->gen_or_uploaded)) { - $this->gen_or_uploaded = trim($this->gen_or_uploaded); + $this->gen_or_uploaded = trim($this->gen_or_uploaded); } if (isset($this->extraparams)) { - $this->extraparams = trim($this->extraparams); + $this->extraparams = trim($this->extraparams); } if (isset($this->fk_user_m)) { - $this->fk_user_m = trim($this->fk_user_m); + $this->fk_user_m = trim($this->fk_user_m); } if (isset($this->acl)) { - $this->acl = trim($this->acl); + $this->acl = trim($this->acl); } if (isset($this->src_object_type)) { $this->src_object_type = trim($this->src_object_type); diff --git a/htdocs/ecm/file_card.php b/htdocs/ecm/file_card.php index 3664a2e7f28..e1796068370 100644 --- a/htdocs/ecm/file_card.php +++ b/htdocs/ecm/file_card.php @@ -139,6 +139,12 @@ if ($action == 'update') $oldfile = $olddir.$oldlabel; $newfile = $newdir.$newlabel; + $newfileformove = $newfile; + // If old file end with .noexe, new file must also end with .noexe + if (preg_match('/\.noexe$/', $oldfile) && ! preg_match('/\.noexe$/', $newfileformove)) { + $newfileformove .= '.noexe'; + } + //var_dump($oldfile);var_dump($newfile);exit; // Now we update index of file $db->begin(); @@ -146,7 +152,7 @@ if ($action == 'update') //print $oldfile.' - '.$newfile; if ($newlabel != $oldlabel) { - $result = dol_move($oldfile, $newfile); // This include update of database + $result = dol_move($oldfile, $newfileformove); // This include update of database if (!$result) { $langs->load('errors'); @@ -190,7 +196,7 @@ if ($action == 'update') $object->entity = $conf->entity; $object->filepath = preg_replace('/[\\/]+$/', '', $newdirrelativetodocument); $object->filename = $newlabel; - $object->label = md5_file(dol_osencode($newfile)); // hash of file content + $object->label = md5_file(dol_osencode($newfileformove)); // hash of file content $object->fullpath_orig = ''; $object->gen_or_uploaded = 'unknown'; $object->description = ''; // indexed content @@ -208,6 +214,11 @@ if ($action == 'update') $db->commit(); $urlfile = $newlabel; + // If old file end with .noexe, new file must also end with .noexe + if (preg_match('/\.noexe$/', $newfileformove)) { + $urlfile .= '.noexe'; + } + header('Location: '.$_SERVER["PHP_SELF"].'?urlfile='.urlencode($urlfile).'§ion='.urlencode($section)); exit; } @@ -264,9 +275,13 @@ while ($tmpecmdir && $result > 0) $i++; } +$urlfiletoshow = preg_replace('/\.noexe$/', '', $urlfile); + $s = img_picto('', 'object_dir').' '.$langs->trans("ECMRoot").' -> '.$s.' -> '; -if ($action == 'edit') $s .= ''; -else $s .= $urlfile; +if ($action == 'edit') $s .= ''; +else $s .= $urlfiletoshow; + +$morehtml = ''; $object->ref = ''; // Force to hide ref dol_banner_tab($object, '', $morehtml, 0, '', '', $s); @@ -289,10 +304,9 @@ print dol_print_size($totalsize); print ''; */ +// Hash of file content print ''.$langs->trans("HashOfFileContent").''; $object = new EcmFiles($db); -//$filenametosearch=basename($filepath); -//$filedirtosearch=basedir($filepath); $object->fetch(0, '', $filepathtodocument); if (!empty($object->label)) { diff --git a/htdocs/ecm/index.php b/htdocs/ecm/index.php index e51efb01aaa..852d148e258 100644 --- a/htdocs/ecm/index.php +++ b/htdocs/ecm/index.php @@ -124,15 +124,17 @@ if ($action == 'confirm_deletefile') if (GETPOST('confirm') == 'yes') { // GETPOST('urlfile','alpha') is full relative URL from ecm root dir. Contains path of all sections. - //var_dump(GETPOST('urlfile'));exit; $upload_dir = $conf->ecm->dir_output.($relativepath?'/'.$relativepath:''); $file = $upload_dir . "/" . GETPOST('urlfile', 'alpha'); + //var_dump($file);exit; $ret=dol_delete_file($file); // This include also the delete from file index in database. if ($ret) { - setEventMessages($langs->trans("FileWasRemoved", GETPOST('urlfile', 'alpha')), null, 'mesgs'); + $urlfiletoshow = GETPOST('urlfile', 'alpha'); + $urlfiletoshow = preg_replace('/\.noexe$/', '', $urlfiletoshow); + setEventMessages($langs->trans("FileWasRemoved", $urlfiletoshow), null, 'mesgs'); $result=$ecmdir->changeNbOfFiles('-'); } else diff --git a/htdocs/langs/en_US/errors.lang b/htdocs/langs/en_US/errors.lang index c5b61e50ed6..d145e75bad6 100644 --- a/htdocs/langs/en_US/errors.lang +++ b/htdocs/langs/en_US/errors.lang @@ -255,3 +255,4 @@ WarningNumberOfRecipientIsRestrictedInMassAction=Warning, number of different re WarningDateOfLineMustBeInExpenseReportRange=Warning, the date of line is not in the range of the expense report WarningProjectClosed=Project is closed. You must re-open it first. WarningSomeBankTransactionByChequeWereRemovedAfter=Some bank transaction were removed after that the receipt including them were generated. So nb of cheques and total of receipt may differ from number and total in list. +WarningFailedToAddFileIntoDatabaseIndex=Warnin, failed to add file entry into ECM database index table \ No newline at end of file