Update addArg() to improve escaping (fix #44)

mikehaertl · Dec 19, 2019 · 2bab79f · 2bab79f
1 parent 6c6f44c
commit 2bab79f
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 16 deletions.
diff --git a/src/Command.php b/src/Command.php
@@ -272,8 +272,8 @@ public function getArgs()
      * @param string|array|null $value the optional argument value which will
      * get escaped if $escapeArgs is true.  An array can be passed to add more
      * than one value for a key, e.g. `addArg('--exclude',
-     * array('val1','val2'))` which will create the option `--exclude 'val1'
-     * 'val2'`.
+     * array('val1','val2'))` which will create the option `'--exclude' 'val1'
+     * '--exclude' 'val2'`.
      * @param bool|null $escape if set, this overrides the $escapeArgs setting
      * and enforces escaping/no escaping
      * @return static for method chaining
@@ -288,19 +288,22 @@ public function addArg($key, $value = null, $escape = null)
             setlocale(LC_CTYPE, $this->locale);
         }
         if ($value === null) {
-            // Only escape single arguments if explicitely requested
-            $this->_args[] = $escape ? escapeshellarg($key) : $key;
+            $this->_args[] = $doEscape ? escapeshellarg($key) : $key;
         } else {
-            $separator = substr($key, -1)==='=' ? '' : ' ';
             if (is_array($value)) {
-                $params = array();
                 foreach ($value as $v) {
-                    $params[] = $doEscape ? escapeshellarg($v) : $v;
+                    $this->addArg($key, $v, $escape);
                 }
-                $this->_args[] = $key . $separator.implode(' ',$params);
             } else {
-                $this->_args[] = $key . $separator .
-                    ($doEscape ? escapeshellarg($value) : $value);
+                if (substr($key, -1) === '=') {
+                    $this->_args[] = $doEscape ?
+                        escapeshellarg($key . $value) :
+                        $key . $value;
+                } else {
+                    $this->_args[] = $doEscape ?
+                        escapeshellarg($key) . ' ' . escapeshellarg($value) :
+                        $key . ' ' . $value;
+                }
             }
         }
         if ($useLocale) {

diff --git a/tests/BlockingCommandTest.php b/tests/BlockingCommandTest.php
@@ -62,7 +62,7 @@ public function testCanRunCommandWithArguments()
         $command->nonBlockingMode = false;
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", $command->getExecCommand());
+        $this->assertEquals("ls '-l' '-n'", $command->getExecCommand());
         $this->assertFalse($command->getExecuted());
         $this->assertTrue($command->execute());
         $this->assertTrue($command->getExecuted());

diff --git a/tests/CommandTest.php b/tests/CommandTest.php
@@ -81,8 +81,8 @@ public function testCanAddArguments()
         $command->addArg('-b=', array('v4','v5','v6'));
         $command->addArg('-c', '');
         $command->addArg('some name', null, true);
-        $this->assertEquals("--arg1=x --a --a '中文字äüp' --a 'v'\''1' 'v2' 'v3' -b=v -b='v4' 'v5' 'v6' -c '' 'some name'", $command->getArgs());
-        $this->assertEquals("test --arg1=x --a --a '中文字äüp' --a 'v'\''1' 'v2' 'v3' -b=v -b='v4' 'v5' 'v6' -c '' 'some name'", $command->getExecCommand());
+        $this->assertEquals("--arg1=x '--a' '--a' '中文字äüp' '--a' 'v'\''1' '--a' 'v2' '--a' 'v3' -b=v '-b=v4' '-b=v5' '-b=v6' '-c' '' 'some name'", $command->getArgs());
+        $this->assertEquals("test --arg1=x '--a' '--a' '中文字äüp' '--a' 'v'\''1' '--a' 'v2' '--a' 'v3' -b=v '-b=v4' '-b=v5' '-b=v6' '-c' '' 'some name'", $command->getExecCommand());
     }
     public function testCanResetArguments()
     {
@@ -102,14 +102,29 @@ public function testCanDisableEscaping()
         $command->addArg('-b=','v', true);
         $command->addArg('-b=', array('v4','v5','v6'));
         $command->addArg('some name', null, true);
-        $this->assertEquals("--a --a v --a v1 v2 v3 -b='v' -b=v4 v5 v6 'some name'", $command->getArgs());
+        $this->assertEquals("--a --a v --a v1 --a v2 --a v3 '-b=v' -b=v4 -b=v5 -b=v6 'some name'", $command->getArgs());
+    }
+    public function testCanPreventCommandInjection()
+    {
+        $command = new Command(array(
+            'command' => 'curl',
+        ));
+        $command->addArg('http://example.com --wrong-argument || echo "RCE 1"');
+        $this->assertEquals("'http://example.com --wrong-argument || echo \"RCE 1\"'", $command->getArgs());
+
+        $command = new Command(array(
+            'command' => 'curl',
+        ));
+        $command->addArg('http://example.com');
+        $command->addArg('--header foo --wrong-argument || echo "RCE 2" ||', 'bar');
+        $this->assertEquals("'http://example.com' '--header foo --wrong-argument || echo \"RCE 2\" ||' 'bar'", $command->getArgs());
     }
     public function testCanRunCommandWithArguments()
     {
         $command = new Command('ls');
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", $command->getExecCommand());
+        $this->assertEquals("ls '-l' '-n'", $command->getExecCommand());
         $this->assertFalse($command->getExecuted());
         $this->assertTrue($command->execute());
         $this->assertTrue($command->getExecuted());
@@ -163,7 +178,7 @@ public function testCanCastToString()
         $command = new Command('ls');
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", (string)$command);
+        $this->assertEquals("ls '-l' '-n'", (string)$command);
     }
 
     // Exec