CVE-2020-35136: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') (CWE-88)
value: Dolibarr 12.0.3 is vulnerable to authenticated Remote Code Execution. An attacker who has the access the admin dashboard can manipulate the backup function by inserting a payload into the filename for the zipfilename_template parameter to admin/tools/dolibarr_export.php.
CWE description: The product constructs a string for a command to be executed by a separate component
in another control sphere, but it does not properly delimit the
intended arguments, options, or switches within that command string.
CVSS v3 base score: 7.2
Published: 2020-12-23T15:15Z
Repository: https://github.com/Dolibarr/dolibarr
Language: PHP
Files changed: htdocs/admin/tools/export_files.php, htdocs/core/lib/functions.lib.php, test/phpunit/SecurityTest.php
Commit date: 2020-12-11 15:12:42 +0100
Commit message: Fix disallow -- string into filename for security purpose. Vulnerability
reported by Yılmaz Değirmenci
Diff: diff --git a/htdocs/admin/tools/export_files.php b/htdocs/admin/tools/export_files.php
index de10759c92d4..587750d129ea 100644
--- a/htdocs/admin/tools/export_files.php
+++ b/htdocs/admin/tools/export_files.php
@@ -32,11 +32,11 @@ $langs->load("admin");
 
 $action = GETPOST('action', 'alpha');
 $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);
 
 $sortfield = GETPOST('sortfield', 'alpha');
diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php
index 5094fad392ac..37791d1e72dd 100644
--- a/htdocs/core/lib/functions.lib.php
+++ b/htdocs/core/lib/functions.lib.php
@@ -864,12 +864,13 @@ function dol_size($size, $type = '')
 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);
 }
 
 /**
  *	Clean a string to use it as a path name
  *
diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php
index 5a2480064982..6d2a3cf3cd86 100644
--- a/test/phpunit/SecurityTest.php
+++ b/test/phpunit/SecurityTest.php
@@ -242,11 +242,11 @@ class SecurityTest extends PHPUnit\Framework\TestCase
         print __METHOD__." login=".$login."\n";
         $this->assertEquals($login, '');
 
         $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";
         $this->assertEquals($login, 'admin');
 
@@ -324,6 +324,29 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		//$result=restrictedArea($dummyuser,'societe');
 
 		$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);
+    }
 }

Vulnerable code: $compression = GETPOST('compression');
	$filesystem_forbidden_chars = array('<', '>', '/', '\\', '?', '*', '|', '"', ':', '°', '$', ';');
	return dol_string_nospecial($unaccent ?dol_string_unaccent($str) : $str, $newstr, $filesystem_forbidden_chars);
        $this->assertEquals($login, 'admin');
Fixed code: $compression = GETPOST('compression', 'aZ09');
	// -- 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);
        $this->assertEquals($login, 'admin', 'The test to check if pass of user "admin" is "admin" has failed');

    /**
     * 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);
    }