Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix disallow -- string into filename for security purpose. Vulnerability
reported by Yılmaz Değirmenci
  • Loading branch information
eldy committed Dec 11, 2020
1 parent 89854ea commit 4fcd3fe
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 4 deletions.
2 changes: 1 addition & 1 deletion htdocs/admin/tools/export_files.php
Expand Up @@ -34,7 +34,7 @@
$what = GETPOST('what', 'alpha');
$export_type = GETPOST('export_type', 'alpha');
$file = trim(GETPOST('zipfilename_template', 'alpha'));
$compression = GETPOST('compression');
$compression = GETPOST('compression', 'aZ09');

$file = dol_sanitizeFileName($file);
$file = preg_replace('/(\.zip|\.tar|\.tgz|\.gz|\.tar\.gz|\.bz2)$/i', '', $file);
Expand Down
5 changes: 3 additions & 2 deletions htdocs/core/lib/functions.lib.php
Expand Up @@ -866,8 +866,9 @@ function dol_sanitizeFileName($str, $newstr = '_', $unaccent = 1)
// List of special chars for filenames in windows are defined on page https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
// Char '>' '<' '|' '$' and ';' are special chars for shells.
// Char '/' and '\' are file delimiters.
$filesystem_forbidden_chars = array('<', '>', '/', '\\', '?', '*', '|', '"', ':', '°', '$', ';');
return dol_string_nospecial($unaccent ?dol_string_unaccent($str) : $str, $newstr, $filesystem_forbidden_chars);
// -- car can be used into filename to inject special paramaters like --use-compress-program to make command with file as parameter making remote execution of command
$filesystem_forbidden_chars = array('<', '>', '/', '\\', '?', '*', '|', '"', ':', '°', '$', ';', '--');
return dol_string_nospecial($unaccent ? dol_string_unaccent($str) : $str, $newstr, $filesystem_forbidden_chars);
}

/**
Expand Down
25 changes: 24 additions & 1 deletion test/phpunit/SecurityTest.php
Expand Up @@ -244,7 +244,7 @@ public function testCheckLoginPassEntity()

$login=checkLoginPassEntity('admin', 'admin', 1, array('dolibarr')); // Should works because admin/admin exists
print __METHOD__." login=".$login."\n";
$this->assertEquals($login, 'admin');
$this->assertEquals($login, 'admin', 'The test to check if pass of user "admin" is "admin" has failed');

$login=checkLoginPassEntity('admin', 'admin', 1, array('http','dolibarr')); // Should work because of second authetntication method
print __METHOD__." login=".$login."\n";
Expand Down Expand Up @@ -326,4 +326,27 @@ public function testRestrictedArea()
$result=restrictedArea($user, 'societe');
$this->assertEquals(1, $result);
}

/**
* testDolSanitizeFileName
*
* @return void
*/
public function testDolSanitizeFileName()
{
global $conf,$user,$langs,$db;
$conf=$this->savconf;
$user=$this->savuser;
$langs=$this->savlangs;
$db=$this->savdb;

//$dummyuser=new User($db);
//$result=restrictedArea($dummyuser,'societe');

$result=dol_sanitizeFileName('bad file | evilaction');
$this->assertEquals('bad file _ evilaction', $result);

$result=dol_sanitizeFileName('bad file --evilparam');
$this->assertEquals('bad file _evilparam', $result);
}
}

1 comment on commit 4fcd3fe

@DanielRuf
Copy link

Choose a reason for hiding this comment

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

I think prepared statements should be used.

It is not optimal how the SQL code is written in general:

$sql = "INSERT INTO ".MAIN_DB_PREFIX."product_customer_price(";
$sql .= "entity,";
$sql .= "datec,";
$sql .= "fk_product,";
$sql .= "fk_soc,";
$sql .= "price,";
$sql .= "price_ttc,";
$sql .= "price_min,";
$sql .= "price_min_ttc,";
$sql .= "price_base_type,";
$sql .= "default_vat_code,";
$sql .= "tva_tx,";
$sql .= "recuperableonly,";
$sql .= "localtax1_type,";
$sql .= "localtax1_tx,";
$sql .= "localtax2_type,";
$sql .= "localtax2_tx,";
$sql .= "fk_user,";
$sql .= "import_key";
$sql .= ") VALUES (";
$sql .= " ".$conf->entity.",";
$sql .= " '".$this->db->idate(dol_now())."',";
$sql .= " ".(!isset($this->fk_product) ? 'NULL' : "'".$this->db->escape($this->fk_product)."'").",";
$sql .= " ".(!isset($this->fk_soc) ? 'NULL' : "'".$this->db->escape($this->fk_soc)."'").",";
$sql .= " ".(empty($this->price) ? '0' : "'".$this->db->escape($this->price)."'").",";
$sql .= " ".(empty($this->price_ttc) ? '0' : "'".$this->db->escape($this->price_ttc)."'").",";
$sql .= " ".(empty($this->price_min) ? '0' : "'".$this->db->escape($this->price_min)."'").",";
$sql .= " ".(empty($this->price_min_ttc) ? '0' : "'".$this->db->escape($this->price_min_ttc)."'").",";
$sql .= " ".(!isset($this->price_base_type) ? 'NULL' : "'".$this->db->escape($this->price_base_type)."'").",";
$sql .= " ".($this->default_vat_code ? "'".$this->db->escape($this->default_vat_code)."'" : "null").",";
$sql .= " ".(!isset($this->tva_tx) ? 'NULL' : (empty($this->tva_tx) ? 0 : $this->tva_tx)).",";
$sql .= " ".(!isset($this->recuperableonly) ? 'NULL' : "'".$this->db->escape($this->recuperableonly)."'").",";
$sql .= " ".(empty($this->localtax1_type) ? "'0'" : "'".$this->db->escape($this->localtax1_type)."'").",";
$sql .= " ".(!isset($this->localtax1_tx) ? 'NULL' : (empty($this->localtax1_tx) ? 0 : $this->localtax1_tx)).",";
$sql .= " ".(empty($this->localtax2_type) ? "'0'" : "'".$this->db->escape($this->localtax2_type)."'").",";
$sql .= " ".(!isset($this->localtax2_tx) ? 'NULL' : (empty($this->localtax2_tx) ? 0 : $this->localtax2_tx)).",";
$sql .= " ".$user->id.",";
$sql .= " ".(!isset($this->import_key) ? 'NULL' : "'".$this->db->escape($this->import_key)."'")."";
$sql .= ")";

Are there such plans to migrate to prepared statements using mysqli / PDO?

Please sign in to comment.